Open ZebulanStanphill opened 2 years ago
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:
H
, the block calls setAttributes({ content: 'H' })
.i
, the block calls setAttributes({ content: 'Hi' })
, isUpdatingSameBlockAttribute
is true, not pushing to the undo stack (not persistent).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:
H
, the Heading block calls setAttributes({ content: 'H' })
, the Heading block is updated to H
.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.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 calluseSelect
inSave
, 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.
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.
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 (https://github.com/WordPress/gutenberg/issues/41173#issuecomment-1613437159), it's fine to dive in after—but otherwise it's hardly noticeable.
👋🏼 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.
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
Screenshots, screen recording, code snippet
No response
Environment info
trunk
as of https://github.com/WordPress/gutenberg/commit/ff56d57b3b05c7e3459f7b07ecb93c24223e215aPlease confirm that you have searched existing issues in the repo.
Yes
Please confirm that you have tested with all plugins deactivated except Gutenberg.
Yes