Table of Contents: Use a custom store subscription for observing headings #54094
Conversation
…ings Co-authored-by: Jarda Snajdr <jsnajdr@gmail.com>
Mamaduka
added
[Type] Performance
| // store by string. When the store is not available, editorSelectors | ||
| // will be null, and the block's saved markup will lack permalinks. | ||
| // eslint-disable-next-line @wordpress/data-no-store-string-literals | ||
| const permalink = select( 'core/editor' ).getPermalink() ?? null; |
There was a problem hiding this comment.
@ZebulanStanphill, do you remember the reason for using absolute URLs instead of relative ones?
There was a problem hiding this comment.
Relative links don't work in situations where the contents of the post (and therefore the Table of Contents, if the post has one) appear on a page other than the one dedicated to the post itself. The most common example of this: a blog template where the entire contents of multiple posts are laid out (which was the default home page template for several past WordPress themes).
|
Size Change: +1.32 kB (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
| // When undoing changes to an attribute, `getBlockAttributes` returns `null`, | ||
| // which triggers an infinite loop of updates. The `undefined` check prevents that. | ||
| if ( | ||
| storedHeadings !== undefined && |
There was a problem hiding this comment.
This condition prevents the headings attribute from being set when I create the TOC block from scratch. Then the initial value of the attribute is undefined, and the observer hook never updates it.
Maybe the headings attribute should have a [] default value specified in block.json.
How do I trigger the infinite update loop that you're trying to prevent here? I suppose it's triggered by undo. I.e., I insert a new heading and on the undo stack there is one record with two updates: block insertion and TOC attribute update. Then I press Ctrl-Z and it causes both updates to be undone, in one step. And the observer hook should handle that gracefully. Is that right?
There was a problem hiding this comment.
Thanks for catching that!
Maybe the headings attribute should have a [] default value specified in block.json.
Yes. I'm going to add an empty default value.
How to trigger an infinite update loop:
- Remove the safeguard and build the code.
- Open a post.
- Add ToC blocks.
- Add a few headings.
- Save post and reload.
- Make changes to one heading.
- Undo all changes. The last undo step triggers the loop.
This highlights that the block attribute isn't reverted to an empty state (undefined), but getBlockAttributes returns null for some reason at that point.
There was a problem hiding this comment.
Yes, I can confirm that undo behaves very strangely when the TOC block is active. Even without triggering the infinite loop. When I edit the heading text from "a" to "ab", and then click Undo, the heading becomes empty instead of "a".
I've been debugging this and one tangible result I already have is the transform fix in #54108 🙂
There was a problem hiding this comment.
I pushed the default value change, which fixes the issue with fresh ToC blocks.
Yes, the undo quirk is a known issue (#41031), there is probably a race condition with RichText's useMarkPersistent.
I plan to change the method of storing headings from attribute to post meta, just like the Footnotes block, and use the isCached flag for the entity instead of __unstableMarkNextChangeAsNotPersistent (#51644). This way, we should avoid race conditions 🤞
P.S. I pushed refactoring as a separate PR to make reviewing easier.
There was a problem hiding this comment.
How to trigger an infinite update loop:
I found what is going on. When you perform undo, the TOC block is replaced with a new one, with the old attributes, and also with a new clientId.
But at the time this RESET_BLOCKS action is done and the observer reacts to it, the old block's Edit component is still mounted, and the useObserveHeadings hook is also still mounted. The effect cleanup will happen a bit later.
What we need to do in the observer loop is to detect that the clientId block no longer exists. That should be easy because the getBlockAttributes selector returns null. You already know that, you wrote it into the comment 🙂 Now we also know what it means. We need to return early from the callback when this condition is detected, and do no update.
There was a problem hiding this comment.
I added an early return in 2c9dc84 + comment. Let me know if you want to include more details in the comment.
There was a problem hiding this comment.
Thanks for the fix, I have nothing to add to the comment.
When you perform undo, the TOC block is replaced with a new one, with the old attributes, and also with a new
clientId.
This, however, seems to be another undo quirk (in addition to #41031). In most cases, undo and redo keep the blocks' clientIds stable, it just modifies the attributes. But in the specific case, where the "heading typing" is the very first edit after loading the post, the undo (namely the RESET_BLOCKS action) regenerates all the affected clientIds. Worth investigating.
There was a problem hiding this comment.
Do you think that's a different issue but more highlighted by the ToC observer? I'm just wondering if this should be a blocker for this refactoring.
As I mentioned in my previous comment, I plan to change the storing mechanism for headings in follow-up PR.
There was a problem hiding this comment.
It's unrelated to the TOC block because I can easily reproduce the behavior (regenerating client IDs on undo) without it:
- create a post with one paragraph block, save it
- reload the page to start editing with an empty undo stack
- edit the paragraph by adding one letter
- hit Undo
After Undo, you'll get the original content, but the paragraph client ID will change. That's because this code in the useEntityBlockEditor is called with undefined editedBlocks:
gutenberg/packages/core-data/src/entity-provider.js
Lines 174 to 182 in c7ff22a
and that will cause the content to be re-parsed from scratch.
@youknowriad recently refactored this code in #52417. Any idea why blocks on the edited entity are lost and why we're parsing the content again? Such things should happen only when the post is refreshed from the server, or other similar events. Not when editing the post and doing undo/redo.
This behavior doesn't cause any user-visible bugs (yet), but it's very suspicious and might indicate some deeper problems.
There was a problem hiding this comment.
Discussion of this problem, which is not strictly related to the TOC blocks, continues here on the PR that introduced the regression: https://github.com/WordPress/gutenberg/pull/52417/files#r1314930854
There was a problem hiding this comment.
Looks good 👍
There is room for further optimization though. At this moment, the getLatestHeadings calculation is performed on every core/block-editor change, even when just moving the cursor, which fires SELECTION_CHANGE actions. This would be nice to optimize/memoize -- detect if the block tree content has really changed.
What?
Part of #42229.
PR refactors the table of contents block to use a custom store subscription instead of
useSelectfor generating the latest heading data.Props to @jsnajdr for suggesting this method 🙇
Why?
The data returned from the
useSelectwasn't directly used for rendering. A custom subscription gives us more control over when to update the data and re-render the block by dispatching theupdateBlockAttributesaction.How?
Most of the logic for getting the latest headings is now in the
getLatestHeadingsmethod. TheobserveCallbackhandles attribute updates.Testing Instructions