Performance: Remove is-typing root class
#32567
Merged
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: +777 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
gwwar
changed the title
gwwar
changed the title
is-typing root class
|
Re-opening due to interest, since the is-typing class is only used in one area. |
|
related #32101 |
|
The other alternative is to actually apply the class to the hovered block only (to avoid having to rerender all blocks but just one) |
I avoided that since this was previously moved up in #30106. I can try it again if we changed our minds. |
|
Oh interesting, it'd be good to get @ellatrix's perspective on this PR. |
|
To recap for folks reviewing:
I'm open to other approaches if folks think of it too. Overall, I personally do think the outline behavior in site editor is a bit confusing when working with nested blocks, so I wonder if there have been some other proposed interactions. I'm thinking list view can probably help a lot with navigation. |
youknowriad
reviewed
Jun 10, 2021
| @@ -83,6 +88,15 @@ function BlockListBlock( { | |||
| const { removeBlock } = useDispatch( blockEditorStore ); | |||
| const onRemove = useCallback( () => removeBlock( clientId ), [ clientId ] ); | |||
|
|
|||
| const removeBlockOutline = useSelect( | |||
There was a problem hiding this comment.
Do you think we should move this to useBlockProps instead of here (the leaf place where the classname is applied)
There was a problem hiding this comment.
Oh good catch, useBlockClassNames felt like a better fit, so I moved that in 71d7488
There was a problem hiding this comment.
🤔 the class could maybe use a better name, but I'd like to decouple it from is-typing so we can apply it only the situations we need it for.
That's interesting — this may be a historical memory, but I recall the is-typing class existing primarily to support removing the block toolbar when you're hiding. It's very possible that's been refactored since, all the more reason to revisit this:
Looking at what's left, I stand corrected, the class appears to be primarily there to not show the border when you're typing in the site editor while the "outline mode" is enabled. As a single PR that improvese things, this makes sense to me. But I also think we can probably revisit this in the future. The outline mode is a bit experimental and might be absorbed entirely by "select mode", and if that's the case we can remove the extra check. Hope that's helpful. |
youknowriad
added
[Type] Performance
youknowriad
approved these changes
Jun 11, 2021
|
It's interesting that a simple class change on an element re-renders everything within that element. I guess that's just how React works, but I've always wondered why element attributes cannot be seen as things that are next to the tree/branch (like sibling components) rather than on the same tree/branch. |
ellatrix
approved these changes
Jun 11, 2021
ellatrix
reviewed
Jun 11, 2021
| return { | ||
| isOutlineMode: outlineMode, | ||
| isFocusMode: focusMode, | ||
| isNavigationMode: _isNavigationMode(), |
There was a problem hiding this comment.
Similarly, it seems really annoying that all blocks have to re-render if we simply want to toggle any of these modes above and add a class. :)
There was a problem hiding this comment.
💯 though hopefully these have less impact since the modes should be toggled less often 🤞
|
Thanks for the reviews @youknowriad @ellatrix @jasmussen ! |
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
pushed a commit
that referenced
this pull request
Jun 14, 2021
youknowriad
pushed a commit
that referenced
this pull request
Jun 14, 2021
pento
pushed a commit
to WordPress/wordpress-develop
that referenced
this pull request
Jun 15, 2021
…ta 2. This includes: **Various** - Fix multi selection for nested blocks WordPress/gutenberg#32536 - Consistently show the drop indicator while dragging blocks WordPress/gutenberg#31896 - Fix horizontal drop indicator WordPress/gutenberg#32589 - Fix Safari flickering issue WordPress/gutenberg#32581 - Silence useSelect zombie bug errors WordPress/gutenberg#32088 **Template Editor** - Clarify the template creation modal WordPress/gutenberg#32427 - Only add skip links for block templates WordPress/gutenberg#32451 **Widgets Editor** - Add block breadcrumb WordPress/gutenberg#32498 WordPress/gutenberg#32528 WordPress/gutenberg#32569 - Saved deleted and restored widgets. WordPress/gutenberg#32534 - Fix unsaved changes detection WordPress/gutenberg#32573 - Fix button spacing in the header WordPress/gutenberg#32585 - Avoid extra undo levels WordPress/gutenberg#32572 - Move Legacy Widget block to the `@wordpress/widgets` package WordPress/gutenberg#32501 - Fix Social Links color inheritance WordPress/gutenberg#32625 - Use Button appender WordPress/gutenberg#32580 **Global Styles (theme.json)** - Separate the presets per origin in the block editor settings WordPress/gutenberg#32358 WordPress/gutenberg#32622 - Use CSS Custom Properties for the preset styles WordPress/gutenberg#32627 **Performance** - Remove is-typing classname to improve typing performance WordPress/gutenberg#32567 Props nosolosw, jorgefilipecosta, aristath, ntsekouras, peterwilsoncc, mcsf. See #53397. git-svn-id: https://develop.svn.wordpress.org/trunk@51149 602fd350-edb4-49c9-b593-d223f7449a82
nylen
pushed a commit
to nylen/wordpress-develop-svn
that referenced
this pull request
Jun 15, 2021
…ta 2. This includes: **Various** - Fix multi selection for nested blocks WordPress/gutenberg#32536 - Consistently show the drop indicator while dragging blocks WordPress/gutenberg#31896 - Fix horizontal drop indicator WordPress/gutenberg#32589 - Fix Safari flickering issue WordPress/gutenberg#32581 - Silence useSelect zombie bug errors WordPress/gutenberg#32088 **Template Editor** - Clarify the template creation modal WordPress/gutenberg#32427 - Only add skip links for block templates WordPress/gutenberg#32451 **Widgets Editor** - Add block breadcrumb WordPress/gutenberg#32498 WordPress/gutenberg#32528 WordPress/gutenberg#32569 - Saved deleted and restored widgets. WordPress/gutenberg#32534 - Fix unsaved changes detection WordPress/gutenberg#32573 - Fix button spacing in the header WordPress/gutenberg#32585 - Avoid extra undo levels WordPress/gutenberg#32572 - Move Legacy Widget block to the `@wordpress/widgets` package WordPress/gutenberg#32501 - Fix Social Links color inheritance WordPress/gutenberg#32625 - Use Button appender WordPress/gutenberg#32580 **Global Styles (theme.json)** - Separate the presets per origin in the block editor settings WordPress/gutenberg#32358 WordPress/gutenberg#32622 - Use CSS Custom Properties for the preset styles WordPress/gutenberg#32627 **Performance** - Remove is-typing classname to improve typing performance WordPress/gutenberg#32567 Props nosolosw, jorgefilipecosta, aristath, ntsekouras, peterwilsoncc, mcsf. See #53397. git-svn-id: https://develop.svn.wordpress.org/trunk@51149 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Jun 15, 2021
…ta 2. This includes: **Various** - Fix multi selection for nested blocks WordPress/gutenberg#32536 - Consistently show the drop indicator while dragging blocks WordPress/gutenberg#31896 - Fix horizontal drop indicator WordPress/gutenberg#32589 - Fix Safari flickering issue WordPress/gutenberg#32581 - Silence useSelect zombie bug errors WordPress/gutenberg#32088 **Template Editor** - Clarify the template creation modal WordPress/gutenberg#32427 - Only add skip links for block templates WordPress/gutenberg#32451 **Widgets Editor** - Add block breadcrumb WordPress/gutenberg#32498 WordPress/gutenberg#32528 WordPress/gutenberg#32569 - Saved deleted and restored widgets. WordPress/gutenberg#32534 - Fix unsaved changes detection WordPress/gutenberg#32573 - Fix button spacing in the header WordPress/gutenberg#32585 - Avoid extra undo levels WordPress/gutenberg#32572 - Move Legacy Widget block to the `@wordpress/widgets` package WordPress/gutenberg#32501 - Fix Social Links color inheritance WordPress/gutenberg#32625 - Use Button appender WordPress/gutenberg#32580 **Global Styles (theme.json)** - Separate the presets per origin in the block editor settings WordPress/gutenberg#32358 WordPress/gutenberg#32622 - Use CSS Custom Properties for the preset styles WordPress/gutenberg#32627 **Performance** - Remove is-typing classname to improve typing performance WordPress/gutenberg#32567 Props nosolosw, jorgefilipecosta, aristath, ntsekouras, peterwilsoncc, mcsf. See #53397. Built from https://develop.svn.wordpress.org/trunk@51149 git-svn-id: http://core.svn.wordpress.org/trunk@50758 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott
pushed a commit
to gMagicScott/core.wordpress-mirror
that referenced
this pull request
Jun 15, 2021
…ta 2. This includes: **Various** - Fix multi selection for nested blocks WordPress/gutenberg#32536 - Consistently show the drop indicator while dragging blocks WordPress/gutenberg#31896 - Fix horizontal drop indicator WordPress/gutenberg#32589 - Fix Safari flickering issue WordPress/gutenberg#32581 - Silence useSelect zombie bug errors WordPress/gutenberg#32088 **Template Editor** - Clarify the template creation modal WordPress/gutenberg#32427 - Only add skip links for block templates WordPress/gutenberg#32451 **Widgets Editor** - Add block breadcrumb WordPress/gutenberg#32498 WordPress/gutenberg#32528 WordPress/gutenberg#32569 - Saved deleted and restored widgets. WordPress/gutenberg#32534 - Fix unsaved changes detection WordPress/gutenberg#32573 - Fix button spacing in the header WordPress/gutenberg#32585 - Avoid extra undo levels WordPress/gutenberg#32572 - Move Legacy Widget block to the `@wordpress/widgets` package WordPress/gutenberg#32501 - Fix Social Links color inheritance WordPress/gutenberg#32625 - Use Button appender WordPress/gutenberg#32580 **Global Styles (theme.json)** - Separate the presets per origin in the block editor settings WordPress/gutenberg#32358 WordPress/gutenberg#32622 - Use CSS Custom Properties for the preset styles WordPress/gutenberg#32627 **Performance** - Remove is-typing classname to improve typing performance WordPress/gutenberg#32567 Props nosolosw, jorgefilipecosta, aristath, ntsekouras, peterwilsoncc, mcsf. See #53397. Built from https://develop.svn.wordpress.org/trunk@51149 git-svn-id: https://core.svn.wordpress.org/trunk@50758 1a063a9b-81f0-0310-95a4-ce76da25c4cd
|
We were working on a similar issue with child components re-mounting if the wrapping component changed, but we found that simply toggling class names did not rerender anything. I threw up a Code Sandbox example here: https://codesandbox.io/s/es6-forked-up4z8?file=/src/app.js The top component will change the wrapping element based on state. This triggers unmount/remount/rerender for the children. The bottom component does not change the wrapping element, it just toggles the class name. This example does not re-render as this issue seems to indicate would happen. cc @nerrad |
@mikejolley By default a parent will trigger a re-render in all child components. However if a React component returns the exact same element reference in its render output as it did the last time, React will skip re-rendering that particular child. This can be done with a memoization technique or if props.children is the same. When we type, the children property is not the exact same object. children.mp4This is why I tried https://github.com/WordPress/gutenberg/pull/32469/files#diff-d4fe1667e6f1078d2920bddb0fc64559f95cf61e3fe7d2cd4222b7e7ff4bd193R157but folks were not comfortable with memoing items. We can definitely continue to try and stabilize references (if no changes are needed) as it'd be a great improvement in other interactions. |
F-Wilke
pushed a commit
to FiliagoDev/WordPress
that referenced
this pull request
Jul 31, 2021
…ta 2. This includes: **Various** - Fix multi selection for nested blocks WordPress/gutenberg#32536 - Consistently show the drop indicator while dragging blocks WordPress/gutenberg#31896 - Fix horizontal drop indicator WordPress/gutenberg#32589 - Fix Safari flickering issue WordPress/gutenberg#32581 - Silence useSelect zombie bug errors WordPress/gutenberg#32088 **Template Editor** - Clarify the template creation modal WordPress/gutenberg#32427 - Only add skip links for block templates WordPress/gutenberg#32451 **Widgets Editor** - Add block breadcrumb WordPress/gutenberg#32498 WordPress/gutenberg#32528 WordPress/gutenberg#32569 - Saved deleted and restored widgets. WordPress/gutenberg#32534 - Fix unsaved changes detection WordPress/gutenberg#32573 - Fix button spacing in the header WordPress/gutenberg#32585 - Avoid extra undo levels WordPress/gutenberg#32572 - Move Legacy Widget block to the `@wordpress/widgets` package WordPress/gutenberg#32501 - Fix Social Links color inheritance WordPress/gutenberg#32625 - Use Button appender WordPress/gutenberg#32580 **Global Styles (theme.json)** - Separate the presets per origin in the block editor settings WordPress/gutenberg#32358 WordPress/gutenberg#32622 - Use CSS Custom Properties for the preset styles WordPress/gutenberg#32627 **Performance** - Remove is-typing classname to improve typing performance WordPress/gutenberg#32567 Props nosolosw, jorgefilipecosta, aristath, ntsekouras, peterwilsoncc, mcsf. See #53397. Built from https://develop.svn.wordpress.org/trunk@51149 git-svn-id: http://core.svn.wordpress.org/trunk@50758 1a063a9b-81f0-0310-95a4-ce76da25c4cd
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.
When we start typing in a block, the
is-typingclass is added to the root element. Doing so triggers a re-render of all children, even if child props do not change. This is a PR to see what performance impact this has if we do not update the class on the root element.I re-ran the performance job multiple times, each showing around a 100ms difference for maxType. (Click into the performance test, (eg https://github.com/WordPress/gutenberg/runs/2788795658 ) and look at "compare performance with trunk logs".
What
is-typingis used forCurrently there's only one piece of functionality that uses this class. This is only used in the site-editor. We apply a hover outline to help find blocks, but remove this when folks begin typing.
before.mp4
There are two potential approaches so far.
What's current in this branch adds a
remove-outlineclass on the active block that's being typed when isOutline mode is active. We shouldn't see any behavior changes in the site-editor.Alternatively in b62f8b4 I use
:focus-withinto achieve a similar effect. One main difference is that the outline is removed on input focus, rather than typing.An alternative would be to only apply thecc @jasmussen if you think the behavior here is okay.is-typingclass when outlineMode is enabled to avoid re-renders in other block editors.Performance Runs
a005470
7069153
Run 2
Run 3
b62f8b4 Run 4
Testing Instructions
is-typingclass is not applied when we type. A block with a class ofremove-outlineshould not appear either.remove-outlinewhen typing