Block Editor: Add local change detection. #19741
Conversation
epiqueras
requested review from
ajitbohra,
chrisvanpatten,
ellatrix,
mkaz,
talldan and
youknowriad
as code owners
| // The default save outputs null. | ||
| // We can have more nuanced heuristics in the future. | ||
| isLocalChange = | ||
| select( 'core/blocks' ).getBlockType( getBlockName( rootClientId ) ) |
There was a problem hiding this comment.
If a block implements its own save that returns null, this logic will fail, right?
I also wonder about back compatibility: Imagining a current block that uses InnerBlocks without any save and listens to save actions (e.g: executes when saved state changes) to save its InnerBlocks using a rest API. Would a block like that break with this change because if its InnerBlocks are changed the post would not become dirty? I think the odds of a block like that existing are low, but at least we would need to create a dev note.
As an alternative solution, what if we use a supports flag to indicate this type of blocks?
There was a problem hiding this comment.
If a block implements its own save that returns null, this logic will fail, right?
I also wonder about back compatibility: Imagining a current block that uses InnerBlocks without any save and listens to save actions (e.g: executes when saved state changes) to save its InnerBlocks using a rest API. Would a block like that break with this change because if its InnerBlocks are changed the post would not become dirty?
Yes, for both, I don't see value in supporting either, though.
As an alternative solution, what if we use a supports flag to indicate this type of blocks?
It would work, but it would be adding more cruft.
I am not sure. Thoughts @youknowriad?
There was a problem hiding this comment.
If i understand properly where we're coming from here. i believe all these issues are happening because we mix the blocks of the sub-entity into the blocks of the top entity.
I think we discussed this previously, IMO, we should find a way to stop doing this as it doesn't make sense to mix content from entity A into an edit of entity B.
One potential idea I shared previously is to have a unique block list in the "editor" store and each entity has its own "blocks" edits in the "core" store.
There was a problem hiding this comment.
I think the issue is a bit more general than that.
We save serialized content yet detect changes on block objects. Any edit to a block will be dirtying even if its serialization doesn't change.
One potential idea I shared previously is to have a unique block list in the "editor" store and each entity has its own "blocks" edits in the "core" store.
So like reusable blocks, but resolved into a single blocklist? What do you see as the benefits of that approach over this one?
There was a problem hiding this comment.
So like reusable blocks, but resolved into a single blocklist? What do you see as the benefits of that approach over this one?
The benefits is that an entity is dirty only if its block list is dirty (A template block list won't include the blocks from the template area)
There was a problem hiding this comment.
The same is true in this PR.
So for your suggestion, we would need a new property in block objects that potentially contain a selector that gets a block list for their inner blocks. The editor store would then dynamically resolve a complete block list. However, when the block editor makes an edit, it will mutate the full block list, how would the editor know to not dirty the top-level entity. It seems like we are trading some complexity for another.
There was a problem hiding this comment.
Agreed no silver bullet here. I think "onChange" of the "controlled" inner blocks might be the place where we dispatch the change to two block lists. Conceptually speaking, I feel like having in an entity of core-data, things that are saved in another entity will just bite us with this kind of hidden bugs.
There was a problem hiding this comment.
I think "onChange" of the "controlled" inner blocks might be the place where we dispatch the change to two block lists.
We'll still have the same issues when we nest one of these blocks inside another.
Conceptually speaking, I feel like having in an entity of core-data, things that are saved in another entity will just bite us with this kind of hidden bugs.
I don't see a conceptual issue with it. They are object references, it's not like the data is duplicated. That's why this PR was so simple to do.
|
Can we revisit this @jorgefilipecosta? |
|
I am just testing the flows listed for Bug 1 and Bug 2 from #19521 (review). I am assuming 'dirty' in this context corresponds to being in a state of unsaved changes, whether it should be in that state or not. However, I am a bit unsure how to tell which sections are currently 'dirty' - what I am currently going on is that 'something' is dirty when we are able to 'Save Draft' in the top right. I am editing a template with template paragraphs "Template P1" and "Template p2". Bug 1 flow -
Bug 2 flow -
Comparing to Master branch -Flow 2 on master does become savable and brings back the content on 'undo', but stays dirty. |
|
Bug 1 was about template parts becoming dirty from unrelated "undos" in the parent template. This is fixed, as can be seen in your gif. Bug 2 was about changes in template parts, making the parent template dirty. This is also fixed, as can be seen in your gif. |
|
Note that the publish button shows the dirty template parts. |
|
Thanks for the clarification, this makes much more sense now! It does look like both bugs are fixed. However, the "undo" button that becomes available after making changes to a template-part no longer undoes the changes to the template-part. What are your thoughts on this? Should this be considered a regression to fix before merging this PR? Handled in a follow-up PR? Or any other reasoning about this? |
|
Does it behave differently to |
There was a problem hiding this comment.
Does it behave differently to master in that sense? Have you tried making enough dirtying edits, like typing with pauses and block insertion/removal?
Yes, it seems a little different than master. On Master, it seems all edits to the template-part are undo-able. Including changing text content of a paragraph or adding/removing blocks. On this branch, it seems text content is only undo-able if it is done after a block insertion or removal.
Testing the following flow:
- Edit first template-part paragraph
- Insert a block (image block in this case)
- Edit second template-part paragraph.
- Try to undo everything
Master
This Branch
Notice how on master all content is reverted, after all "undo"s are made we return to have "TP-P1" and "TP-P2" as the content in the template part paragraphs.
However, on this branch only content changed after the image block is inserted is reverted. Ending in "TP" and "TP-P2" after all undos are done. (Also, it seems adding the block to the template-part dirties the template still - but thats not a change from master)
| ...omit( state.cache, visibleClientIds ), | ||
| ...mapValues( flattenBlocks( action.blocks ), () => ( {} ) ), | ||
| ...state.cache, |
There was a problem hiding this comment.
Just curious why these were being omit-ed from everything in this reducer in the first place? Now that we are no longer omitting them in the cache, could this cause any potential issues?
There was a problem hiding this comment.
I think it was just to keep it simpler.
Do you see an issue with this @aduth, @youknowriad?
There was a problem hiding this comment.
This has changed quite a bit since last I recall being familiar with it, but my concerns would be one of (a) are we risking to leave orphaned child blocks or (b) are we not cleaning up blocks which are no longer part of state.
Reading the documentation for the function, it seems like ideally we would want to avoid a merge altogether, and that the only reason we have it is to account for reusable blocks. I'm not familiar if we're to the point with recent changes like #14367 to be able to remove this and let RESET_BLOCKS entirely take over the state (no merge). But if not, then I think we probably still want the omit ? Since otherwise, we may leave some state for blocks which are no longer relevant for the next state, right? When is the cache ever cleaned up?
There was a problem hiding this comment.
I'm not familiar if we're to the point with recent changes like #14367 to be able to remove this and let RESET_BLOCKS entirely take over the state (no merge).
That would be ideal, who would be a good person to ask?
|
@Addison-Stavlo I believe that behavior is expected because, from the perspective of the selector that queries for the undo stack, those changes are not "persistent". They are only "persistent" in the context of a specific set of inner blocks. How we deal with this depends on how we want to deal with multiple undo contexts. Do template parts get their own undo stacks? Does everything share a global undo stack? I think a global undo stack makes sense, but I wonder if design has given this more thought. |
epiqueras
force-pushed
the
add/local-change-detection
branch
from
f93895d to
3665ec9
Compare
|
Size Change: +317 B (0%) Total Size: 864 kB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Regarding - Bug 2: Changing things inside the template part also makes the template dirty.
It seems this is still the case: While changing content within blocks in the template part no longer dirties the template, adding/removing blocks in the template part still does.
Regarding - template part text content only undo-able for changes made after block addition/removal
How we deal with this depends on how we want to deal with multiple undo contexts. Do template parts get their own undo stacks? Does everything share a global undo stack?
Either way, I would expect one of the following at this point:
- All Template Part content to be undo-able regardless of adding/removing a block.
- No Template Part content to be undo-able.
Considering the following:
- This fixes the two bugs it aims to (minus the block insertion/removal issue).
- The only remaining bugs after this appear limited to the Site Editor experiment.
- We may need to answer some design questions and possibly work on some larger-scope changes to fix those remaining.
The remaining issues can be addressed in follow ups if that is the route we want to go and I am comfortable approving these changes from my end.
|
Since this involves public API changes, I think we need to answer all questions before moving forward with it. |
This PR only handles attributes; blocks get replaced through a different code path. |
|
I just pushed code that handles more types of block editing like replacements, and that resolves the nearest controlled inner blocks parent in a loop so that this works for many layers of nested blocks. |
That's an excellent question to discuss, and with it comes a great deal of complexity. In principle and ideally, you should be able to undo everything you do. There's some nuance there — we don't undo typing a single letter, perhaps not even a single word, but n characters, sure. Inserting an image? Sure. Changing it? Sure. Changing a property? Sure. As best I can tell, this PR is about each template part having its own undo history, where changes made in a template part are undone separately from changes made to the document itself. Is that correct-ish? I suspect we can probably live with this in the near future, but it does feel like undo should ideally be a global action that applies to every single change you made, regardless of where or how, even if it means marking a document/site dirty again if it means undoing a change inside a template part. So this seems to be something to strive for. |
|
The undo stack is global right now, this PR fixes these bugs. |
|
more work towards this in #21368 |
|
closing in favor of #21368, feel free to reopen if you feel differently ;) |
Description
This PR enhances change detection mechanisms so that an edit's dirtiness can scope to a specific block and its children, solving issue number 2 found by @jorgefilipecosta in #19521 (review). The approach taken is to have attribute updates optionally provide a root client ID and have the persistent change selector optionally take a root client ID. This way, changing a block's inner blocks that don't save to the top-level won't dirty the top-level.
This PR also solves issue number 1 found by @jorgefilipecosta in #19521 (review) by reusing block editor store caches for the blocks that haven't changed by referential equality after a reset. This way, undoing or redoing won't dirty nested blocks that save to other entities unless they have changed.
How to test this?
Follow the steps for the two issues from #19521 (review).
Types of Changes
New Feature: Change detection/dirtiness can now scope to specific blocks. E.g., template parts.
Checklist: