fix(#3912): reflect formatting button state when moving cursor via bullet#4535
Open
ethan-james wants to merge 3 commits into
Open
fix(#3912): reflect formatting button state when moving cursor via bullet#4535ethan-james wants to merge 3 commits into
ethan-james wants to merge 3 commits into
Conversation
…rsor via bullet getCommandState treated the outer editable <div> wrapper (returned by selection.html() for a collapsed caret) as text, which cleared every command match and made the Bold/Italic/Underline/Strikethrough buttons report no formatting. Skip non-formatting tags instead, respecting quoted attribute values (e.g. placeholder="<b>One</b>") that may contain '>'. Adds a data-active hook to ToolbarButton, an e2e regression test, and unit tests for the editable-div-wrapped cases.
Contributor
|
|
…tead of getCommandState Return the editable's inner HTML rather than its outer wrapper element when the caret is collapsed on the editable element (e.g. cursor moved via bullet tap), so selection.html() no longer emits the wrapper <div> and its placeholder attribute (which contains raw HTML). This also benefits the other html() callers (copy/cut/formatWithTag/Note). Reverts the getCommandState tag-skipping change, which is no longer needed. Adds a jsdom unit test for the html() collapsed-caret case.
raineorshine
approved these changes
Jul 8, 2026
raineorshine
left a comment
Contributor
There was a problem hiding this comment.
You can see from 13cc49e that Copilot initially wanted to pursue a defensive fix to skip unrecognized tags in
getCommandState. I opted to fixselection.htmlsince this fix seems to me to be in line with its purpose. If you disagree, we can go back to thegetCommandStatefix.
Thanks, I'm with you. The sneaky polymorphism could be a gotcha in the future, but for now I don't foresee any usage of selection.html outside of Thoughts and Notes.
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.
#3912
Problem
On Chrome/Android, the Bold / Italic / Underline / Strikethrough toolbar buttons flickered back to their inactive state when the cursor was moved to a fully-formatted thought by tapping its bullet — even though the whole thought is formatted. The button briefly lit up (correct) and then settled to inactive (wrong).
Root cause
The toolbar button's active state comes from
commandStateStore, which is derived fromselection.html()when there is an active selection.When the cursor is moved to a thought via its bullet, the caret is a collapsed selection whose
startContaineris the editable<div>element itself. In that caseselection.html()returned the element'souterHTML— i.e. the entire editable wrapper, including its attributes:Downstream,
getCommandStatewalks this string for formatting tags, treats the leading<div …>wrapper as plain text, and clears every command match — so a fully-bold thought reported no formatting. (Theplaceholder="<b>One</b>"attribute, whose value contains raw HTML with>characters, made this even harder to parse defensively.)Fix
Fix it at the source.
selection.html()is meant to return the selection's contents, not the container element. ItsElementbranch now returns the editable's inner HTML when the node is the contentEditable host:So a collapsed caret on the editable now yields
<b></b>instead of the wrapped<div …>, andgetCommandStatereportsbold: true.The change is narrowly scoped — it only affects the case where the selection's
startContaineris the editable element itself (inner<b>/<span>selections still useouterHTML). It's also a strict improvement for the otherselection.html()callers (formatWithTag,useOnCopy,useOnCut,Note), which want selection content rather than the editable wrapper.Changes
html()returns the editable's inner HTML (not the wrapper element) for a collapsed caret on the editable.data-activeso tests can assert the button's active state without brittle color/selector coupling.Verification
getCommandState,selectionincl. the newhtml()test).eslintandtscclean on the changed files.Alternate Solution
You can see from 13cc49e that Copilot initially wanted to pursue a defensive fix to skip unrecognized tags in
getCommandState. I opted to fixselection.htmlsince this fix seems to me to be in line with its purpose. If you disagree, we can go back to thegetCommandStatefix.