Fix initial block parsing #52417
Fix initial block parsing #52417
Conversation
youknowriad
added
the
[Type] Bug
|
Size Change: -12 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
The useMemo makes more sense here and kind of reminds me of this tweet - https://twitter.com/sophiebits/status/1293710971274289152.
The failing tests might be related to the change. Do you mind rebasing the branch so we can be sure?
youknowriad
force-pushed
the
fix/initial-block-parsing
branch
from
980394c
to
0a83da8
Compare
|
Flaky tests detected in 0a83da8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5519472039
|
|
@youknowriad, I checked both Screencastsvideo.webmvideo.webm |
|
@Mamaduka I think both behaviors in this PR and trunk are kind of random for the initial selection. I'll see if there's a way to restore trunk's behavior though. |
|
I know wonder whether the behavior in this branch is actually better. At least it's more "logical".
On a new post, both this PR and trunk lose selection. Any thoughts on this @ellatrix |
|
@youknowriad My opinion is that undo/redo selection is flawed, so we shouldn't let that block any other changes. We should look into history with selection again because it doesn't make sense in a lot of cases. |
|
So let's just adjust the e2e test and maybe leave a note with it :) |
|
Ok I went ahead and just updated the tests here. |
youknowriad
force-pushed
the
fix/initial-block-parsing
branch
from
faa1b88
to
3cef5c9
Compare
|
This seems to be breaking some mobile tests sigh |
Hey @youknowriad 👋 , I can take a look tomorrow and share the findings. After a preliminary check, seems that on some tests the HTML generated in the editor is not being propagated to the mocked host app. Hence, when checking the snapshots they don't match. |
|
This PR prevents the editor from creating an "edit" for the "blocks" property in the core state if no changes have been made. Maybe the tests are always serializing the blocks "edit" and not falling back to the actual "content" property in that case. but not sure where this happens. |
@youknowriad I investigated the issue and seems the test failure seems to actually identify a breakage in the app. If I edit the content and undo all modifications, when saving the post it clears all content. ios-empty-content-after-saving.mp4As far as I checked, the problem is related to the initial HTML and getting the blocks in the editor provider. For the latter, if no edits are made it returns an empty list. This is used in the bridge function that synchronizes the post content with the host app to save it. gutenberg/packages/editor/src/components/provider/index.native.js Lines 271 to 306 in 3cef5c9 If you check the return of the above call in the native version of |
|
Following the changes of this PR, I understand that the block list returned by |
|
@fluiddot We have two options: either "parse" the blocks in the selector of So for me the right fix would be to update the component instead. |
Ah, I see. Thanks for elaborating @youknowriad 🙇. However, wouldn't be expected that the selector returns the current block list before editing? At least, based on the selector description, I'd assume that's the case. gutenberg/packages/editor/src/store/selectors.js Lines 1085 to 1093 in 3cef5c9 I'm not opposed to keeping it as-is, but if we go this way, probably we should reflect this nuance in the description to avoid confusion.
I can create a follow-up issue to update this component in the native version following the above convo. Probably I can try to spare some time over the next few days to tackle it. I assume the change would be straight-forward but I'd like to double check no regressions are introduced, especially as that component is a core one. |
I agree but the thing is that it never worked that way and I'm worry about changing the selector now. |
|
Seems like it worked. This PR should be ready now. |
I've tested the iOS app and confirmed that this issue is not happening. Thanks Riad for updating the selector 🙇 ! |
|
Any approvals here :) |
|
Ok there's something wrong in this PR. It multiplied the "type" metric by a factor of 10 :) That's obviously wrong, I'm not sure yet why, but I'll take a look :) and revert if I don't find anything. |
|
Ok, so it's actually the "getEditorBlocks" selector change that is impactful here. Let me see if I can find the reason for that. |
|
It appears the changes in this PR lead to no |
|
@aaronrobertshaw Can you clarify the steps to reproduce the issue here? |
|
@youknowriad, apologies, I should have added those replication steps to my initial comment. They are covered in the PR fixing this issue for sidebar navigation editing #52899.
As noted, for the specific case of editing navigation blocks contained within template parts, this has been fixed as of #52899. I'm not currently aware of any other use cases that might have relied on the |
|
This also broke the 
The I'll open a PR similar to #52899. @youknowriad: Do you think we need to add some backwards compatibility logic to |
If it doesn't hurt performance, I'm all for it. But I want to note that the previous behavior was not guaranteed, it was just an "effect" that was triggered in a random component, that effect could have been triggered or not, depending on the context. It happens that it was in most contexts though. |
|
I guess let's wait to see if we get reports about this from third parties. I assume it won't impact performance if we make get blocks() {
return parse( record.content );
} |
|
I think there's something imperfect with the "transient edits", we haven't specified exactly how these should work and who's responsible for setting them, saving them back to regular edits... |
| return content && typeof content !== 'function' | ||
| ? parse( content ) | ||
| : EMPTY_ARRAY; | ||
| }, [ editedBlocks, content ] ); |
There was a problem hiding this comment.
This change has had one unpleasant side effect on the undo stack. When the post editor is initially loaded, the post entity in the Core Data store will have the content attribute, but there is no blocks attribute. The parsed blocks are stored only as a local memo state in this provider component, and there is no longer any editEntityRecord call that would save them in the store. The blocks attribute is set only on the first real edit.
The first real edit will produce an EDIT_ENTITY_RECORD action that has action.meta.undo.edits.blocks set to undefined. The state.undo reducer will create an undo stack record for this edit, and it will look like { property: 'blocks', from: undefined, to: [ ... ] }.
Applying that undo record means that the blocks attribute will be set to undefined, the value of the from field of the undo record. But instead of undefined, it should have been the original parsed blocks, the value is known.
Setting blocks to undefined means that on undo, they will need to be parsed from scratch from the content string and all client IDs will be different.
This will cause all blocks' edit components to be unmounted and re-created from scratch, on a simple undo operation that shouldn't do this. It should just update one little attribute of one little block.
We've seen the unpleasant effect of this when refactoring the TOC block in #54094 (comment).
There was a problem hiding this comment.
Setting blocks to undefined means that on undo, they will need to be parsed from scratch from the content string and all client IDs will be different.
This will cause all blocks' edit components to be unmounted and re-created from scratch, on a simple undo operation that shouldn't do this. It should just update one little attribute of one little block.
I agree that ideally the components should be unmounted and recreated from scratch but making the "blocks" edit undefined on undo is (in isolation) a fine.
That said, I think maybe we need a way to tell core-data to load these transient values on initial "fetch" of the entities instead of leaving that to consumer code which will always lead to inconsistencies.
There was a problem hiding this comment.
making the "blocks" edit
undefinedon undo is (in isolation) a fine.
In this case it's not fine, because conceptually the blocks value is known and it's not undefined. It just lives outside the data store, in React state. Losing that value leads to losing information, namely the blocks' already assigned client IDs.
I don't know how to fix it, and the issue is not terribly urgent. I mainly wanted to document that it exists and write down what I found.
What?
This fixes a small bug (not something you can see at the moment), where if a post is loaded and then refreshed from the server before any edits happen on that post locally, we will see the previous version of that post and not the last one.
While right now, this bug is not visible, it can be programmatically triggered potentially and also if ever we add an offline cache (say local storage or index db or whatever) for our remote objects (posts), we'll start noticing this post. The editor loads an outdated version of the blocks.
This is also a code quality and performance improvement anyway as we avoid a useless action from being triggered so avoid a lot of useless re-renders...
Also since we're not triggering these useless actions, I expect improvements for things like reusable blocks, template part, post content block loading.
How?
The solution here is to replace the
useEffectthat was responsible for initializing the blocks (initial parsing) with auseMemobecause the "blocks" is actually a computed value based on "content" or any previous block edits.Testing Instructions