Skip to content

fix: action buttons of left column in toggle block hover (#2526) #2671

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yamiletpineda wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: action buttons of left column in toggle block hover (#2526) #2671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f9b2255
fix: action buttons of left column in toggle block hover (#2526)
Rn123456789 Apr 22, 2026
3aef93b
test: add draghandle test for column nested in toggle block
Rn123456789 Apr 22, 2026
efc1016
fix: include non-editable blocks in Ctrl+A selection and fix nested p…
Rn123456789 Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions packages/core/src/api/clipboard/toClipboard/copyExtension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Extension } from "@tiptap/core";
import { Fragment, Node } from "prosemirror-model";
import { Fragment, Node, Slice } from "prosemirror-model";
import { NodeSelection, Plugin } from "prosemirror-state";
import { CellSelection } from "prosemirror-tables";
import type { EditorView } from "prosemirror-view";
Expand Down Expand Up @@ -127,13 +127,18 @@ export function selectedFragmentToHTML<
);
}

let selectedFragment = view.state.selection.content().content;

if (selectedFragment.childCount === 1 &&
selectedFragment.firstChild?.type.name === "blockGroup") {
selectedFragment = selectedFragment.firstChild.content;
}

// Uses default ProseMirror clipboard serialization.
const clipboardHTML: string = view.serializeForClipboard(
view.state.selection.content(),
new Slice(selectedFragment, 0, 0)
).dom.innerHTML;

const selectedFragment = view.state.selection.content().content;

