Skip to content

fix(drawnix): keep freehand selection aligned when zooming - #454

Merged
nightt5879 merged 1 commit into
developfrom
nightt5879/fix-freehand-active-on-viewport
Aug 3, 2026
Merged

fix(drawnix): keep freehand selection aligned when zooming#454
nightt5879 merged 1 commit into
developfrom
nightt5879/fix-freehand-active-on-viewport

Conversation

@nightt5879

Copy link
Copy Markdown
Member

Found while testing zoom behavior. With a freehand element selected, its selection outline could stay at the previous position and scale while the rest of the selection updated.

FreehandComponent was missing the active-section refresh hook, so the outline was not redrawn after viewport changes. This adds the missing hook and a focused regression test.

Tested with:

  • Drawnix tests (26 passed)
  • Drawnix build
  • Manual A/B zoom testing with selected freehand elements

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying drawnix with  Cloudflare Pages  Cloudflare Pages

Latest commit: 783f7f2
Status: ✅  Deploy successful!
Preview URL: https://56add433.drawnix.pages.dev
Branch Preview URL: https://nightt5879-fix-freehand-acti.drawnix.pages.dev

View logs

@nightt5879
nightt5879 marked this pull request as ready for review July 15, 2026 07:49
@nightt5879

Copy link
Copy Markdown
Member Author
2026-07-15.15-40-38.mp4

the move and zoom bug, i test in my computer with plait_PR and this PR, both of them were clear

@pubuzhixing8

Copy link
Copy Markdown
Contributor

Hi @nightt5879 , actually, I didn't reproduce this question, I only found that the selection don't update while I selected some elements and changed pointer to hand, but this problem had been fixed in PR.

@nightt5879

Copy link
Copy Markdown
Member Author

I’ll recheck it later. Maybe I made a mistake.

@nightt5879

Copy link
Copy Markdown
Member Author

Thanks for pointing me to Plait PR #1152. I rechecked everything, and I think my previous video may have caused some confusion because it showed two different issues together.

Here is the distinction:

  1. On the current official Drawnix website, I can reproduce both:

    • the moving/panning issue;
    • the zooming issue with a selected freehand element.
image image image
  1. I tested again with the latest develop branches:

    • Drawnix: db8b5c3
    • Plait: e57a152

    This Plait develop already contains PR #1152. With this combination, the moving/panning issue is fixed, but the freehand zooming issue is still reproducible.

image
  1. PR #1152 fixes the general selection rectangle behavior in hand mode. Its changes are in Plait Core's with-selection.ts and selection.ts, and its regression test checks that board.drawSelectionRectangle refreshes after a viewport change while the pointer is set to hand.

    That is the moving/hand-mode issue you described, and I confirm that it is fixed.

  2. The zooming issue in this PR follows a different refresh path. It is specific to Drawnix's FreehandComponent: when the viewport zoom changes, the selected freehand element is transformed correctly, but its component-specific active outline can remain at the previous position or scale.

    This PR adds the missing updateActiveSection implementation to FreehandComponent, so its active generator redraws the outline using the new viewport state.

  3. The Cloudflare preview generated for this PR is not suitable for reproducing the zooming bug because it already includes this fix. Therefore, it is expected that zooming works correctly in that preview.

So, to summarize:

I hope this clarifies why PR #1152 does not replace the fix in this PR.

@pubuzhixing8

pubuzhixing8 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

But I tested it on my local envirnoment, it works, are there something I don't notice?

Screen.Recording.2026-07-29.at.22.59.52.mov

@nightt5879

Copy link
Copy Markdown
Member Author

The only difference I can think of might be macOS versus Windows. We’re both using Chrome, but I’m on Windows 11 and you’re on a Mac.

At first, I thought the zooming issue only occurred when an image and a freehand stroke were selected together. However, I tested locally with the latest Drawnix develop (d97ecf1) and Plait develop (479b674e) and found that it occurs with any multi-element selection.

It looks like we’ll need to investigate this a little further.

2026-07-30.09-51-55.mp4

@pubuzhixing8

pubuzhixing8 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Thanks for the detailed follow-up. I revisited this locally and have a few findings:

  1. I no longer remember the original reason we added support for updateActiveSection. I commented out the updateActiveSection calls in the wrapper component, and the selected state still refreshed correctly while zooming in my local environment.

  2. Since the behavior may differ on Windows, could you check whether the following branch in freehand.component.ts runs during zooming?

