fix: changing block type over a multi-block selection destroys text - #1227
Open
matanrotman wants to merge 1 commit into
Open
fix: changing block type over a multi-block selection destroys text#1227matanrotman wants to merge 1 commit into
matanrotman wants to merge 1 commit into
Conversation
Selecting several blocks and toggling a heading (or Text) leaves every block
holding the FIRST block's text. The other blocks' content is gone — not
merely re-typed or re-ordered, but overwritten and unrecoverable through undo
once the document is saved.
The cause is one line, in three places:
final delta = (node.delta ?? Delta()).toJson(); // read once: first block
editorState.formatNode(selection, (node) => node.copyWith(
attributes: {blockComponentDelta: delta}, // written to EVERY block
));
`formatNode` runs its callback for every node in the selection, so hoisting
the delta out of the callback stamps one block's text over all the others.
The delta is now read from the node being transformed.
Two details made this easier to hit than it looks:
- `heading_toolbar_items.dart` and `paragraph_toolbar_item.dart` also captured
`selection` at widget *build* time, so a press could apply to a selection
that no longer existed. Both now read `editorState.selection` when pressed.
This is why `onlyShowInSingleSelectionAndTextType` did not prevent it: the
guard is evaluated at build time against the old selection.
- `heading_command_shortcut.dart` has no single-selection gate at all, so the
heading keyboard shortcuts could apply to a selection of any size.
Reproduced by the added test in
`test/new/block_component/heading_block_component/heading_command_shortcut_test.dart`,
which fails on the current code with ['first', 'first', 'first'] and passes
with the fix as ['first', 'second', 'third'].
This was found after it destroyed a paragraph in a real document.
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.
What's wrong
Selecting several blocks and toggling a heading — or switching them to Text — leaves every block holding the first block's text. The other blocks' content is overwritten and gone.
Cause
One line, in three files:
formatNoderuns its callback for every node in the selection, so hoisting the delta out of the callback stamps one block's text over all the others. This PR reads the delta from the node actually being transformed.Two details that make it easier to hit than it looks
heading_toolbar_items.dartandparagraph_toolbar_item.dartalso capturedselectionat widget build time, so a press could apply to a selection that no longer existed. Both now readeditorState.selectionwhen pressed. This is whyonlyShowInSingleSelectionAndTextTypedoesn't prevent it — the guard is evaluated at build time, against the old selection.heading_command_shortcut.darthas no single-selection gate at all, so the heading keyboard shortcuts apply to a selection of any size.Test
Added to
test/new/block_component/heading_block_component/heading_command_shortcut_test.dart. On currentmainit reports:and passes with the fix.
Verification, honestly
The failing-then-passing run was done against a checkout whose
heading_command_shortcut.dartis byte-identical to this branch's base, on Flutter 3.27.4. I could not compile this branch locally —appflowy_editoronmainrequires Flutter >=3.32.0 — so CI here is the first real compile.dart formatis clean on all four files.Context
Found after it destroyed a paragraph in a real document. The same mistake exists twice more in AppFlowy-IO/AppFlowy (
text_heading_toolbar_item.dart,heading_toolbar_item.dart); a companion PR covers those.