Skip to content

fix: delete-area hit testing at non-default zoom (#10095) - #10096

Merged
mikeharv merged 5 commits into
RaspberryPiFoundation:mainfrom
mikeharv:zoom-delete
Jul 2, 2026
Merged

fix: delete-area hit testing at non-default zoom (#10095)#10096
mikeharv merged 5 commits into
RaspberryPiFoundation:mainfrom
mikeharv:zoom-delete

Conversation

@mikeharv

@mikeharv mikeharv commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

The basics

The details

Resolves

Fixes #10095

Proposed Changes

Hit-test delete areas now us client/viewport coordinates (clientX/clientY) instead of a client-to-workspace-to-client round-trip

Reason for Changes

After #10078, delete styling would still appear at non-default zoom but blocks were not reliably deleted and the trashcan lid did not animate. The coordinate round-trip misaligned hit tests with getBoundingClientRect() at certain zoom levels, but using client coordinates fixes that.

Test Coverage

  • Extended dragger_test.js with generated trashcan/toolbox delete tests at zoom 0.7 and 1.5 (delete style, trashcan lid, disposal)
  • Generated extra mutator workspace tests with a zoomed parent workspace (delete on pointer-over-area; no false positive on origin-only overlap). Unrelated to bug report, but felt useful enough to add.

@mikeharv
mikeharv requested a review from a team as a code owner July 2, 2026 16:23
@mikeharv
mikeharv requested a review from lizschwab July 2, 2026 16:23
@github-actions github-actions Bot added the PR: fix Fixes a bug label Jul 2, 2026
@mikeharv

mikeharv commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

CI on this PR is failing because main has a package.json / package-lock.json override mismatch (npm ci missing eslint@10.3.0 / prettier@3.8.3). That is unrelated to the delete-area fix.

I opened a separate chore PR to fix main for everyone: #10097

I rebased zoom-delete onto that fix so Node.js CI should pass here once the re-run completes.

@lizschwab lizschwab 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.

Looks good once we get CI happy again

@mikeharv
mikeharv merged commit 9f4f1c1 into RaspberryPiFoundation:main Jul 2, 2026
4 checks passed
@mikeharv
mikeharv deleted the zoom-delete branch July 2, 2026 18:41
@microbit-matt-hillsdon

microbit-matt-hillsdon commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

I think this PR is correct and a net simplification but I was puzzled about the mechanism behind the bug as I felt the coordinates should roundtrip ok.

After poking around with Claude I think this is what went wrong:

  1. Dragger.onDrag computes coordinate once and hands the same object to wouldDeleteDraggable(coordinate) and updateDragTarget(coordinate) (and onDragEnd does something similar).
  2. Each getDragTarget(coordinate) calls wsToScreenCoordinates(this, coordinate).
  3. wsToScreenCoordinates does workspaceCoordinates.scale(ws.scale), and Coordinate.scale() is mutating
  4. So the first call scales the shared object by ws.scale in place; the second call receives an already-×scale coordinate, scales it again, and lands outside the delete area.
  5. At scale = 1 it's a no-op so all's well. At any other zoom it makes a mess.

This explains why the red-cross cursor still shows (call 1, wouldDeleteDraggable, sees the correct coordinate) but things controlled by later calls go wrong (trashcan lid).

From a non-Blockly-team perspective it seems a footgun that wsToScreenCoordinates mutates its argument. This seems like it might be an easy fix. I feel the same way about Coordinate's fluent-but-mutating API but that is a much more disruptive change.

@microbit-matt-hillsdon

Copy link
Copy Markdown
Collaborator

@mikeharv, interested in what you think about the mutability in wsToScreenCoordinates, worth some clean-up?

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

Labels

PR: fix Fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deleting blocks via dragging does not work anymore when zoom isn't on default

3 participants