const externalHTML = fragmentToExternalHTML<BSchema, I, S>(
view,
selectedFragment,
Expand Down
34 changes: 30 additions & 4 deletions packages/core/src/extensions/SideMenu/SideMenu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,18 @@ function getBlockFromCoords(
continue;
}
if (adjustForColumns) {
const column = element.closest("[data-node-type=columnList]");
const column = element.closest("[data-node-type=column]");
if (column) {

const columnRect = column.getBoundingClientRect();

return getBlockFromCoords(
view,
{
// TODO can we do better than this?
left: coords.left + 50, // bit hacky, but if we're inside a column, offset x position to right to account for the width of sidemenu itself
left: Math.min(
Math.max(columnRect.left + 10, coords.left + 20),
columnRect.right - 10,
),
top: coords.top,
},
false,
Expand Down Expand Up @@ -101,6 +106,21 @@ function getBlockFromMousePos(
return undefined;
}

const column = referenceBlock.node.closest("[data-node-type=column]");

if (column) {
const columnRect = column.getBoundingClientRect();

return getBlockFromCoords(
view,
{
left: Math.min(columnRect.left + 20, columnRect.right - 10),
top: mousePos.y,
},
false,
);
}
Comment on lines +109 to +122
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 22, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Preserve nested-block resolution inside columns.

This early return skips the nested-block correction below Line 124. For indented/nested blocks inside a column, probing at columnRect.left + 20 can resolve the parent block instead of the nested block under the cursor.

Suggested fix 
   if (column) {
     const columnRect = column.getBoundingClientRect();
+    const referenceBlocksBoundingBox =
+      referenceBlock.node.getBoundingClientRect();
 
     return getBlockFromCoords(
       view,
       {
-        left: Math.min(columnRect.left + 20, columnRect.right - 10),
+        left: Math.min(
+          Math.max(referenceBlocksBoundingBox.right - 10, columnRect.left + 10),
+          columnRect.right - 10,
+        ),
         top: mousePos.y,
       },
       false,
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/extensions/SideMenu/SideMenu.ts` around lines 109 - 122,
The current early return when a column is found uses getBlockFromCoords with a
fixed left offset and skips the nested-block correction logic, causing parent
blocks to be selected instead of indented children; replace the early return
with assigning the result of getBlockFromCoords to a local variable (e.g.,
candidate = getBlockFromCoords(view, {...})) and then fall through to the
existing nested-block correction logic that appears after this block so that
indented/nested blocks inside a column are still resolved (use candidate only if
the nested/block-correction step doesn't find a better match). Ensure you
reference the same symbols (referenceBlock, view, columnRect,
getBlockFromCoords) and preserve the original coordinate calculation but do not
return before running the nested resolution.


/**
* Because blocks may be nested, we need to check the right edge of the parent block:
* ```
Expand Down Expand Up @@ -250,7 +270,7 @@ export class SideMenuView<
// padding. This is a little weird since this child element will
// be the first block, but since it's always non-nested and we
// only take the x coordinate, it's ok.
column.firstElementChild!.getBoundingClientRect().x
column.getBoundingClientRect().x
: (
this.pmView.dom.firstChild as HTMLElement
).getBoundingClientRect().x,
Expand Down Expand Up @@ -603,6 +623,12 @@ export class SideMenuView<

this.mousePos = { x: event.clientX, y: event.clientY };

const target = event.target as HTMLElement | null;

if (target?.closest(".bn-side-menu")) {
return;
}
Comment on lines +626 to +630
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 22, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

find . -name "SideMenu.ts" -type f | head -20

Repository: TypeCellOS/BlockNote

Length of output: 115

🏁 Script executed:

cat -n ./packages/core/src/extensions/SideMenu/SideMenu.ts | sed -n '610,640p'

Repository: TypeCellOS/BlockNote

Length of output: 1208

🌐 Web query:

DOM event.target type MouseEvent .closest method Element

💡 Result:

In the DOM, for a MouseEvent (e.g., from a 'click' handler), event.target is an EventTarget referencing the innermost element that dispatched the event, typically an Element like an input or button. Since Element.closest(selectors) is a standard method that traverses up the DOM tree to find the nearest ancestor (or self) matching the CSS selectors, you can safely call event.target.closest(selectors) after ensuring it's an Element. This pattern is commonly used for event delegation: attach a single listener to a parent, then use event.target.closest to identify the relevant child element. Example: document.addEventListener('click', function(event) { const button = (event.target as Element).closest('button.special'); if (button) { // Handle click on special button or its children } }); TypeScript requires casting event.target to Element or HTMLElement since EventTarget lacks closest. MouseEvent generics specify the expected target type (e.g., MouseEvent). No changes or deprecations as of 2026 per MDN docs.

Citations:

🌐 Web query:

Text node Comment node .closest() method throws TypeError

💡 Result:

The .closest method throws a TypeError when called on a Text node or Comment node because it is defined only on the Element interface, not on Node. Text nodes (nodeType 3) and Comment nodes (nodeType 8) are not Elements, so they do not have the closest method. To find the closest ancestor element matching a selector from a Text or Comment node, first access its parent Element using node.parentElement (or node.parentNode if you need to check the type), then call closest on that Element. Example: const textNode = someElement.firstChild; // Text node if (textNode.nodeType === Node.TEXT_NODE) { const closestDiv = textNode.parentElement.closest('div'); } This is a current DOM specification behavior as of 2026; an open WHATWG issue proposes extending closest to more node types like Text, but it remains Element-only.

Citations:

Narrow event.target before calling .closest.

The cast to HTMLElement | null does not guarantee the runtime target is an Element. Since event.target has type EventTarget, it could be a non-Element node (e.g., Text or Comment), and calling .closest() on such nodes throws a TypeError. Use an instanceof Element guard to safely narrow the type.

Suggested fix 
-    const target = event.target as HTMLElement | null;
+    const target = event.target;
 
-    if (target?.closest(".bn-side-menu")) {
-        return;
+    if (target instanceof Element && target.closest(".bn-side-menu")) {
+      return;
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const target = event.target as HTMLElement | null;
if (target?.closest(".bn-side-menu")) {
return;
}
const target = event.target;
if (target instanceof Element && target.closest(".bn-side-menu")) {
return;
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/extensions/SideMenu/SideMenu.ts` around lines 626 - 630,
The code in SideMenu.ts currently casts event.target to HTMLElement | null and
calls target.closest(".bn-side-menu"), which can throw if event.target is a
non-Element (e.g., Text node); modify the logic in the event handler where
`const target = event.target as HTMLElement | null` and the subsequent
`target?.closest(...)` check to first guard with `if (!(event.target instanceof
Element)) return;` or similar, then assign a properly typed `const target:
Element = event.target` and call `target.closest(".bn-side-menu")`—this ensures
`closest` is only invoked on an Element and prevents runtime TypeErrors.


// We want the full area of the editor to check if the cursor is hovering
// above it though.
const editorOuterBoundingBox = this.pmView.dom.getBoundingClientRect();
Expand Down
13 changes: 12 additions & 1 deletion ...ges/core/src/extensions/tiptap-extensions/KeyboardShortcuts/KeyboardShortcutsExtension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Extension } from "@tiptap/core";
import { Fragment, Node } from "prosemirror-model";
import { TextSelection } from "prosemirror-state";
import { AllSelection, TextSelection } from "prosemirror-state";

import {
getBottomNestedBlockInfo,
Expand Down Expand Up @@ -953,6 +953,17 @@ export const KeyboardShortcutsExtension = Extension.create<{
"Mod-z": () => this.options.editor.undo(),
"Mod-y": () => this.options.editor.redo(),
"Shift-Mod-z": () => this.options.editor.redo(),
// Forces AllSelection from pos 0 to include non-editable blocks (e.g. images) that
// TextSelection would skip.
"Mod-a": () => {
const { doc } = this.options.editor.prosemirrorState;
this.options.editor.prosemirrorView?.dispatch(
this.options.editor.prosemirrorState.tr.setSelection(
new AllSelection(doc)
)
);
return true;
},
Comment on lines +956 to +966
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Mod-a returns true even when dispatch is skipped.

prosemirrorView is accessed via optional chaining, so if it's undefined the selection is never updated but the handler still returns true, swallowing the shortcut and preventing the browser's native Select-All fallback. Also, dispatching directly bypasses the TipTap command pipeline used elsewhere in this file — prefer this.editor.commands.command(({ tr, dispatch }) => { ... }) or this.editor.view.dispatch for consistency with the rest of the keymap (which relies on this.editor, not this.options.editor.prosemirrorView).

♻️ Suggested change 
-      "Mod-a": () => {
-        const { doc } = this.options.editor.prosemirrorState;
-        this.options.editor.prosemirrorView?.dispatch(
-          this.options.editor.prosemirrorState.tr.setSelection(
-            new AllSelection(doc)
-          )
-        );
-        return true;
-      },
+      "Mod-a": () => {
+        const view = this.options.editor.prosemirrorView;
+        if (!view) {
+          return false;
+        }
+        view.dispatch(
+          view.state.tr.setSelection(new AllSelection(view.state.doc)),
+        );
+        return true;
+      },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Forces AllSelection from pos 0 to include non-editable blocks (e.g. images) that
// TextSelection would skip.
"Mod-a": () => {
const { doc } = this.options.editor.prosemirrorState;
this.options.editor.prosemirrorView?.dispatch(
this.options.editor.prosemirrorState.tr.setSelection(
new AllSelection(doc)
)
);
return true;
},
// Forces AllSelection from pos 0 to include non-editable blocks (e.g. images) that
// TextSelection would skip.
"Mod-a": () => {
const view = this.options.editor.prosemirrorView;
if (!view) {
return false;
}
view.dispatch(
view.state.tr.setSelection(new AllSelection(view.state.doc)),
);
return true;
},
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In
`@packages/core/src/extensions/tiptap-extensions/KeyboardShortcuts/KeyboardShortcutsExtension.ts`
around lines 956 - 966, The "Mod-a" key handler currently uses optional chaining
on this.options.editor.prosemirrorView so it may return true even when no
dispatch occurs; replace the direct dispatch with the TipTap command pattern to
ensure consistent pipeline and only swallow the event when dispatch happened:
inside the "Mod-a" handler call this.editor.commands.command(({ tr, dispatch })
=> { const { doc } = this.options.editor.prosemirrorState; if (dispatch)
dispatch(tr.setSelection(new AllSelection(doc))); return !!dispatch; }); or
alternatively use this.editor.view.dispatch if you must dispatch directly, but
ensure you check view exists and return false when no dispatch occurred. Ensure
references include the existing AllSelection,
this.options.editor.prosemirrorState, and the handler name "Mod-a".

};
},
});
14 changes: 14 additions & 0 deletions tests/src/end-to-end/draghandle/draghandle.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,4 +181,18 @@ test.describe("Check Draghandle functionality", () => {

await compareDocToSnapshot(page, "draghandlenesteddelete");
});

test("Hovering over column nested in toggle block should show draghandle", async () => {
await executeSlashCommand(page, "togglelist");
await page.keyboard.type("Toggle heading");
await page.keyboard.press("Enter");
await executeSlashCommand(page, "two");
await page.keyboard.type("Left column content");
const leftColumn = await page.locator(".bn-block-column").first();
await moveMouseOverElement(page, leftColumn);
await page.waitForSelector(DRAG_HANDLE_SELECTOR);
await page.click(DRAG_HANDLE_SELECTOR);
await page.waitForSelector(DRAG_HANDLE_MENU_SELECTOR);
});
Comment on lines +185 to +196
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 22, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Make the regression test prove the column is inside the toggle.

Right now the test selects the first column globally, so it can still pass if Enter creates the columns as a sibling of the toggle instead of a child. Scope the locator to a toggle ancestor, or assert that relationship before hovering.

Suggested test hardening 
-    const leftColumn = await page.locator(".bn-block-column").first();
+    const leftColumn = page
+      .locator('[data-node-type*="toggle" i] [data-node-type="column"]')
+      .first();
+    await expect(leftColumn).toBeVisible();
     await moveMouseOverElement(page, leftColumn);
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/src/end-to-end/draghandle/draghandle.test.ts` around lines 185 - 196,
The test currently grabs the first global ".bn-block-column" which can pass even
if the columns are siblings of the toggle; change the locator/assertion so the
column is proved to be inside the toggle before hovering. Specifically, in the
"Hovering over column nested in toggle block should show draghandle" test (the
test function name) either scope leftColumn to a toggle ancestor (e.g., find the
toggle block created by executeSlashCommand("togglelist") and then use its
locator().locator(".bn-block-column").first()) or add an explicit assertion that
the selected leftColumn is a descendant of the toggle (using a toggle locator
and an .contains/.getBy* or assertion that
toggle.locator(".bn-block-column").count() > 0) before calling
moveMouseOverElement, then proceed to waitForSelector(DRAG_HANDLE_SELECTOR) and
click as before.


});