Table of Contents: fix undo history quirk

[ZebulanStanphill]

Description

As noted in #29739, there is an undesirable quirk with the current implementation of the Table of Contents block:

When a Table of Contents block is present, every letter typed in a Heading block counts as a separate undo step, in contrast to the default behavior of multiple letters typed in sequence being combined into a single undo step. This only affects typing in Heading blocks; other blocks continue to behave as expected, with whole strings of sequentially typed text counted as a single undo step as expected.

Unfortunately, I am not aware of any way to fix this, so I could use some expert technical help here.

Step-by-step reproduction instructions

1. Insert a Table of Contents block.
2. Insert a Heading block.
3. Type "Hello world" into the Heading block.
4. Click the "Undo" button in the main toolbar of the editor.
5. Notice that each undo step removes a single character, instead of the entire sequence of typed text.

Screenshots, screen recording, code snippet

No response

Environment info

• WP 6.0-alpha-52828
• Gutenberg trunk as of ff56d57

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[kevin940726]

I believe this issue is caused by having to do setAttributes in useEffect on changes of some external data.

As I mentioned in this comment of how and why "undo trap" is an issue, although this issue is technically not that kind of issue, they are similar in a way that most practices still apply.

If we edit a Heading block character-on-character, (I think) this happens:

1. Type in H, the block calls setAttributes({ content: 'H' }).
2. Type in i, the block calls setAttributes({ content: 'Hi' }), isUpdatingSameBlockAttribute is true, not pushing to the undo stack (not persistent).
3. Stop typing, push Hi to the undo stack. Results in only one undo level.

However, if we add a ToC block to the post, something changes because of the useEffect we have:

1. Type in H, the Heading block calls setAttributes({ content: 'H' }), the Heading block is updated to H.
2. The useEffect in the ToC block picks up the change and updates itself with setAttributes({ headings }). This breaks the isUpdatingSameBlockAttribute check in the original flow, thus creating a new undo level.
3. Type in i, the Heading block calls setAttributes({ content: 'Hi' }), the same thing happens again from (1).

I'm not sure if there's an easy solution for this, but one possible solution might be refactoring the block into a dynamic block so that it doesn't have to call setAttributes inside useEffect at the wrong time.

Note: A more general way to think about this is to "derive the state" rather than setting them in useEffect. However, since we can't call useSelect in Save, the architecture makes it extremely hard to do so. The official doc even recommends us to store derived states in attributes. I think we might need a new API or data type for the derived states, but that's for a bigger conversation.

[kevin940726]

I'm not sure if there's an easy solution for this

Well, I think we can listen to isTyping and only update the ToC block when the user stops typing. The ToC block will update itself with a delay though, but I think it's acceptable in most cases. I'll put up a draft PR using this method.

Scratch that, I don't think isTyping is the right property to listen to. But the idea is the same: defer the update. I'll keep looking.

Scratch that too 😅, I don't think delaying the update is the right approach, it'll still break the undo history. We might have to go for the dynamic block approach in the current architecture.

[richtabor]

I don't think this is a big enough deal to block.

Sure, if the block needs to go dynamic to better support identifying headings within the post content, wherever the block is rendered on the page (#41173 (comment)), it's fine to dive in after—but otherwise it's hardly noticeable.

[annezazu]

👋🏼 I'm doing a sweep of high priority issues to keep this label relevant and actionable. This effort has clearly stalled so the label has been removed as it was added in a release specific window. If you believe this should be added back in, please comment with some context. I am making my best estimates across the project and want to get this right!

P.S. there's a current effort underway that might nicely address this issue.