Prevent focus loss on reusable block edition #27950
Merged
Conversation
This file contains 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
youknowriad
added
[Type] Bug
youknowriad
requested review from
ajitbohra,
nerrad,
ntwb and
talldan
as code owners
youknowriad
force-pushed
the
fix/focus-loss-reusable-block-edit
branch
from
83c6695 to
6c9399e
Compare
|
Size Change: -77 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
jsnajdr
reviewed
Oct 13, 2022
| blocks: parsedContent, | ||
| }, | ||
| { undoIgnore: true } | ||
| ); |
There was a problem hiding this comment.
Howdy @youknowriad 👋 I arrived at this editEntityRecord call when debugging the PostPreviewButton component and how it autosaves the post before showing the preview.
The trouble is that when the isEditedPostAutosaveable selector is called, it calls hasChangedContent, which always returns true even if I didn't edit the post at all.
It's because this editEntityRecord call always adds an edit when the editor is being initialized (more precisely, EditorProvider being mounted).
hasChangedContent checks if there are 'blocks' in edits which is always true. The blocks field never goes away.
What do you think would be a good way to fix this? Is the entity's blocks field also initialized anywhere else, outside EditorProvider? Seems to me the initial value of block should be set on the unedited post entity, to live alongside the original content it is derived from. Not in the edits.
There was a problem hiding this comment.
Isn't there a flag maybe we can pass to editEntityRecord to mark the change as "non dirty" or something like that? (we had some kind of initial changes in the past).
There was a problem hiding this comment.
Currently there isn't. And edits.blocks is not really an edit -- it's a computed property from the original entity.content and there is no changed content. Ideally the *.blocks field would always live on the same object as *.content, as they are two different representations of the same underlying data.
There was a problem hiding this comment.
We do have a notion of transientEdits which is what edits.blocks is about but subsequent changes to "blocks" should be considered real edits (mark the post dirty, create undo levels...). Could the first time a "transition edit" be set considered a non-dirty edit?
There was a problem hiding this comment.
Yes, blocks and selection edits are considered transient and should never trigger save. Any real edit will also edit content, setting the value to a serialization function to it. So, we can completely ignore blocks and rely on content edits to be always present.
I'm fixing the hasChangedContent selector in #45090 to ignore blocks and look only at content.
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.
Follow-up to #27887
The recent reusable block refactoring highlighted a bug in the
useEntityBlockEditorhook where the first time blocks are edited, you first get a[]value and then you get the edited value. This was due to hooks order of execution. The fact that sometimes the block came from the edited value and sometimes from the parsed "content" value, caused this issue.This PR updates that hook to always retrieve the blocks value from the "edits" by adding an initialization to that value when the "content" changes.
This should make the "reusable-blocks" e2e test more stable as well.