fix(ui): commit popover forms through submit, not a key handler - #3030
Open
YousefED wants to merge 9 commits into
Open
fix(ui): commit popover forms through submit, not a key handler#3030YousefED wants to merge 9 commits into
YousefED wants to merge 9 commits into
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/diagram-block
@blocknote/mantine
@blocknote/math-block
@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: |
|
YousefED
force-pushed
the
mobile/link-popover
branch
from
August 31, 2026 17:36
56c82a9 to
2b6ff77
Compare
YousefED
force-pushed
the
mobile/link-popover
branch
from
August 31, 2026 17:42
2b6ff77 to
d5df900
Compare
YousefED
force-pushed
the
mobile/link-popover
branch
from
August 31, 2026 17:49
d5df900 to
9909733
Compare
YousefED
force-pushed
the
mobile/link-popover
branch
from
August 31, 2026 17:51
9909733 to
72fae20
Compare
YousefED
force-pushed
the
mobile/link-popover
branch
from
August 31, 2026 17:59
72fae20 to
dff52d6
Compare
YousefED
force-pushed
the
mobile/link-popover
branch
from
August 31, 2026 18:20
dff52d6 to
45b00f6
Compare
YousefED
force-pushed
the
mobile/link-popover
branch
from
September 1, 2026 07:01
3044587 to
103cb26
Compare
YousefED
force-pushed
the
mobile/link-popover
branch
from
September 1, 2026 08:10
103cb26 to
f66cfb4
Compare
YousefED
force-pushed
the
mobile/link-popover
branch
from
September 1, 2026 08:48
f66cfb4 to
89a2620
Compare
YousefED
force-pushed
the
mobile/link-popover
branch
from
September 1, 2026 10:09
89a2620 to
df29e8d
Compare
YousefED
commented
Sep 1, 2026
| // popovers into its own container — so it doubles as "this popover belongs | ||
| // to the mobile toolbar", which is what the two behaviours below actually | ||
| // depend on. Named here so the reason isn't hidden behind an unrelated prop. | ||
| const isMobileToolbarPopover = !!portalRoot; |
Collaborator
Author
There was a problem hiding this comment.
add a TODO here indicating this should be cleaned up once we've settled on a good portalling solution (that's still pending slack discussions)
Collaborator
Author
There was a problem hiding this comment.
TODO added (worktree).
YousefED
force-pushed
the
mobile/link-popover
branch
from
September 1, 2026 11:36
df29e8d to
370d31f
Compare
nperez0111
reviewed
Sep 1, 2026
Creating a link on Android didn't work: the popover's URL never became a link and focus jumped to the next editor instead. The cause is that a mobile IME picks the action its Enter key performs, and with a lone text field it picks "Next" — advancing focus and dispatching no key event at all. A popover that only listens for Enter therefore has nothing to hear. Putting the fields in a real `<form>` is what makes the IME offer a submitting action instead, confirmed on a device; `Form.Root` was a `<div>`, so `onSubmit` could never fire. `Form.Root` now renders a `<form>`, and submission runs off its `submit` event. That has three consequences worth calling out: - HTML only submits implicitly when a form has a submit button or exactly one field, so the link *edit* form — url plus title — would still reach nothing. `Form.Root` renders a submit button to cover any field count. It is visually hidden rather than absent so assistive technology still has a labelled control, and outside the tab order so sighted keyboard users never land on a control they can't see. - The browser performs implicit submission for an Enter that arrives with `isComposing: true`, so accepting an IME candidate would submit the popover mid-word. `useFormSubmit` guards that centrally, replacing the per-callsite `isComposing` checks that had already drifted apart. - With one submission path, the five Enter handlers are redundant and are removed. `EmbedTab` had no form at all and gains one; the AI prompt menu's handler and `onSubmit` disagreed about whether Enter picks the highlighted suggestion or submits the typed text, and now share one decision. `TextInput` also loses its `onSubmit` prop: every skin forwarded it to the `<input>`, and `submit` only fires on a form and bubbles upward, so it could never have fired. `EditLinkMenuItems` passed it, which is plausibly why the gap went unnoticed.
Review follow-ups: - The embed panel ended up with two submit controls: its own Embed button plus the hidden one `Form.Root` adds, so a screen reader announced two separate actions for the one thing that panel does. `Form.Root` now takes `hasOwnSubmitButton` for callers that supply their own. - The three `TextInput`s hand-rolled ref merging. `mergeRefs` already exists here, but returns a fresh callback per call — which detaches and reattaches the ref every render — so this adds `useMergeRefs` alongside it, memoized the way `react-merge-refs` does, and uses that. - The mantine popover keyed two behaviours off `portalRoot` while its comments explained them in terms of mobile. Same condition, but named, so the reason isn't hidden behind an unrelated prop. - `useFormSubmit` documents that it exists for `Form.Root` implementations rather than applications.
…'t fail Second review round, checking whether the tests added in the first one can actually fail. Two could not: - The composition tests built a synthetic form replicating what `Form.Root` does, so deleting the guard from `useFormSubmit` left them all green — the shipped code had no coverage at all. A test now drives the real link popover through a CDP composition, and fails when the guard is removed. The synthetic ones stay as what they are: the platform fact that a browser submits for an Enter carrying `isComposing: true`. - "the embed tab commits exactly once" asserted one image was present, which is true whether the update ran once or twice. Its replacement counted the form's submit events, but that cannot fail either: only mantine runs in this suite and its panel button already defaults to `type="button"`. The structural check — no button inside the form — is what actually guards both the double-commit and the duplicate-control problems, and it does fail when the button is moved inside, so that one is kept and the outcome-based tests are dropped rather than left as decoration. Also renames `hasOwnSubmitButton` to `omitSubmitButton`: EmbedTab's button sits outside the form, so the form has no submit button at all and relies on single-field implicit submission. The old name asserted something untrue of its only caller, and hid the constraint the flag carries.
…es IMEs The guard answered the wrong category of problem. `isComposing` checks are needed in *keydown* handlers, because an IME-consumed key still dispatches to JS — that is what the five removed Enter handlers were. Native form submission never sees that key: the IME consumes the confirming Enter (it reaches the page as keyCode 229, which the browser runs no default action for), so implicit submission cannot fire mid-composition. This is why no plain form on the web carries composition handling. The state the guard defended — composition open, unconsumed trusted Enter delivered — is one only CDP emulation can fabricate: `imeSetComposition` sets composition state with no IME in the loop to consume the key. No real IME produces the sequence. Worse, the guard carried real risk in the other direction: Gboard's action key commits the composition and submits in one press, so if any IME delivers `submit` before `compositionend`, the guard would swallow a legitimate submission — the original bug, reintroduced for exactly the users it claimed to protect. `Form.Root` goes back to plain `preventDefault` wiring, `useFormSubmit` is deleted, and the composition tests now pin the *native* contract against the real popover: accepting a candidate does not submit, Enter afterwards does.
Form.Root's optional omitSubmitButton becomes a required
submitButton: ReactElement | "none" — the compiler now forces every
caller to decide the form's one submit affordance instead of a boolean
opt-out defaulting silently (ReactElement, not ReactNode, so the
"none" sentinel can't be satisfied by an arbitrary string).
@blocknote/react owns the default control (ScreenReaderOnlySubmit,
which reads the dictionary) and its clip CSS, in one copy; the three
skin Forms collapse to rendering whatever they're given.
The embed tab shows why: its visible button is now the form's
submitButton — inside the <form>, one commit path for click, Enter and
a mobile IME's action key, one labelled action for assistive
technology. That required FilePanel.Button to take an explicit
type ("button" | "submit"): the skins disagreed on the default
(shadcn hardcoded submit, Mantine defaults to button), which is exactly
what had forced the button outside the form before.
Layout-wise the <form> is a semantic wrapper only, never a box: every
skin renders it as class bn-form and one shared rule gives it
display: contents. Ariakit and shadcn rendered no wrapper element at
all before the real <form> arrived, so their flex+gap containers
(popover contents, the file panel's tab column) lay out fields as
direct children; mantine's containers are block, making contents a
no-op there — verified per skin against computed layout.
Breaking (release notes): Form.Root requires submitButton;
FilePanel.Button requires type. The pointer:coarse input sizing in the
same stylesheet region is from #2982.
implicitSubmit and compositionSubmit assert browser behavior against raw createElement fixtures — nothing BlockNote in them — so they move out of form/ into end-to-end/platform/ with a README stating the contract: these are the per-engine platform facts Form.Root's design rests on, and a red test here after a browser update points at the platform fact that moved, not at BlockNote.
The portalRoot-implies-mobile inference in Popover gets a TODO pending the portalling discussion. TextInput documents why its manual preventScroll focus and Mantine's traps can never fight: no trap runs in the form popovers at all (Popover's trapFocus defaults to false), and in trap-active subtrees nearby (toolbar Tab-cycling, desktop menus) the data-autofocus attribute makes a trap pick this same element.
matthewlipski
left a comment
matthewlipski
left a comment
Collaborator
There was a problem hiding this comment.
Tested on Android and iOS - works very nicely! Though the PR description focuses on the enter handling/form submission fixes, the whole issue of the virtual keyboard handling on focus moving to the input is also fixed. What change in the code is actually responsible for this? Since I don't really see that mentioned in the description.
From review: the manual focus block was copy-pasted across the three
skins and had already drifted (mantine's copy had grown a comment and a
data-autofocus attribute the others lacked). The hook in @blocknote/react
now owns the ref, the focus({ preventScroll: true }) effect, and the
rationale — including how the official implementations compare (React's
autoFocus is a bare .focus() at commit, which is exactly the scroll-yank
this exists to avoid; Mantine defers via setTimeout; floating-ui via
microtask + rAF) and why no extra deferral is used here: nothing to wait
for, and added hops erode the user-gesture window in which iOS Safari
lets a programmatic focus open the keyboard (validated on real iOS).
data-autofocus is set per skin only where the UI library reads it:
Mantine's focus trap and Ariakit's dialog initial-focus both select it
(Ariakit's popovers run autoFocusOnShow by default, so the attribute
makes their pick explicit instead of positional); shadcn's Base UI has
no attribute convention — its mechanism is the initialFocus prop — so
that skin omits it.
- EmbedTab's submit callback renamed to handleSubmit (consistency with EditLinkMenuItems). - The Form contract docs in ComponentsContext trimmed to the contract; the device-verified rationale lives in the PR and its tests. - One focus principle across skins, now enforced rather than raced: BlockNote owns focus in its popovers (useAutoFocus). Ariakit's autoFocusOnShow is disabled — its default bare-focuses the first tabbable (no preventScroll, plus a Safari scrollIntoView), the exact scroll-yank useAutoFocus avoids, previously masked only by effect ordering. Mantine's popover trapFocus pinned to its (identical) default, dead ternary dropped. data-autofocus stays only where the library reads it AND focuses safely — Mantine's trap; Ariakit fails the second test, Base UI the first.
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
3 participants
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.
Third layer of the stack, on #3029.
The bug
Creating a link on Android didn't work: the URL never became a link and focus jumped to the next editor. A mobile IME picks what its Enter key does — with a lone text field outside a form it picks "Next": advance focus, no key event at all. A popover listening for Enter has nothing to hear. Being inside a real
<form>is what makes the IME offer a submitting action instead (verified on a physical device, withenterkeyhintruled out as the cause) — andForm.Rootwas a<div>, so itsonSubmitcould never fire.The fix
Form.Rootrenders a real<form>; submission runs off itssubmitevent. Three consequences, each pinned by tests:Form.RootrequiressubmitButton: ReactElement | "none", withScreenReaderOnlySubmitin@blocknote/reactas the standard control (visually hidden — clipped, notdisplay:none— so assistive technology keeps a labelled control;tabIndex={-1}so sighted keyboard users never land on an invisible tab stop; CSS and dictionary in one copy instead of per skin). The embed tab passes its real button as thesubmitButton— inside the form, one commit path for click/Enter/IME-action, one labelled action — which also forcedFilePanel.Buttonto take an explicittype(the skins disagreed on the default, which is what had exiled the button from the form in the first place).isComposingchecks the old keydown handlers carried don't transfer to the submit path. The tests pin the native contract against the real popover: accepting a candidate does not submit, Enter afterwards does.EmbedTabhad no form at all and gains one; the AI prompt menu's handler andonSubmitdisagreed on whether Enter picks the highlighted suggestion or submits the raw text, and now share one decision.Also:
TextInputloses itsonSubmitprop — every skin forwarded it onto the<input>, wheresubmitnever fires, so it was dead since #652 (and plausibly why the gap went unnoticed).Notes for review
Form.RootrequiressubmitButton(replaces the earlier optional opt-out),FilePanel.Buttonrequirestype, and the new dictionary keygeneric.form_submit(the submit control's accessible name) makes hand-rolled dictionaries a compile error until added. The 23 non-English translations are machine-generated and unreviewed.mobile/linkSubmit.test.tsx) — exactly what the IME's submitting action does — but which action the IME itself offers is an OS decision no emulation can observe. That half is on the manual release checklist (testing skill);linkSubmit.test.tsx's header documents the boundary. A device suite automating it exists parked onmobile/emulator-layer(test(device): local emulator layer — real Chrome/Gboard as normal CI #3034).end-to-end/platform/directory (rawcreateElementfixtures, nothing BlockNote — the README states the contract): implicit-submission rules and composition behaviour, per engine, on the desktop engines and the emulated-android instance alike. All fixes proven red-first (details in commit messages).