Skip to content

fix(ui): commit popover forms through submit, not a key handler - #3030

Draft
YousefED wants to merge 8 commits into
mobile/test-infrafrom
mobile/link-popover
Draft

fix(ui): commit popover forms through submit, not a key handler#3030
YousefED wants to merge 8 commits into
mobile/test-infrafrom
mobile/link-popover

Conversation

@YousefED

@YousefED YousefED commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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, with enterkeyhint ruled out as the cause) — and Form.Root was a <div>, so its onSubmit could never fire.

The fix

Form.Root renders a real <form>; submission runs off its submit event. Three consequences, each pinned by tests:

  • HTML only submits implicitly with a submit button or exactly one field — the two-field link edit form would still reach nothing. Form.Root renders a submit button: visually hidden (clipped, not display:none) so assistive technology keeps a labelled control, tabIndex={-1} so sighted keyboard users never land on an invisible tab stop. omitSubmitButton opts out for callers with their own control (the embed tab).
  • No composition guard is needed on this path — the IME consumes the confirming Enter (it reaches the page as keyCode 229, which triggers no default action), so implicit submission cannot fire mid-composition; the isComposing checks 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.
  • One submission path makes the five Enter key handlers redundant; removed. EmbedTab had no form at all and gains one; the AI prompt menu's handler and onSubmit disagreed on whether Enter picks the highlighted suggestion or submits the raw text, and now share one decision.

Also: TextInput loses its onSubmit prop — every skin forwarded it onto the <input>, where submit never fires, so it was dead since #652 (and plausibly why the gap went unnoticed). getSelectedLinkUrl scans the selection for the link mark instead of probing a boundary position.

