Navigation: Use block context to determine whether Page List is nested in Submenu#79048
Conversation
|
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. |
|
Size Change: +19 B (0%) Total Size: 8.46 MB 📦 View Changed
|
|
Flaky tests detected in de015e5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/27265276625
|
3a52971 to
8ae77ee
Compare
| }, | ||
| "isParentSubmenu": { | ||
| "type": "boolean", | ||
| "default": true |
There was a problem hiding this comment.
Never, but this is us working within the constraints of block-context mechanics. (Is there a better way?)
We need an attribute with some defined default value here, so that it can then propagate as context. Then, Page List will know that a non-empty $block->context['core/isInsideSubmenu'] signals the presence of the ancestor, and convertly that an empty value means that Page List is to be treated as standalone.
There was a problem hiding this comment.
Could we use an attribute of the navigation block itself rather than the submenu block?
There was a problem hiding this comment.
You're the domain expert, so you tell me. :) I only used Submenu because that was the preexisting logic:
gutenberg/packages/block-library/src/page-list/edit.js
Lines 269 to 273 in 7dd7d62
What might we gain or lose by moving it to Navigation? Is there any possible edge case where a Page List would find itself nested somewhere in a Navigation but without a Submenu as an intermediate node? If so, there would now be a change in behaviour in how Page List renders.
There was a problem hiding this comment.
I'm going to merge based on Nik's approval, but feel free to open a follow-up if you think it's justified to move the attribute. 🙇
There was a problem hiding this comment.
What might we gain or lose by moving it to Navigation? Is there any possible edge case where a Page List would find itself nested somewhere in a Navigation but without a Submenu as an intermediate node? If so, there would now be a change in behaviour in how Page List renders.
I wasn't thinking about moving the attribute, I was wondering if we could use an attribute that already exists in the navigation block, so we don't need to introduce a new one. :)
There was a problem hiding this comment.
Ah, well, my question still stands: would it change anything? Any edge cases? To put it differently: why was the original condition for isNested based on Submenu and not Navigation?
There was a problem hiding this comment.
Looking at #46414, submenu is the correct place for the context. The page list block gets an extra <ul> wrapper when at the root of a navigation block, and when in a submenu it doesn't.
There was a problem hiding this comment.
Looking at #46414, submenu is the correct place for the context. The page list block gets an extra
<ul>wrapper when at the root of a navigation block, and when in a submenu it doesn't.
Thanks for the confirmation, that was my intuition. :)
Attribute `isNested` can be safely removed from the schema. Props ntsekouras
f920da8 to
de015e5
Compare
) ## What this addresses In #78989, opening the post editor with **Show template** enabled can passively render related template parts and referenced Navigation entities. Stale Navigation block attributes are then normalized during render, marking unrelated template parts and navigation menus dirty before the user edits them. Miguel's #79048 removed the Page List `isNested` self-setting path, so this PR now focuses on the remaining `core/navigation` attribute normalization. Without this PR, `core/navigation` converted horizontal `submenuVisibility: "always"` to `"hover"` with `showSubmenuIcon: true`. With this PR, that conversion only runs when the Navigation block, or one of its inner blocks, is actively selected. It doesn't solve the underlying problem of `setAttribute()` calls inside a `useEffect()`, but it softens the impact. A useful follow-up to this PR would reorganize the data model such that we wouldn't need that call at all. ## Testing - Rebased onto current `trunk`, including #79048. - Confirmed in Playground that #79048 removes the Page List `isNested` attribute, but the original #78989 repro still dirties the related Navigation/template-part entities without this PR. - Added an E2E regression test that seeds a template part containing a stale Navigation block referencing a menu, enables Show template in the post editor, and asserts: - passive template rendering leaves dirty entity records empty; - editing the post dirties only the post entity. - Ran `node --check test/e2e/specs/editor/blocks/navigation-passive-rendering.spec.js`. - Ran `git diff --check origin/trunk...HEAD`.
What
Fixes #79015
Currently, the Page List block has an attribute,
isNested, which is automatically set whenever the block renders in the editor, and whose value reflects whether the block has a Navigation Submenu block among its ancestors. The attribute then influences the outcome of the block's server-side render callback.This is a problematic approach for a few reasons (see original issue and links therein).
This PR inverts the control by:
How
Due to the limited mechanics of block context, Navigation Submenu needs an attribute that we can reliably count on to then provide as context. The block has no such attribute (e.g.
idis not required, etc.), so this PR adds a new one —isParentSubmenu— whose value defaults to true. The attribute has no function other than to then provide it as context (core/isInsideSubmenu).Page List, in turn, can sense that signal by declaring
core/isInsideSubmenuamong itsusesContextdata. If the value isn't truthy, it can assume that it is not a descendant of any Submenu.Finally, Page List can lose the problematic
isNestedattribute. This PR removes the attribute and adds the respective block deprecation.Testing
There should be no change in behaviour, except perhaps that it may reduce the occurrence of unwanted editor dirtying (per the original report). Both blocks should work the same, whether they exist independently or whether Page List is found nested within a Navigation Submenu.