fix: Drag & drop of blocks without inline content opens formatting toolbar (BLO-1116)#2628
Conversation
|
@miadnguyen is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughRemoved NodeSelection-based toolbar suppression and added a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DOM as ProseMirror Root (DOM)
participant Editor
participant ToolbarStore as FormattingToolbar Store
User->>DOM: pointer down on drag handle
DOM->>DOM: emit dragstart
DOM->>Editor: dragstart event -> set preventShowWhileDragging = true
Editor->>ToolbarStore: onChange/onSelectionChange calls suppressed (early return)
User->>DOM: drag move (drop targets update)
Note over Editor,ToolbarStore: Toolbar remains hidden during dragging
User->>DOM: drop (dragend)
DOM->>Editor: dragend event -> set preventShowWhileDragging = false
Editor->>ToolbarStore: trigger recalculation (toolbar may re-open)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/extensions/FormattingToolbar/FormattingToolbar.ts (1)
103-110:⚠️ Potential issue | 🟠 Major
pointercancelshould clearpreventShowWhileMouseDown, not set it.Line 107 contradicts the comment above it. In this file, the only reset path for
preventShowWhileMouseDownis thepointeruphandler, so setting it totruehere can leave the toolbar suppressed after a canceled drag/pointer sequence.Suggested change
dom.addEventListener( "pointercancel", () => { - preventShowWhileMouseDown = true; + preventShowWhileMouseDown = false; }, { signal, capture: true }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/extensions/FormattingToolbar/FormattingToolbar.ts` around lines 103 - 110, The pointercancel handler currently sets preventShowWhileMouseDown to true which contradicts its comment and leaves the toolbar suppressed; update the "pointercancel" event listener so it clears the flag (set preventShowWhileMouseDown to false) — mirror the reset behavior of the existing pointerup handler and ensure the handler on "pointercancel" (inside FormattingToolbar.ts near the preventShowWhileMouseDown logic) resets the state instead of setting it.
🧹 Nitpick comments (1)
tests/src/end-to-end/images/images.test.ts (1)
166-171: Please assert the drop result aftermouse.up().This currently proves only that the toolbar stays hidden mid-drag. It still passes if the handle path never reorders the document, which is the user-visible part of
#2603. Add a post-drop snapshot or DOM-order assertion.Possible follow-up
const toolbar = page.locator(".bn-formatting-toolbar"); await expect(toolbar).not.toBeVisible(); await page.mouse.up(); + await compareDocToSnapshot(page, "dragImageViaHandle"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/end-to-end/images/images.test.ts` around lines 166 - 171, After the drag is finished (after the existing await page.mouse.up()), add a post-drop assertion to verify the actual reorder happened rather than only that the toolbar is hidden: use the same test's DOM locators to either take a snapshot of the document and compare it (post-drop snapshot) or assert the DOM order of the moved elements (e.g., grab the image/handle nodes and assert their src/text/index positions have changed). Place this assertion immediately after await page.mouse.up() (near the existing toolbar locator) so the test fails if the handle never reorders the document.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/extensions/FormattingToolbar/FormattingToolbar.ts`:
- Around line 103-110: The pointercancel handler currently sets
preventShowWhileMouseDown to true which contradicts its comment and leaves the
toolbar suppressed; update the "pointercancel" event listener so it clears the
flag (set preventShowWhileMouseDown to false) — mirror the reset behavior of the
existing pointerup handler and ensure the handler on "pointercancel" (inside
FormattingToolbar.ts near the preventShowWhileMouseDown logic) resets the state
instead of setting it.
---
Nitpick comments:
In `@tests/src/end-to-end/images/images.test.ts`:
- Around line 166-171: After the drag is finished (after the existing await
page.mouse.up()), add a post-drop assertion to verify the actual reorder
happened rather than only that the toolbar is hidden: use the same test's DOM
locators to either take a snapshot of the document and compare it (post-drop
snapshot) or assert the DOM order of the moved elements (e.g., grab the
image/handle nodes and assert their src/text/index positions have changed).
Place this assertion immediately after await page.mouse.up() (near the existing
toolbar locator) so the test fails if the handle never reorders the document.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b850337-2b91-4ef2-bdbc-615cbe5cb0d4
📒 Files selected for processing (2)
packages/core/src/extensions/FormattingToolbar/FormattingToolbar.tstests/src/end-to-end/images/images.test.ts
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/mantine
@blocknote/react
@blocknote/server-util
@blocknote/shadcn
@blocknote/xl-ai
@blocknote/xl-docx-exporter
@blocknote/xl-email-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
commit: |
|
This is great, thanks! With your fix, I've also removed the check in |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/extensions/FormattingToolbar/FormattingToolbar.ts (1)
94-118:⚠️ Potential issue | 🔴 Critical
pointercancelhandler contradicts its own comment and permanently suppresses the toolbar after a drag.The comment on line 94 explicitly states the goal: "we don't want to be stuck in the
preventShowWhileMouseDownstate". However, the handler setspreventShowWhileMouseDown = trueon line 98—directly trapping it in that state. The flag can only be reset tofalsein thepointeruphandler (line 85).In a typical drag sequence:
pointerdown→preventShowWhileMouseDown = truedragstart→preventShowWhileDragging = true, toolbar hiddenpointercancel→ fires when browser takes over for native DnD; flag remainstruedragend→ only clearspreventShowWhileDragging;preventShowWhileMouseDownis not resetpointerup→ typically not dispatched after native HTML5 dragResult: after the first drag,
preventShowWhileMouseDownis stucktrue. Since bothonChange(line 58) andonSelectionChange(line 65) short-circuit when this flag is set, the formatting toolbar will stop appearing on text selections until a clean pointerdown→pointerup cycle occurs.Fix: change line 98 from
preventShowWhileMouseDown = true;topreventShowWhileMouseDown = false;, or defensively also reset it in thedragendhandler to cover the drag-takeover path on all browsers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/extensions/FormattingToolbar/FormattingToolbar.ts` around lines 94 - 118, The pointercancel handler currently sets preventShowWhileMouseDown = true which contradicts the comment and leaves the toolbar permanently suppressed after native drag; update the pointercancel handler to set preventShowWhileMouseDown = false (or at minimum also clear that flag in the dragend handler) so preventShowWhileMouseDown is reset when the browser takes over dragging; locate the pointercancel listener and the dragend listener in FormattingToolbar (look for preventShowWhileMouseDown, preventShowWhileDragging, and the event listeners on editor.prosemirrorView.root/dom) and change the assignment accordingly so selection/toolbar logic (onChange/onSelectionChange) can run after drag sequences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/extensions/FormattingToolbar/FormattingToolbar.ts`:
- Around line 94-118: The pointercancel handler currently sets
preventShowWhileMouseDown = true which contradicts the comment and leaves the
toolbar permanently suppressed after native drag; update the pointercancel
handler to set preventShowWhileMouseDown = false (or at minimum also clear that
flag in the dragend handler) so preventShowWhileMouseDown is reset when the
browser takes over dragging; locate the pointercancel listener and the dragend
listener in FormattingToolbar (look for preventShowWhileMouseDown,
preventShowWhileDragging, and the event listeners on
editor.prosemirrorView.root/dom) and change the assignment accordingly so
selection/toolbar logic (onChange/onSelectionChange) can run after drag
sequences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d9af037-1a14-49e1-b111-ca27e7c67691
📒 Files selected for processing (1)
packages/core/src/extensions/FormattingToolbar/FormattingToolbar.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/src/end-to-end/dragdrop/dragdrop.test.ts (1)
101-114: Assertion only covers post-drop state, not during-drag.The toolbar-suppression fix targets the window between
dragstartanddragend, but this test only checks.bn-formatting-toolbarvisibility afterdragAndDropBlockhas fully completed. If a regression ever re-introduces a transient toolbar flash mid-drag (the exact symptom from#2603), this assertion would still pass.Consider also asserting the toolbar is hidden mid-drag — e.g., split
dragAndDropBlockinto its mousedown/move/up steps and checkawait expect(toolbar).not.toBeVisible()after the drag has started but before the drop, or listen for thedragstartevent and sample visibility then. Non-blocking; the current assertion still guards the end-state user impact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/end-to-end/dragdrop/dragdrop.test.ts` around lines 101 - 114, The test only asserts toolbar visibility after dragAndDropBlock completes; update it to verify the toolbar is hidden during the drag start window as well by splitting the drag into explicit steps: use focusOnEditor/executeSlashCommand/insertHeading as before, locate dragTarget (IMAGE_SELECTOR) and dropTarget (H_ONE_BLOCK_SELECTOR), then initiate the drag manually (e.g., dispatch mousedown or call page.mouse.move/start by moving to dragTarget and issuing a mouse.down and a mouse.move toward dropTarget) and immediately assert await expect(page.locator(".bn-formatting-toolbar")).not.toBeVisible() before completing the action (mouse.up or drop); alternatively adjust dragAndDropBlock to provide a “pause before drop” hook and assert mid-drag there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/src/end-to-end/dragdrop/dragdrop.test.ts`:
- Around line 87-114: The two new tests ("Should be able to drag image" and
"Formatting toolbar should not appear when dragging image block") use
dragAndDropBlock and must be skipped on Firefox like the existing drag tests;
add the same Firefox guard (e.g., test.skip(browserName === "firefox",
"Playwright doesn't correctly simulate drag events in Firefox")) to each test
declaration that uses dragAndDropBlock (referencing IMAGE_SELECTOR,
H_ONE_BLOCK_SELECTOR and the dragAndDropBlock helper) so they are not executed
on the Firefox runner.
---
Nitpick comments:
In `@tests/src/end-to-end/dragdrop/dragdrop.test.ts`:
- Around line 101-114: The test only asserts toolbar visibility after
dragAndDropBlock completes; update it to verify the toolbar is hidden during the
drag start window as well by splitting the drag into explicit steps: use
focusOnEditor/executeSlashCommand/insertHeading as before, locate dragTarget
(IMAGE_SELECTOR) and dropTarget (H_ONE_BLOCK_SELECTOR), then initiate the drag
manually (e.g., dispatch mousedown or call page.mouse.move/start by moving to
dragTarget and issuing a mouse.down and a mouse.move toward dropTarget) and
immediately assert await
expect(page.locator(".bn-formatting-toolbar")).not.toBeVisible() before
completing the action (mouse.up or drop); alternatively adjust dragAndDropBlock
to provide a “pause before drop” hook and assert mid-drag there.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82bbc5d4-fc0d-49ad-a12c-242bd2955bce
📒 Files selected for processing (5)
tests/src/end-to-end/dragdrop/dragdrop.test.tstests/src/end-to-end/dragdrop/dragdrop.test.ts-snapshots/dragImage-chromium-linux.jsontests/src/end-to-end/dragdrop/dragdrop.test.ts-snapshots/dragImage-firefox-linux.jsontests/src/end-to-end/dragdrop/dragdrop.test.ts-snapshots/dragImage-webkit-linux.jsontests/src/end-to-end/images/images.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/src/end-to-end/images/images.test.ts
matthewlipski
changed the title
matthewlipski
changed the title
|
Thanks @miadnguyen, we really appreciate your contribution. Should be out sometime early next week |
3 participants
Summary
Fixes #2603. When dragging an image block using the side menu drag handle, the formatting toolbar incorrectly opens mid-drag.
Rationale
The formatting toolbar should only appear when the user clicks on the image or during text selection, not during a drag-and-drop operation. The unexpected toolbar appearance is disruptive.
Changes
preventShowWhileDraggingflag inFormattingToolbar.tsthat mirrors the existingpreventShowWhileMouseDownpatterndragstartanddragendlisteners oneditor.prosemirrorView.rootto set and clear this flag for the full duration of any drag operationimagestodragdropImpact
The toolbar is now suppressed during any block drag operation. Normal toolbar behavior is unaffected.
Testing
tests/src/end-to-end/images/images.test.ts: "Formatting toolbar should not appear when dragging image block"Screenshots/Video
image-drag-drop-toolbar.mov
Checklist
Additional Notes
When the drag handle is used, ProseMirror sets a
NodeSelectionon the image node before the drag begins. The existingpreventShowWhileMouseDownflag did not cover HTML drag events (dragstart/dragend), only pointer events, so the toolbar was not suppressed during drags initiated from the side menu.Summary by CodeRabbit
Bug Fixes
Tests