Allow direct selection of nested Page List block by avoiding dual rendering within block #45143
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
|
|
getdave
requested review from
scruffian,
MaggieCabrera,
annezazu and
draganescu
getdave
added
[Type] Bug
|
Size Change: +138 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
|
Thanks for fixing this @getdave. I pushed a commit which extracts that logic to a function which means we can return early, which I prefer because it makes it much harder for bugs like this to happen again. What do you think? |
draganescu
approved these changes
Oct 20, 2022
|
Pretty sure the failing test is also failing on |
getdave
added
the
Backport to WP 6.7 Beta/RC
|
cc @ockham this will need backporting as it fixes a pretty big UX problem for the Navigation block in that the fallback placeholder (i.e. Page List) will not be selectable in canvas. |
|
Hey @getdave! Since these changes are JS-only, the "Backport to WP Beta/RC" label that you added is enough to bring it on our radar for our Monday triage session, and for inclusion in RC3 on Tuesday 😄 |
ockham
pushed a commit
that referenced
this pull request
Oct 25, 2022
…dering within block (#45143) * Fix dual rendering within Page List * refactor the content of the block to a function Co-authored-by: Ben Dwyer <ben@scruffian.com>
|
I just cherry-picked this PR to the wp/6.1 branch to get it included in the next release: 7061258 |
ockham
removed
the
Backport to WP 6.7 Beta/RC
Reverting on the |
|
ockham
added
the
Backport to WP Minor Release
Mamaduka
pushed a commit
that referenced
this pull request
Nov 10, 2022
…dering within block (#45143) * Fix dual rendering within Page List * refactor the content of the block to a function Co-authored-by: Ben Dwyer <ben@scruffian.com>
Mamaduka
pushed a commit
that referenced
this pull request
Nov 10, 2022
…dering within block (#45143) * Fix dual rendering within Page List * refactor the content of the block to a function Co-authored-by: Ben Dwyer <ben@scruffian.com>
Mamaduka
removed
the
Backport to WP Minor Release
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?
Fixes the Page List to make it selectable in the canvas when it is a child of a container block such as the Navigation block.
Closes #45083
Why?
In #45083 it was reported that it was impossible to directly select the Page List block when it was within a Navigation block. This was also reproducable when the Page List was nested inside other containers such as Group thus confirming that this was not a Nav block specific Issue.
How?
After a lot of deep diving (see Issue) I determine this was due to a
focusinhandler responsible for dispatching theselectBlockaction being _de-_registered on the Page List block node. This meant that despite being able to focus the block it was not being "selected" in the editor.The cleanup function that removes the
focusinevent listener is heregutenberg/packages/block-editor/src/components/block-list/use-block-props/use-focus-handler.js
Lines 62 to 64 in dc10788
This hook uses
useRefEffectwhich is an effect which will run when therefchanges:gutenberg/packages/block-editor/src/components/block-list/use-block-props/use-focus-handler.js
Line 22 in 1b73e8b
The callback that removes the
focusinwith be called byuseRefEffectwhen there is nonodegutenberg/packages/compose/src/hooks/use-ref-effect/index.ts
Lines 38 to 40 in 1b73e8b
Therefore the registering of the
focusinlistener on the Page List node is due to the node being added/remove from the DOM.This made me immediately suspicious of the
rendermethod of Page List. After some debugging in the browser I was able to observe the Page List block node being rendered twice. I tracked this down to the following linesgutenberg/packages/block-library/src/page-list/edit.js
Lines 70 to 74 in 1b73e8b
gutenberg/packages/block-library/src/page-list/edit.js
Lines 91 to 98 in 1b73e8b
These are not mutually exclusive due to the later not being predicated on
hasResolvedPagesbeing truthy. Therefore it was possible for both the loading and the "resolved" rendering state of the block to be rendered at the same time. As both useblockPropsit was causing problems with the registeration of hanlders described above.The fix in this PR ensures that only one representation of the block can be rendered at any one time.
Testing Instructions
Before
http://localhost:8888/wp-admin/edit.php?post_type=wp_navigation).After
http://localhost:8888/wp-admin/edit.php?post_type=wp_navigation).<ul>element.For bonus points also do the following (watch the screencast below to see how to do this):
focusinlistener.<ul>node.onFocuscallback of theuseFocusHandlerhook.Screenshots or screencast
Screen.Capture.on.2022-10-20.at.09-30-48.mp4