fix(editor): spelling suggestions in the context menu; one menu per secondary click - #1864
Merged
Conversation
Chromium's spellchecker was already flagging misspellings, and Electron already handed `misspelledWord` and `dictionarySuggestions` to the `context-menu` handler. `buildEditableTextContextMenu` never declared either field, so the menu it built went straight to undo/cut/copy/paste and the red squiggle led nowhere. Prepend the suggestions, each applying via `replaceMisspelling`, then "Add to Dictionary" backed by `session.addWordToSpellCheckerDictionary`. An empty suggestion list shows a disabled "No Suggestions" item so the feature is visibly present rather than silently absent. The builder now takes the `WebContents` it acts on; both APIs hang off it. Refs #1850
A secondary click popped the native context menu from the main process while BlockNote independently opened its floating formatting toolbar off the same pointer sequence. Neither knew about the other, so the toolbar appeared without focus and stayed inert until it was clicked twice. BlockNote opens the toolbar from a capture-phase `pointerup`, and whether `contextmenu` lands before or after that is platform-dependent. Racing that listener would need a platform branch, so gate on state instead: a `contextmenu` inside the editor hides the floating toolbar until the next ordinary interaction (a non-secondary `pointerdown`, or any key). The sticky toolbar and the double-click selection path are untouched. Refs #1850
Documents what the two fixes change for readers: how to correct a flagged word, that the dictionary addition persists, which language the spellchecker uses on each platform, and that the context menu now stands the floating toolbar down instead of opening beside it. Refs #1850
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
Verifying the previous commit in the real app showed the suppression firing and then undoing itself. `FormattingToolbarController` tears the toolbar down and mounts a fresh one during the same interaction, twice within 14ms, so the `useState` that recorded the context menu died with the old instance and the replacement rendered as if nothing had happened. Move the gate up to `ReviewFormattingToolbarController`, which ContentArea mounts and BlockNote does not touch. Same listeners, same release conditions; only the owner changes. The unit tests missed this because they rendered the toolbar directly and never saw a remount. The mocked controller is now keyed so a test can force that remount, and one covers it. Refs #1850
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
8 tasks
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
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.
Closes #1850
Two unrelated defects behind one report. Aurelie guessed they might be the same bug; they are not.
Bug 1 — spelling suggestions were never wired up
Chromium's spellchecker was doing its job. The red squiggle simply led nowhere.
Electron hands
misspelledWordanddictionarySuggestionsto thecontext-menuhandler inapps/desktop/src/main/index.ts, and has done all along.buildEditableTextContextMenunever declared either field, so the menu it built went straight to undo/cut/copy/paste.replaceMisspellingandaddWordToSpellCheckerDictionaryappeared nowhere in the codebase.The builder now prepends the suggestions, each applying via
replaceMisspelling, then a separator, then Add to Dictionary, then a separator before the existing edit items. An empty suggestion list shows a disabled No Suggestions entry so the feature is visibly present instead of silently absent. All labels go through i18n like the rest of the file.The builder takes the
WebContentsit acts on as a third argument; both APIs hang off it.On
setSpellCheckerLanguagesConfirmed genuinely unset, and deliberately left that way. Electron's typings state the macOS spellchecker is the OS one and detects the language itself, making the call a no-op there; Windows and Linux fall back to the dictionary for the app locale. The reporter's squiggles are proof that the default already resolves to a working dictionary on her machine.
Wiring it to the app's UI language would actively regress anyone running an English UI while writing in another language. Choosing a writing-language source is a product decision, not a bug fix, so this PR does not invent one.
Bug 2 — two menus, and the wrong one held focus
The native menu is popped from the main process while the renderer independently opens BlockNote's floating formatting toolbar off the same pointer sequence. Neither knew about the other, so the toolbar appeared without focus and stayed inert until force-clicked.
BlockNote opens that toolbar from a capture-phase
pointeruplistener on the document. Whethercontextmenuarrives before or after thatpointerupis platform-dependent, so racing its listener would have meant a platform branch. Instead the toolbar gates on state: acontextmenuinside the ProseMirror DOM hides the floating toolbar until the next ordinary interaction (a non-secondarypointerdown, or any key). The listener is capture-phase onwindow, which runs ahead of anything that could stop propagation.Where that state lives turned out to be the whole problem. The first attempt put it in
ReviewFormattingToolbarand passed its unit tests, but failed in the real app.FormattingToolbarControllertears the toolbar down and mounts a fresh one during the very interaction being suppressed — aMutationObserverrecorded removed/added twice, at 4ms and 14ms — so theuseStatedied with the old instance and the replacement rendered as if nothing had happened. The suppression fired and then undid itself.The gate now sits in
ReviewFormattingToolbarController, which ContentArea mounts and BlockNote never touches. When it is open the controller returns null andFormattingToolbarControlleris not rendered at all. The sticky toolbar and the double-click selection path are untouched, which the reporter explicitly asked for.Verification
Bug 1, in a real Electron process. A probe loaded a spellchecked
contenteditable, sent a genuine secondary-buttonsendInputEvent, and fed the resulting params through the realbuildEditableTextContextMenu:Clicking the suggestion replaced the word in the live page. Add-to-dictionary was verified across a full app restart: a second run reported
misspelledWord: ""for the same word, andlistWordsInSpellCheckerDictionary()returned["recieve"]. The probe word was then removed again, so no dictionary state was left behind.Bug 2, in the packaged app under Playwright. Against a renderer bundle rebuilt from scratch and grepped to confirm it contained this arrangement: a normal double-click still opens the floating toolbar, a right-click leaves none visible, and the next ordinary selection opens it again. Four consecutive passes, no flake. The build carrying the earlier broken arrangement served as the negative control and fails the same assertion, so the test is load-bearing. Guards against a vacuous pass: the selection still reads the clicked word afterwards, the editor keeps focus, and a main-process counter saw exactly one genuine
context-menuevent.Unit tests. Suggestion ordering, the empty-suggestions placeholder, the correctly-spelled no-op, both click handlers, and the toolbar gate. Every new test was mutation-checked: reverting the corresponding fix turns it red. The mocked
FormattingToolbarControlleris keyed so one test forces the remount that defeated the first attempt — putting the state back in the inner component fails that test and only that test.Gates.
pnpm lint,typecheck:web,typecheck:node,typecheck:test,i18n:check,git diff --check,docs:impact --strict,docs:buildall pass.Windows
The reporter is on Windows; I can only run macOS. No platform-specific branch was added anywhere in this change, which is the point of gating bug 2 on state rather than on event order. The one platform-dependent behaviour in play, the spellchecker's language source, is described above and is Electron's own default on both platforms.
One honest gap: the Playwright harness has to neutralise the native menu to run at all, since Playwright cannot see or dismiss an OS menu. So the E2E covers the toolbar half only. The native menu half — that the suggestions actually render in the Windows context menu — is covered by the Electron probe above on macOS and by construction elsewhere, and deserves one manual pass on Windows before release.
Note on a pre-existing red gate
pnpm typecheckandpnpm buildfail onorigin/mainbefore this branch exists, in the architecture boundary check:That file arrived with #1863 and this PR does not touch
apps/mobile. It needs its own fix.The final commit pushed with
MEMRY_DOCS_IMPACT_SKIP=1: it relocates where the gate's state lives and changes no user-visible behaviour beyond what the docs commit on this branch already describes.docs:impact --base origin/main --strictreports the branch as covered.