fix(super-editor): restore toolbar dropdown keyboard focus#3304
Conversation
sergiogomes
force-pushed
the
fix/toolbar-dropdown-keyboard-focus
branch
from
0bb5ffe to
518d0db
Compare
|
Rebased onto the latest |
…fter Escape After Escape restored focus to the inner ToolbarButton (.toolbar-item), @keydown.enter.stop on that element swallowed Enter before ButtonGroup's roving-tabindex handler could see it. Result: pressing Enter on the restored focus did nothing - the dropdown was unable to be reopened by keyboard until focus moved elsewhere. New prop `allowEnterPropagation` on ToolbarButton. When true (set by ButtonGroup's dropdown branch), the keydown handler does not stopPropagation, so Enter bubbles to .toolbar-item-ctn where activateToolbarItem reopens the dropdown. Plain buttons keep the default (false) so they do not double-fire their command emission. Split buttons (Bullet list / Numbered list main) are unaffected: handleSplitMainClick stops propagation internally, so Enter still runs the main command and does not toggle the dropdown. Tests added in ButtonGroup.test.js exercising the real ButtonGroup + ToolbarButton + ToolbarDropdown stack: - Enter on inner .toolbar-item opens the dropdown - Escape-then-Enter on restored focus reopens the dropdown - Space on inner .toolbar-item opens the dropdown - Split button: Enter runs the main command, dropdown stays closed
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@sergiogomes after Escape, focus lands on the inner button, but Enter on it was being stopped before the toolbar could see it, so the dropdown couldn't reopen by keyboard.
i pushed a follow-up commit (c68770b3c): a new allowEnterPropagation prop lets Enter bubble from the dropdown branch only, plus tests for Escape-then-Enter and the Bullet/Numbered list case.
two PR-description notes: Bullet/Numbered list main keeps running its main action on Enter (menu does not open), and preventDefault now only fires for keys you actually handle.
lgtm once CI is green.
| trigger.setAttribute('tabindex', '-1'); | ||
| trigger.focus(); |
There was a problem hiding this comment.
this branch never runs for any real dropdown (checked every one in the dev app). worth a one-line comment saying it's there to guard against trigger slots with no focusable child inside.
There was a problem hiding this comment.
@caio-pizzol thanks for the review, detailed feedback, and follow-up improvements 👊🏻
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🎉 This PR is included in @superdoc-dev/mcp v0.3.0-next.104 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.3.0-next.150 |
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.148 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.8.0-next.119 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.8.0-next.102 |
|
🎉 This PR is included in superdoc v1.30.0-next.98 The release is available on GitHub release |
3 participants
Summary
Test plan
pnpm exec vitest run packages/super-editor/src/editors/v1/components/toolbar/ButtonGroup.test.js packages/super-editor/src/editors/v1/components/toolbar/ToolbarDropdown.test.jspnpm run format:checkpnpm run lintNote: local commands printed existing warnings outside the toolbar files touched in this PR.