Avoid undo issues when reset parent blocks for controlled blocks #37484
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
|
Size Change: +998 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
Mamaduka
added
the
[Priority] High
youknowriad
force-pushed
the
fix/controlled-blocks-undo-issue
branch
from
e5a6dea to
292ea19
Compare
youknowriad
force-pushed
the
fix/controlled-blocks-undo-issue
branch
from
292ea19 to
8f1807f
Compare
|
can I get some testing for this, it's an important bug fix and I'd like to get it on time for beta 4. We can follow-up with the test later. |
|
Thanks for the PR Riad! In my testing I noticed that if we change something in a reusable block, the first Screen.Recording.2021-12-20.at.12.19.50.PM.movAlso the failing tests seem to be related. I'll keep testing. |
|
@ntsekouras I'm not exactly sure how to reproduce your issue, I haven't been able so far. For the tests, I believe they are unrelated, the navigation test is also failing from time to time on trunk. |
The |
|
@ntsekouras Indeed I can reproduce now, it seems that the last undo level before moving out of the reusable block is always ignore on redo. Seems less important than the issue being fixed by the current PR but I'll take a look. |
|
@ntsekouras this is proving very hard to debug for me and maybe it's something that knows undo/redo state better. cc @ellatrix @mcsf It's also unclear whether this is specific to this branch or something hidden in trunk. (as this mixed undo is completely broken in trunk) |
I think it's on trunk as well and we can land this PR and look at it separately. In general this seems to work well in my testing, but unfortunately I'm having problems running the |
|
This fixes the original issue: fixed.mp4Are there any more detailed testing instructions? Without understanding the technical details of the bug I'm not sure what else to do to reproduce :) |
|
Just noting that some of the failing tests are the same with a different PR |
…d and uncontrolled behavior
|
Ok I think I've managed to fix the e2e tests for the navigation block by refactoring it a bit. I think it was relying on the presence of a bug in the redux store (where we keep blocks there even if not existing in the tree anymore). |
|
I'll add some unit tests for the reducer changes. |
ntsekouras
reviewed
Dec 20, 2021
ntsekouras
approved these changes
Dec 20, 2021
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
tellthemachines
pushed a commit
that referenced
this pull request
Dec 21, 2021
|
Thank you, Riad ❤️ |
|
@adamziel I think you'll be interested in this changeset. |
This was referenced Apr 23, 2024
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.
closes #37361
This uses the first approach here #37361 (comment)
The problem is that useBlockSync is getting a bit out of hand now in terms of complexity but to be honest I'm not sure if we can do better (we probably can but it would take a lot of thinking and refactoring).
Here's the logic here: marking a block as "controlled" is set by useBlockSync but the block can become uncontrolled again if its parent get "reset", what we need at that point is to avoid calling "onChange" for the now uncontrolled block and instead mark it controlled again and populate its content.
It would be good to add an e2e test for this PR as a follow-up or directly here. Would appreciate help with that if you're up for it @Mamaduka