[lexical][lexical-rich-text] Bug Fix: Fix formatText toggle direction and add SET_TEXT_FORMAT_COMMAND#8807
Open
mayrang wants to merge 4 commits into
Open
[lexical][lexical-rich-text] Bug Fix: Fix formatText toggle direction and add SET_TEXT_FORMAT_COMMAND#8807mayrang wants to merge 4 commits into
mayrang wants to merge 4 commits into
Conversation
mayrang
requested review from
acywatson,
etrepum,
fantactuka,
ivailop7,
potatowagon and
zurfyx
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
… for mixed formatting selections Use selection.format (AND intersection) as toggle reference instead of first node when alignWithFormat is null. Add SET_TEXT_FORMAT_COMMAND and $setTextFormat for explicit format set/unset. Closes facebook#6935
…ion tests for jsdom onSelectionChange does not run in jsdom, so selection.format defaults to 0. The $formatText fix (using selection.format as toggle reference) needs the AND-intersection to be set manually in tests where all TextNodes share a format.
… selection.format toggle Two e2e tests encoded the old buggy toggle direction where the first TextNode determined whether format was added or removed. With the fix (using selection.format AND-intersection as reference), mixed-format selections now toggle ON when the toolbar shows the format as OFF.
mayrang
force-pushed
the
fix/6935-format-text-toggle
branch
from
78aaced to
1ffca49
Compare
etrepum
reviewed
Jul 7, 2026
| }, | ||
| COMMAND_PRIORITY_EDITOR, | ||
| ), | ||
| editor.registerCommand<Partial<Record<TextFormatType, boolean>>>( |
Collaborator
There was a problem hiding this comment.
The generic here should be inferred from the phantom type in the command
Suggested change
| editor.registerCommand<Partial<Record<TextFormatType, boolean>>>( | |
| editor.registerCommand( |
Comment on lines
+2200
to
+2205
| for (const type of Object.keys(formats) as TextFormatType[]) { | ||
| const value = formats[type]; | ||
| if (value !== undefined) { | ||
| $formatText(selection, type, value ? TEXT_TYPE_TO_FORMAT[type] : 0); | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
This could use entries instead:
Suggested change
| for (const type of Object.keys(formats) as TextFormatType[]) { | |
| const value = formats[type]; | |
| if (value !== undefined) { | |
| $formatText(selection, type, value ? TEXT_TYPE_TO_FORMAT[type] : 0); | |
| } | |
| } | |
| for (const [type, value] of Object.entries(formats) as [TextFormatType, undefined | boolean][]) { | |
| if (typeof value === 'boolean') { | |
| $formatText(selection, type, value ? TEXT_TYPE_TO_FORMAT[type] : 0); | |
| } | |
| } |
Wouldn't it be better to compose a format/alignment bitmask and call something equivalent to $formatText once though?
Contributor
Author
There was a problem hiding this comment.
Done — went with the bitmask approach in 60ffebd.
… $setTextFormat, infer registerCommand generic Rewrite $setTextFormat to compose a setBits/clearBits bitmask upfront and apply it in a single node traversal, instead of calling $formatText N times. Remove the unnecessary explicit generic from SET_TEXT_FORMAT_COMMAND registration.
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
2 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.
Description
$formatTextused the first TextNode's format as the toggle reference whenalignWithFormatwasnull. This disagreed withhasFormat()and the toolbar, which useselection.format— the AND-intersection of all selected TextNodes' formats. The mismatch caused the bold button to remove bold from a mixed-format selection even though the toolbar showed bold as inactive.This PR changes
$formatTextto useselection.formatas the toggle reference forRangeSelectionwhen no explicit alignment is given, so the toggle direction now matches what the toolbar displays.It also adds
SET_TEXT_FORMAT_COMMANDand$setTextFormat()for explicitly setting or unsetting individual text formats without toggling.SET_TEXT_FORMAT_COMMANDaccepts aPartial<Record<TextFormatType, boolean>>payload, and$setTextFormat()iterates the map and delegates to$formatTextwith an explicitalignWithFormatper format type.Other changes:
NodeSelectionpath in$formatTextnow passesalignWithFormatthrough togetFormatFlags()instead of hardcodingnull.hasFormatJSDoc corrected from "any" to "all" text nodes.Closes #6935
Test plan
pnpm vitest run --project unit— 211 files, 3684 tests pass.pnpm playwright test --project=chromium— 768 pass, 19 skipped.TextFormatting.spec.mjsexpectations updated to reflect the corrected toggle direction.DecoratorTextExtension.test.ts— setselection.formatmanually (jsdom doesn't runonSelectionChange).$formatTexttoggle direction and$setTextFormat.$setTextFormat(selection, {bold: true, italic: false})sets/unsets independently.