Fix synced pattern editing in write mode and refactor block editing mode to reducer #67026
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
|
Size Change: +869 B (+0.05%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 773c76d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11947756308
|
talldan
force-pushed
the
demo/block-editing-mode-branches
branch
from
9363eb2 to
d636bac
Compare
talldan
added
[Type] Bug
talldan
force-pushed
the
demo/block-editing-mode-branches
branch
from
47827e8 to
6f69a32
Compare
This was referenced Nov 20, 2024
talldan
requested review from
draganescu,
youknowriad,
getdave,
ramonjd,
tellthemachines and
aaronrobertshaw
youknowriad
reviewed
Nov 20, 2024
| registry.dispatch( preferencesStore ).set( 'core', 'editorTool', mode ); | ||
|
|
||
| if ( mode === 'navigation' ) { | ||
| speak( __( 'You are currently in Write mode.' ) ); | ||
| } else if ( mode === 'edit' ) { | ||
| speak( __( 'You are currently in Design mode.' ) ); | ||
| } | ||
|
|
||
| dispatch( { type: 'SET_EDITOR_MODE', mode } ); |
There was a problem hiding this comment.
This is the one that I'm not sure about because some people could call registry.dispatch( preferencesStore ).set( 'core', 'editorTool', mode ); directly. Maybe it's fine I don't know
There was a problem hiding this comment.
Yeah, that's a good point, though I guess the spoken messages also won't work when changing the mode that way. Some options:
- (I think you mentioned this one) Always calculate the block editing modes for write mode even when in design mode, then the selector can choose the right one. This'd be a shame as I expect it'd cause a performance hit.
- Add a listener (probably an effect) that reacts to the preference change and sets the mode in the block editor store.
- Make the preference key private so that it can only be called internally. I think this preference has already shipped in the plugin, so it would need some back compat wrangling. Possibly my preferred solution though.
There was a problem hiding this comment.
Also, what happens on "init"? which mode is used?
I may be leaning towards the first option personally to decouple the preference setting entirely from the computation of the mode.
There was a problem hiding this comment.
Also, what happens on "init"? which mode is used?
It uses whatever value the preferences store has at the time of computation. I guess there could be a concern if the preferences store isn't ready yet for some reason.
There was a problem hiding this comment.
Apart from resolving this question, the rest of the PR is pretty finalized (I've added lots of tests).
I'll have a go at your suggestion, I think it should be pretty easy to implement, we'll probably just need an extra state property for navModeBlockEditingModes or something. One thing on my mind is preventing this from being calculated when write mode isn't an option (I think in the post editor it's not an option), as that would be unnecessary.
youknowriad
reviewed
Nov 20, 2024
youknowriad
reviewed
Nov 20, 2024
youknowriad
reviewed
Nov 20, 2024
youknowriad
reviewed
Nov 20, 2024
draganescu
reviewed
Nov 20, 2024
draganescu
reviewed
Nov 20, 2024
draganescu
reviewed
Nov 20, 2024
talldan
force-pushed
the
demo/block-editing-mode-branches
branch
from
6f69a32 to
63d0d2f
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I'm curious to know if I'm testing this correctly or if what I'm seeing is expected. When editing a post with the template preview off, the write/design mode drop down is not visible. There's functionality in the post editor, for example, in the block settings dropdown, that uses the editing mode value from the store. That means, if I leave the site editor or template preview mode after having activated "Write Mode", the post editor knows about it and won't render handy settings such as "Add before" and "Add after":
Especially swapping between site and post editors, I'd have expected to land in default editing mode for regular posting editing, and therefore have access to the block settings features. |
talldan
force-pushed
the
demo/block-editing-mode-branches
branch
from
773c76d to
7b25f6e
Compare
|
@youknowriad As per this discussion, I've updated this now so that it calculates both Also addressed some of the other review feedback. Hopefully it's good to go now. |
|
I'm going to merge this seeing as nobody is telling me not to 😄 The gutenberg RC was only just cut yesterday, so there should be a bit of time to resolve any issues or even revert again if needed before it lands in the next RC. |
WAIT! Just jokes. 🚢
Sounds good. If there's anything, hopefully it'll pop up in the next two weeks with all the eyes on it. |
|
Some of the changes to an e2e test in #67232, which needs to be backported to the 19.8 branch, depend on this PR, so we have two options:
Since this is a bugfix, I'm tempted to backport it instead of removing the test. How confident are you about launching this tomorrow, @ramonjd @talldan ? 😄 |
|
@priethor This PR introduced the E2E test in question, so removing the test from 19.8 seems like the best option to me. This PR is pretty big, so I personally wouldn't want to see it cherry-picked into the release. There have been a couple of follow-up bug fixes it to it already, so it'd mean cherry-picking a few things. |
|
Thanks for confirming, Dan, I was leaning toward that as well 👍 |
michalczaplinski
pushed a commit
that referenced
this pull request
Dec 5, 2024
…ode to reducer (#67026) * Add higher order reducer for pattern block editing modes. - Remove the prev. React effect for managing pattern block editing modes - Implement higher order reducer in the block editor store ... it: - Tracks the clientIds of pattern blocks. - Uses the pattern block clientIds to manage block editing modes for pattern blocks and their inner blocks. - Updates both on any actions that change block lists. * Add higher order reducer for block editing modes while section editing * Handle RESET_ZOOM_LEVEL action * Bug fixes * Avoid mutating `state` in new higher order reducers * Try moving synced pattern client ids into a separate reducer * Revert "Try moving synced pattern client ids into a separate reducer" This reverts commit e1d6ca4. It doesn't really work, since reducer won't have access to `state.blocks.tree`. * Try amalgamating the different derived block editing modes * Fixes * Fix synced patterns in write mode, unbound content blocks being editable * Also update derived block editing mode on `REPLACE_INNER_BLOCKS * Fix descending through controlled inner blocks * Fix nested pattern handling - always process synced patterns in the reducer. Add special handling for only adding pattern block itself as content only when not in zoomed out or nav mode * Zoomed out fixes - content should never be editable in zoomed out, even synced pattern overrides or when write mode is active * Docs * Add end to end test * Remove navigation mode selector tests * Fix partial mocking of blocks package * Add test for isContentBlock * Add unit tests * Remove defaultBlockEditingMode concept * Handle patterns that are outside sections in nav mode * Remove comment * Refactor to handle tree subsections * Optimize each individual action * Fixes and refinements * Comments and renamings * Remove test mocking - else test chokes trying to unlock privateApis * Rework reducer unit tests and add more cases * Add more tests * Handle when the SET_HAS_CONTROLLED_INNER_BLOCKS block has been removed from the state * Inline editing mode calculation for individual blocks * Calculate both the regular and nav mode derived block editing modes at the same time * Update getEnabledClientIdsTree dependencies * Fix tests * Update more tests * Enable write mode experiment for e2e pattern overrides block editing mode tests ---- Co-authored-by: talldan <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: ramonjd <[email protected]>
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.
What?
Supercedes some previous attempts #66919, #66949, and #65408!
Fixes #66424
In Write Mode, parts of synced patterns that are supposed to be non-editable are editable. This PR fixes the problem, plus undertakes some refactoring of how block editing mode works due to some performance caused by so much logic being in the
getBlockEditingModeselector.Why?
This bug happens because Write Mode overrides the blockEditingMode of a synced pattern's inner blocks.
Pattern blocks try calling
setBlockEditingModefor child blocks imperatively (in the pattern block's edit function). This does set the correct block editing modes in the block editor store, but when write mode is active thegetBlockEditingModeselector returns early with a different value.#65408 tried to fix this by updating the
getBlockEditingModeselector, but it cause a performance hit.How?
This PR fixes the issue by moving the pattern block editing modes to the block editor store, so that it can work harmoniously with the way Write Mode and Zoom Out mode determine the block editing mode.
The PR also undertakes a refactor of how block editing mode works for zoomed out and write mode to improve performance. The
getBlockEditingModeselect is called a lot (including recursively), so any code added there can cause performance regressions.The summary of the changes is:
getBlockEditingModeselectorwithDerivedBlockEditingModeshigher order reducer, it calculates a derivedBlockEditingMode state for any actions that might require this is recomputed using a few other functions:getDerivedBlockEditingModesForTreetraverses a block tree calculating the block editing modes for all the blocks in that tree, returning them in aMap. This can be used for the entire block tree or just a part of the block tree.getDerivedBlockEditingModeForBlockcalculates the block editing mode for a single client id. Called bygetDerivedBlockEditingModesForTreeon each block in the tree.getDerivedBlockEditingModesUpdatesused for actions that add/remove a partial subset of blocks. It should only return a value when there are block editing mode changes, meaning the store state doesn't unnecessarily change.traverseBlockTree,getBlockTreeBlock,findParentInClientIdsList,hasBindings.isContentBlockprivate function to the blocks package.It's quite a lot of code, so I'm open to suggestions on how to structure it.
Testing Instructions
In addition to this, give a good test to both zoomed out and write mode, move blocks around, insert and remove stuff and make sure there are no issues. 😄
Screenshots or screencast
Before
Kapture.2024-11-13.at.17.24.45.mp4
After
Kapture.2024-11-13.at.17.28.44.mp4