FSE: Compute the block state of different entities separately #21368
Conversation
|
Size Change: +53 B (0%) Total Size: 827 kB
ℹ️ View Unchanged
|
mtias
added
the
[Feature] Nested / Inner Blocks
|
I am not entirely sure "controlled inner blocks" is the right abstraction for the motivation here. What other cases are to be considered here apart from template parts? Would reusable blocks be the same? How does "no knowledge over inner blocks of template parts" interact with:
It might also be that conceptualizing it as a "provider" can lead to better nomenclature. |
mtias
added
the
Needs Technical Feedback
|
That’s excellent feedback! I think there are two different things going on here, so we discussed adding two separate “getBlocks” cases to handle them. Essentially, there is:
Currently, the list of blocks is the same in both cases. Part of this is on the edit-site level, for example, where the onChange and value props of the block editor provider are 1:1 with the same methods for the template entity. So I think the ultimate goal for this PR is to make sure that the parent template entity does not include the controlled inner blocks of a template part when it updates. On approach is making the onChange handler not include those controlled blocks. So the approach using state gives us an opportunity to detect which blocks are being controlled, so that they could be excluded in that scenario. I imagine there is more than one way to accomplish it though! |
noahtallen
force-pushed
the
try/controlled-inner-blocks-detection
branch
from
3a1be24
to
ed7cbc4
Compare
This is a great thought, though. Indeed, I can imagine some more scenarios that could work similarly to this one. For example, when a local block has set a direct templateLock on its own InnerBlocks, it does its own template validation separately from the global template validation. (see #11681 for more context). I think a very similar mechanism could be used to resolve that issue, so could our abstraction be clearer to allow more than just "controlledInnerBlocks"? Maybe we ought to be tracking InnerBlocks metadata, or something along those lines. When it comes to reusable blocks, I'm not familiar with how those work. But, with some testing locally, I see that they do not have the same issue we have with template parts. Editing a reusable block does not dirty the state of the entity which contains it. I'll need to figure out how reusable blocks manages to avoid the same problem, and maybe we can learn some lessons from it. I wonder, since reusable blocks and template parts are so similar, if our UI and code around them should also work similarly |
|
Looking into reusable blocks a bit, and I notice that they are implemented by:
Do you think that we would move to a similar mechanism as the template part block and do away with the nested BlockEditorProvider? |
|
This came up a bit in today's Slack meeting in the context of reusable blocks and nested editors: https://wordpress.slack.com/archives/C02QB2JS7/p1586357559128600 (registration required) At @MeredithCorp we're doing a lot of exploration around persisting data from nested block areas to other posts, post meta, and other "non-standard" data stores. The nested block editor approach works for this but comes with a number of limitations detailed in #19436. @youknowriad suggested that working toward this approach might be a better longer term plan, so I'm very excited to see progress here. I haven't been following FSE too closely (it will likely never be an option for our brands) but I'll have to keep a closer eye on those issues as it seems like there will be more general benefits that come out of that work! |
noahtallen
force-pushed
the
try/controlled-inner-blocks-detection
branch
2 times, most recently
from
a07b9c5
to
5ede108
Compare
|
I added much of the code from #19741 and integrated it with the inner block controller state (previously it did some detection of the block save property). Still not working, but it should eventually help solve the problem where onChange fires even when the changes happen in a controlled inner block area. |
|
@epiqueras you might also be interested in this PR :) |
|
Yeah, I suggested that we integrate #19741 into this, yesterday. The issues discussed here happen because we clear the cache after resets, so we need to persist it. And, because edits bubble up to the root, when they should stop at the first parent without a |
|
@noahtallen Let me know when this is ready for a review :) |
noahtallen
force-pushed
the
try/controlled-inner-blocks-detection
branch
from
f4e8749
to
ed30a91
Compare
docs/designers-developers/developers/data/data-core-block-editor.md
Outdated
Show resolved
Hide resolved
docs/designers-developers/developers/data/data-core-block-editor.md
Outdated
Show resolved
Hide resolved
|
The current status on this is: almost ready, but still buggy. Specifically, some actions trigger the persistence state correctly, but some don’t, and there are some cases where onChange even with the correct state. So I’m still working those issues out. I more or less understand what causes the issues independently, but it’s proving difficult to fix each bug without unfixing a different one! :) my goal is to have this ready for review tomorrow. (Thursday) |
Allows us to tell if a certain block is controlling its own InnerBlocks. This is helpful for entities like template parts which persist their blocks outside of the root block-editor onChange method. Without this state, it would be hard to tell which blocks belong to which entity. With this state, we can easily traverse up the block heierarchy to find the closest parent block which is an inner block controller. Practically, we can use this to make sure that the dirty state between different entities is consistent. This commit also updates the tests which refer to the block state so that they all contain the new state slice in the before state. Otherwise, some of those tests were failing some some of the before state did not exist.
The useBlockSync hook syncs a component't block-edior changes with an onChange or onInput handler. The onChange handler is used for persistent changes, and onInput is used for other changes. All changes are scoped to the entity provided: e.g. w.r.t. a root clientId. No changes are included which are part of other entities. For example, a wp_template containing a wp_template_part will not be notified of changes within that template part, but only to changes in the template itself. In other words, it ignores changes which happen in nested inner block controllers. Previously, very similar logic was used for BlockEditorProvider, but it proved necessary to extract so that it could be integrated into InnerBlocks later on. At the same time, we have refactored BlockEditorProvider into a functional component with hooks to save space and to remove needless higher order components.
This changes InnerBlocks into a functional component. Practically, this should work exactly the same as before. This allows us to more easily use the useBlockSync hook in order to set up inner block controllers (i.e. with template parts). We now specify a ControlledInnerBlocks component which handles block sync instead of having that logic within InnerBlocks itself. It also allows us to reuse the same logic as the root BlockEditorProvider, so that we don't miss any of the logic and race condition fixes that already exist there. I did try adding similar logic to the old InnerBlocks component with what existed, but it proved very prone to breakage and was hard to make compatible with the root entity controller. It is much more straightforward to keep the logic in the same place! We also refactored the react native file for inner blocks, and added forwardedRef and block context.
- Correctly include and exclude clientIDs from block reset depending on whether they are part of an inner block controller or not. - Also handle inner block controllers themselves, not updating their cache or order when the action occurs - Stop invalidating grandparent controller cache - Fix undefined index access in selector state - Handle replaceInnerBlocks action similar to the reset blocks action so that we do not inadvertantly delete a template part from the editor. There were a lot of bugs around RESET_BLOCKS and REPLACE_INNER_BLOCKS actions as relating to controlled inner blocks. This is because those actions tend to delet all blocks from the editor without providing back all of the blocks to reinsert. This is because the entity which dispatches the action does not have access to the inner blocks of other entities, so they are not dispatched in the action. These changes update the reducers to make sure that nested inner block controllers are not deleted when they should stay in the editor. Additionally, there are some fixes for the cache state so that it does not invalidate when the changes affect a different entity.
Some tests began failing since the block did not exist immediately when the tests tried to interact with the inserted blocks.
noahtallen
force-pushed
the
try/controlled-inner-blocks-detection
branch
from
a0d3968
to
7c31ce3
Compare
|
not sure what happened since the tests were passing earlier, but every e2e test is failing now. it almost seems like an env issue since the failures are related to not being able to activate the test plugins |
|
Yes, all other PRs are failing too. |
Co-authored-by: Chris Van Patten <hello@chrisvanpatten.com>
|
cc @epiqueras @youknowriad do we think this is ready to go? 👀 Tests are green ✅ |
noahtallen
changed the title
Description
Ultimately, this PR fixes dirty and undo state in the site editor related to different entities. Edits to template parts, nested template parts, and the root template are now considered separately. To test this, make sure that changes you make to one entity do not cause any other entity to become dirty. Additionally, test that undoing a parent entity does not delete a child entity from the editor. Also test that you do not dirty the editor by switching between different templates/template parts from the template selector dropdown.
A few aspects of the block-editor were changed to accomplish these changes.
getBlockselector to not return controlled inner blocks. This way, blocks which belong to a child entity are not returned when asking for the blocks of a parent entity. This also meant updating thegetBlockscache so that it does not refresh when blocks belonging to a different entity are changed.Many edge cases have been fixed so far in this PR, so it should be ready to go after we have more tests! Here is a list of a few things we were considering at the start of the PR that have now been fixed:
Not sure if nested controlled inner blocks work properly. My guess is that they do not work as expected.✅ Nested inner block controllers should now work properly.Will things like the undo stack still work correctly?✅ FixedNeed to probably update react native files✅ DoneHow has this been tested?
Locally in edit-site, in the post/page editor, and via unit/e2e tests.
Types of changes
Bug fix/enhancement
Checklist: