fix: guard drag and dragleave handlers without resolving an event position - #3216
Merged
Conversation
… position `handleDrag` and `handleDragLeave` ran `getEventPosition` on every event and used the result only as a bail-out guard: the forwarded `drag.drag` and `drag.dragleave` behavior events carry no position. `getEventPosition` runs `caretPositionFromPoint` (a synchronous hit test) plus first-block, last-block, and target `getBoundingClientRect` reads, and the browser fires `drag` continuously on the drag source and `dragleave` on every element boundary crossed, so the resolution was pure per-pointer-move cost on the drag hot path. Layout dirtied between events (indicator commits, style writes) turns each of those reads into a forced reflow that scales with page size. Guard with the same checks `getEventPosition` front-loads: skip while the editor actor is setting up, and require `DOMEditor.hasTarget(editorEngine, event.target)`. One narrow behavioral delta rides along: in cases where position resolution would have failed beyond those checks (missing block DOM nodes mid-teardown, a failed caret hit-test), the behavior events now fire instead of being swallowed; they carry no payload a consumer could read stale data from. Pinned by a browser test asserting both events reach behaviors and that no caret hit-test runs while dispatching them (red on the old handlers: two `caretPositionFromPoint` calls).
🦋 Changeset detectedLatest commit: 3f623a2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Bundle Stats✅ No significant changes. All scenario measurements (7)🗺️
Significant means at least 1.0 KB and 1% gzip, or at least 5 ms and 10% import time. |
christianhg
marked this pull request as ready for review
September 3, 2026 07:38
christianhg
enabled auto-merge (rebase)
September 3, 2026 07:43
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
During a drag, the editor runs a full
getEventPositionfor everydraganddragleaveevent: acaretPositionFromPointhit-test plus first-block, last-block, and target rect reads. Both handlers use the result only as a bail-out guard; the forwardeddrag.draganddrag.dragleavebehavior events carry no position. The browser firesdragcontinuously on the drag source anddragleaveon every element boundary crossed, so this is wasted work at pointer-move frequency on the same hot path #3214 fixed, and whenever layout is dirty (indicator commits, style writes), each rect read becomes a forced reflow that scales with page size.The handlers now guard with the same checks
getEventPositionfront-loads (editor actor not setting up,DOMEditor.hasTargeton the event target) and skip the resolution. One narrow delta rides along: where position resolution would have failed beyond those checks (missing block DOM nodes mid-teardown, a failed hit-test), the behavior events now fire instead of being swallowed; they carry no payload a consumer could read stale data from.Pinned by a browser test asserting both events reach behaviors and that dispatching them runs no caret hit-test; red on the old handlers, which make two
caretPositionFromPointcalls. Full editor browser suite green (2040 passed).Note
Low Risk
Localized drag-handler performance change with a narrow semantic shift (more behavior events in teardown/hit-test failure cases) and no position payload for consumers to misread.
Overview
draganddragleaveno longer callgetEventPositionbefore forwardingdrag.draganddrag.dragleaveto behaviors. Those behavior events never included a caret position, but each DOM event was still paying forcaretPositionFromPointand block rect reads—expensive while the browser firesdragcontinuously anddragleaveon every nested boundary.The handlers now bail out only when the editor is still in setup or the event target is outside the editor (
DOMEditor.hasTarget), matching the cheap checksgetEventPositionalready did up front. Edge case: if position resolution would have failed later (e.g. missing block DOM during teardown), the behavior events still fire instead of being dropped; the payload is unchanged (no position).Adds a Vitest browser test that asserts both behaviors run and
caretPositionFromPointis never invoked for these events. Patch changeset for@portabletext/editor.Reviewed by Cursor Bugbot for commit 3f623a2. Bugbot is set up for automated code reviews on this repo. Configure here.