Notes for review

  • New dictionary key generic.form_submit (the submit button's accessible name): consumers with hand-rolled dictionaries get a compile error until they add it — release-notes worthy. The 23 non-English translations are machine-generated and unreviewed.
  • Device recertification pending: the earlier BrowserStack pass predates this rework; the suite should be re-run before merge.
  • Platform facts asserted rather than assumed in end-to-end/form/: implicit-submission rules and composition behaviour, per engine. All fixes proven red-first (details in commit messages).

@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@YousefED YousefED mentioned this pull request Aug 31, 2026
Draft
@vercel

vercel Bot commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
blocknote Error Error Sep 1, 2026 12:58am UTC
blocknote-website Error Error Sep 1, 2026 12:58am UTC

Request Review

@pkg-pr-new

pkg-pr-new Bot commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown

Open in StackBlitz

@blocknote/ariakit

npm i https://pkg.pr.new/@blocknote/ariakit@3030

@blocknote/code-block

npm i https://pkg.pr.new/@blocknote/code-block@3030

@blocknote/core

npm i https://pkg.pr.new/@blocknote/core@3030

@blocknote/diagram-block

npm i https://pkg.pr.new/@blocknote/diagram-block@3030

@blocknote/mantine

npm i https://pkg.pr.new/@blocknote/mantine@3030

@blocknote/math-block

npm i https://pkg.pr.new/@blocknote/math-block@3030

@blocknote/react

npm i https://pkg.pr.new/@blocknote/react@3030

@blocknote/server-util

npm i https://pkg.pr.new/@blocknote/server-util@3030

@blocknote/shadcn

npm i https://pkg.pr.new/@blocknote/shadcn@3030

@blocknote/xl-ai

npm i https://pkg.pr.new/@blocknote/xl-ai@3030

@blocknote/xl-docx-exporter

npm i https://pkg.pr.new/@blocknote/xl-docx-exporter@3030

@blocknote/xl-email-exporter

npm i https://pkg.pr.new/@blocknote/xl-email-exporter@3030

@blocknote/xl-multi-column

npm i https://pkg.pr.new/@blocknote/xl-multi-column@3030

@blocknote/xl-odt-exporter

npm i https://pkg.pr.new/@blocknote/xl-odt-exporter@3030

@blocknote/xl-pdf-exporter

npm i https://pkg.pr.new/@blocknote/xl-pdf-exporter@3030

commit: de4a058

@github-actions

github-actions Bot commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://TypeCellOS.github.io/BlockNote/pr-preview/pr-3030/

Built to branch gh-pages at 2026-09-01 00:58 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@YousefED
YousefED force-pushed the mobile/link-popover branch from 56c82a9 to 2b6ff77 Compare August 31, 2026 17:36
@YousefED
YousefED force-pushed the mobile/link-popover branch from 2b6ff77 to d5df900 Compare August 31, 2026 17:42
@YousefED
YousefED force-pushed the mobile/link-popover branch from d5df900 to 9909733 Compare August 31, 2026 17:49
@YousefED
YousefED force-pushed the mobile/link-popover branch from 9909733 to 72fae20 Compare August 31, 2026 17:51
@YousefED
YousefED force-pushed the mobile/link-popover branch from 72fae20 to dff52d6 Compare August 31, 2026 17:59
@YousefED
YousefED force-pushed the mobile/link-popover branch from dff52d6 to 45b00f6 Compare August 31, 2026 18:20
@YousefED
YousefED force-pushed the mobile/link-popover branch from 45b00f6 to d10475e Compare August 31, 2026 19:26
@YousefED
YousefED force-pushed the mobile/link-popover branch from d10475e to 93e6389 Compare August 31, 2026 19:36
@YousefED
YousefED force-pushed the mobile/link-popover branch 2 times, most recently from 4176f2e to 6aa2abb Compare August 31, 2026 19:44
@YousefED
YousefED force-pushed the mobile/link-popover branch from 6aa2abb to 01fecda Compare August 31, 2026 19:51
@YousefED YousefED closed this Aug 31, 2026
@YousefED YousefED reopened this Aug 31, 2026
@YousefED
fix(ui): commit popover forms through submit, not a key handler
6698f6e 
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.
@YousefED
fix(ui): one submit control per form, and reuse mergeRefs
a90622b 
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.
@YousefED
test(ui): cover the composition guard, and drop two tests that couldn…
0892097 
…'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.
@YousefED
fix(core): scan the selection for the link URL instead of probing a b…
828cef5 
…oundary

The `from + 1` probe fixed the left-edge case (`marks()` excludes a link
at its left boundary) but is still fragile: browsers disagree by a
position on where a selection over a link starts, so a single-position
lookup can land outside the mark either way. For a non-empty selection,
scan the selected range for the first link mark instead; an empty
selection keeps the plain position lookup.
@YousefED
fix(ui): drop the composition guard — native submission already handl…
797e3de 
…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.
@YousefED
test(device): link helpers next to the link tests, submitting via the…
127aef0 
… form

Review feedback (two threads): the link-flow device helpers belong next
to the tests that use them, not in the shared lib — moved here from
editorPage/gestures.

typeAndSubmit also changes how it submits, answering why it dispatched a
synthetic Enter: the on-screen keyboard's action key is unreachable by
any automation channel (see README), and the dispatched keydown only
worked while the popovers had key handlers. With submission running off
the form's submit event, an untrusted keydown does nothing — the helper
was silently broken by the form rework. requestSubmit() is the browser's
own submission path and exercises the popover's real onSubmit wiring;
the IME's own action-key choice stays a manual release check.
@YousefED
test(device): submit the link popover by pressing the real Enter key
f5541a3 
Two review questions shaped this. First: the old dispatched KeyboardEvent
could never submit once the popovers moved to the form's submit event —
synthetic events trigger no default action. Second: 'why not hit the
Enter key?' — no reason not to, and the rig already knew how: on iOS,
pressSoftKeyboardEnter taps the on-screen keyboard's actual return key
(the RETURN_KEY_RATIOS offset ladder) — the real user gesture. On
Android, where BrowserStack blocks native taps, a W3C protocol Enter is
used instead: trusted input, so the browser runs its default action and
the real path is exercised (key press -> implicit form submission -> the
popover's submit handling). Only Gboard's own choice of *which* action
its key performs stays out of reach, on the manual release checklist.
Callers supply the verify script the iOS tap ladder needs.
@YousefED
YousefED force-pushed the mobile/link-popover branch from 01fecda to f5541a3 Compare August 31, 2026 20:07
@YousefED
docs(device): the IME action key is reachable on a local emulator
de4a058 
The 'no input channel can press that key' claim was BrowserStack-scoped
truth stated as absolute. A local Android emulator runs real Chrome and
real Gboard, and adb can tap the on-screen action key — verified end to
end (action key tapped, link created in the correct editor, focus
retained). Recorded as the known path to automating the release-checklist
item.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@YousefED