if (needUpdate || value.selected) {
  this.activeGenerator.processDrawing(
    this.element,
    PlaitBoard.getActiveHost(this.board),
    {
      selected: this.selected,
    }
  );
}

For a selected freehand element, value.selected should be true, so this path should execute and redraw the selection state even without an explicit updateActiveSection call. It would be useful to confirm on Windows whether this code is reached during zooming and whether value.selected is indeed true.

  1. I also noticed that a slight misalignment can still occur in some cases. It is subtle, and sometimes it corrects itself afterward. I will investigate that separately later.

For now, let us first determine why the existing redraw path appears to behave differently on Windows.

Screenshot 2026-07-30 at 20 11 26 the issue about point 3

@nightt5879

Copy link
Copy Markdown
Member Author

@pubuzhixing8 I did a side-by-side check and found that this is not actually a macOS vs Windows difference. The difference in our original observations came from the input device: your recording used a trackpad, while mine used a mouse wheel.

Around 100% zoom in our recordings:

  • trackpad pinch zoom changed by about 1 percentage point per update;
  • mouse-wheel zoom changed by 10 percentage points per step.

I also verified that FreehandComponent.onContextChanged does enter the needUpdate || value.selected branch, and value.selected === true.

The actual issue is the timing of that redraw. It runs before initializeViewBox() / updateViewportOffset(), so the freehand selection outline can be calculated using the new zoom with the previous viewport state.

With the trackpad, the smaller and more frequent updates make the offset much less visible, and the next update often masks the previous stale frame. However, when the gesture stops, a slight lag or residual misalignment can still remain. The 10% mouse-wheel step simply makes the same issue much more obvious.

The attached video shows, in order:

  1. before this PR — mouse-wheel zoom;
  2. before this PR — trackpad pinch zoom;
  3. after this PR — mouse-wheel zoom;
  4. after this PR — trackpad pinch zoom.
2026-07-31.21-07-02.mp4

After adding the post-layout updateActiveSection refresh, the freehand selection outline stays aligned in both cases.

So this is the same timing bug being exposed at different magnitudes because of different zoom steps, rather than an OS-specific behavior difference. I believe the current PR is the appropriate scoped fix. What do you think?

@pubuzhixing8 pubuzhixing8 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the careful follow-up and the side-by-side reproduction. This makes the cause much clearer.

The difference we observed is from the input device rather than the operating system:

  • trackpad pinch zoom changes the viewport in small, frequent steps, so the stale active-outline frame is subtle and is often masked by the next update;
  • mouse-wheel zoom changes it in larger steps, so the same timing issue becomes much easier to see.

You also confirmed that FreehandComponent.onContextChanged enters the needUpdate || value.selected branch and that value.selected === true. This matches our investigation: the problem is not that the freehand component fails to receive a selected-state update, but that its first redraw happens before initializeViewBox() / updateViewportOffset(), while the active outline depends on the final viewport state.

Given the current active-host architecture, adding the missing post-layout updateActiveSection refresh is an appropriate scoped fix. It resolves the issue for both mouse-wheel and trackpad zooming, so I am happy for this PR to be merged as a temporary solution. LGTM.

I suggest two non-blocking follow-ups:

  1. Create a separate issue to investigate whether we can eventually remove updateActiveSection by fixing the lifecycle/order between listRender.update and the viewport update. Ideally, active overlays should be refreshed from one consistent viewport state instead of requiring a component-level second pass.

  2. The unit test could be made more representative in a follow-up. Instead of assigning board.viewport.zoom directly and manually invoking the hook, we could try the newer setupTestingBoard / TestingBoardFixture from @plait/core, prepare the host/element host/rough SVG, trigger a real Transforms.setViewport or BoardTransforms.updateZoom, flush the change cycle, and then verify the ordering or replacement of the freehand active drawing. That would cover the real viewport-operation path without requiring fragile pixel-position assertions.

Neither follow-up needs to block this PR. Thanks for tracking down the input-device difference and validating both paths.

@nightt5879
nightt5879 merged commit d010277 into develop Aug 3, 2026
2 checks passed
@nightt5879
nightt5879 deleted the nightt5879/fix-freehand-active-on-viewport branch August 3, 2026 01:44
@nightt5879

Copy link
Copy Markdown
Member Author

Merged, thanks for the review!

I opened #461 to track the two non-blocking follow-ups together: the active-overlay/viewport lifecycle investigation and more representative real viewport-operation regression coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants