Footnotes/RichText: fix getRichTextValues for deeply nested blocks #53034
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
ellatrix
added
[Type] Bug
|
Size Change: +59 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 1b7320b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5681377432
|
There was a problem hiding this comment.
Thanks for looking into this! Unfortunately, it looks like the e2e failures are legitimate. I looked into the failing inserts patterns by dragging and dropping from the global inserter test and attempted to reproduce manually, and there's some odd behaviour surrounding adding and editing nested blocks, particularly with the Social Icons block and with Code editor view.
To reproduce:
- Add a Social Icons block to a post
- Attempt to add child blocks to it, note that the blocks are added very slowly
- Attempt to edit the URL for one of the social icons blocks, note that typing in the field is very slow to respond
- View the code editor view, note that the inner blocks markup for the Social Icons block is empty
Here's a video:
2023-07-28.11.23.18.mp4
| @@ -13,18 +13,20 @@ import { | |||
| import InnerBlocks from '../inner-blocks'; | |||
| import { Content } from './content'; | |||
|
|
|||
| const elementToInnerBlocksMap = new WeakMap(); | |||
There was a problem hiding this comment.
Will this be an issue in subsequent calls? I.e. should the weak map only exist for the lifecycle of a call to getRichTextValues? I'm not too familiar with how WeakMap works, so just thought I'd mention it in case it's related to the odd behaviour with the social icons blocks 🤔
There was a problem hiding this comment.
The problem there is just that we're saveElement may be null, which I didn't take into account. Should be fixed now.
ellatrix
commented
Jul 28, 2023
| // If the innerBlocks passed to `getSaveElement` are not blocks but already | ||
| // components, return the props as is. This is the case for | ||
| // `getRichTextValues`. | ||
| if ( ! firstBlock.clientId ) return { ...props, children: innerBlocks }; |
There was a problem hiding this comment.
|
This turned out to be a simplification too, so that's good. |
andrewserong
approved these changes
Jul 31, 2023
There was a problem hiding this comment.
Nice fix @ellatrix!
This turned out to be a simplification too, so that's good.
Agreed, I like how this means there's only a single entry point for recursing into subsequent addValuesForBlocks calls by consolidating around the InnerBlocks.Content component.
This is testing well for me:
✅ Tested a bunch of permutations of different blocks inside blocks, including Quote inside a Group, and numbering is working correctly, with no duplication.
✅ Adding and editing other container blocks (e.g. social icons) works as on trunk with no perceivable difference.
✅ Gallery blocks (whose edit component expects innerBlocks to be an array here) works as on trunk, and isn't affected by this particular usage of innerBlocks being a component. As far as I can tell, the change in this PR appears to be pretty safe, and any blocks' save functions should ideally check that innerBlocks is not empty before attempting to call array methods like forEach (AFAICT no core blocks' save functions grab innerBlocks in a way that would be of concern).
Quick screengrab of the numbering working nicely for Quote nested in a Group:
LGTM! ✨
|
I just cherry-picked this PR to the update/second-round-6-3-rc3 branch to get it included in the next release: 9a04e31 |
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
tellthemachines
added a commit
that referenced
this pull request
Aug 1, 2023
* Top toolbar: Fix issues with save button overlap, and plugin buttons (#53101) * Shorten the width of the top toolbar overlay and make doc title smaller. * Add a scrim and a margin to handle plugin buttons better. * Remove block tools back compat component schedule for deprecated in 6.3 (#53115) * Removes usage of BlockToolsBackCompat * Remove unwanted BlockTools from Nav sidebar * Footnotes/RichText: fix getRichTextValues for deeply nested blocks (#53034) * Defer to preceding handlers in command palette keyboard shortcut (#53001) * Image block: fix image size at wide and full width (#53184) * Fix regression with Edit site Navigate regions (#52940) * Make the navigabel region wrap the inert sidebar. * Adjust animation. * Fix not expanding pattern in page editor (#53169) --------- Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Footnotes: fix published preview (#53072) * Footnotes: fix published preview * remove var dump * Fix php lint * PHP lint * Address feedback * Add e2e test * Footnotes: disable for synced patterns and prevent duplication for pages in site editor (#53003) * Initial commit: - Prevent footnote creation withing core/block - Only insert a footnote if one isn't found in the entity block list * Try grabbing controlled blocks from parent post content block * Cache `selectedClientId` Get hasParentCoreBlocks using separate useSelect call. * Rename hasParentCoreBlocks to isBlockWithinPattern Add comments * Removing while loop since we're already fetching the post content parent in the `getBlockParentsByBlockName` call above * Reinstating while loop because it can deal with nested blocks --------- Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> * Footnotes: add missing _ in revision field filter (#53135) * Footnotes: add missing _ in revision field filter * Use correct hook name * Revert prefixing callback names * don't display BlockContextualToolbar at all in contentonly locking (#53110) * Render the footer conditionally in the global styles sidebar component so that any side effects from the footer wrapper are not rendered, e.g., styles and what not (#53204) Ensure that the precise bottom margin persists if the footer isn't rendered * Pattern: Add getBlockRootClientId call (#53206) --------- Co-authored-by: Joen A <1204802+jasmussen@users.noreply.github.com> Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: Ella <4710635+ellatrix@users.noreply.github.com> Co-authored-by: Mitchell Austin <mr.fye@oneandthesame.net> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Andrea Fercia <a.fercia@gmail.com> Co-authored-by: Kai Hao <kevin830726@gmail.com> Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>
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 #53009. The problem is that when there's a quote nested in a group, when we encounter the group's
InnerBlocks.Content, we are using the group's inner blocks instead of the quotes.I changed things to enforce using
InnerBlock.Contentfor all blocks, so inner block handling goes through a single code path.I'd love to add some good tests to this come, either in this PR or a follow-up if we run out of time.
Why?
How?
We should link save elements to a block so we can change the innerBlocks when looping over the save elements.
Testing Instructions
Add a quote in a group and add a footnote to the citation. There's shouldn't be a double entry in the footnotes block.
Testing Instructions for Keyboard
Screenshots or screencast