github-actions[bot]
commented
4 days ago Size Change: -7 B (0%)
Total Size: 1.81 MB
| Filename | Size | Change |
|---|---|---|
build/block-editor/index.min.js |
255 kB | -7 B (0%) |
Closed stokesman closed 19 hours ago
github-actions[bot]
commented
4 days ago Size Change: -7 B (0%)
Total Size: 1.81 MB
| Filename | Size | Change |
|---|---|---|
build/block-editor/index.min.js |
255 kB | -7 B (0%) |
github-actions[bot]
commented
4 days ago Flaky tests detected in 5edd745e6e824d80d5a36ee618599314d82a5ea5. Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.
🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11566904008 📝 Reported issues:
/test/e2e/specs/editor/blocks/navigation-frontend-interactivity.spec.js
ramonjd
commented
2 days ago but is there anything else we should be doing?
Do you mean whether there are alternatives to this PR?
stokesman
commented
2 days ago I’m pretty confident this would be okay to land. I’d kept it drafted mostly because I didn’t complete the testing instructions.
I was also interested in testing with Aki’s cool plugin as I figured it’d be a good case where a performance difference could be significant. I just now got around to trying that but only got as far as noticing that the toolbar still shifts with the scrolling 😵💫. I then switched to trunk and saw the same thing:
https://github.com/user-attachments/assets/c93f692a-4094-42f0-bf17-82fe3f2af597
So this PR still appears okay but it seems like the detection of elements that can be skipped is lacking somehow.
github-actions[bot]
commented
2 days ago 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 props-bot label.
If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
Co-authored-by: stokesman <presstoke@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.
t-hamano
commented
2 days ago Thanks for the PR!
I was also interested in testing with Aki’s cool plugin as I figured it’d be a good case where a performance difference could be significant. I just now got around to trying that but only got as far as noticing that the toolbar still shifts with the scrolling 😵💫
Yes, I reported issue #66112 because I was testing my plugin with WP 6.7 😅
I was trying to figure out why my plugin couldn’t fix the toolbar positioning issue, and I found that it occurred when an element was inside a scrollable element.
Below is the code to reproduce the issue:
The video below reproduces the issue, and shows that if I hide the descendants of the scrollable element, the block toolbar works correctly:
https://github.com/user-attachments/assets/35f30779-572b-4bde-b51a-13b0647bd260
ramonjd
commented
1 day ago If @t-hamano is happy with this PR, then so am I 😄
Thank you!
stokesman
commented
19 hours ago However, as discovered in this comment, I still think it's not ideal with regard to the block toolbar position
Right, it seems #66188 fixed the scrollable issue for reduced test cases but for some reason not on your plugin.
Thanks for reviewing folks ❤️. We might as well get this in.
What?
A minor improvement of
getVisibleElementBounds.Why?
To avoid extraneous DOM method invocations. I think it also makes the code more clear. The performance probably isn’t really an issue. Scrollable elements aren’t expected to be the primary use case of the utility anyway.
How?
Skips iteration of children when the parent is scrollable.
Testing Instructions
Scrollable block for use in reproduction/testing
```js wp.blocks.registerBlockType( 'test/scrollable', { apiVersion: 3, title: 'Scrollable block', attributes: { isHorizontal: { default: false, type: 'boolean', } }, edit: ( { attributes: { isHorizontal }, setAttributes } ) => { const { createElement: h, Fragment, useRef } = wp.element; const controls = h( wp.blockEditor.BlockControls, { group: 'block' }, h( wp.components.ToolbarButton, { isPressed: isHorizontal, onClick: () => setAttributes( { isHorizontal: ! isHorizontal } ), }, 'Horizontal' ) ); const menuRef = useRef(); const toggleMenu = ( event ) => { if ( menuRef.current.hasAttribute( 'hidden' ) ) menuRef.current.removeAttribute( 'hidden' ); else menuRef.current.setAttribute( 'hidden', '' ); }; const block = h( 'div', wp.blockEditor.useBlockProps(), h( 'header', { style: { background: '#fff', marginInline: '-.5em', padding: '.5em', borderRadius: '.5em', filter: 'drop-shadow(0 0 .25em #0003)', display: 'flex', } }, '📜 Scroll please', h( 'button', { onClick: toggleMenu, style: { marginInline: 'auto 0' } }, '⋮' ), h( 'div', { ref: menuRef, style: { position: 'absolute', inset: '3rem 0 auto auto', height: '300px', fontSize: '300px', lineHeight: .93, overflow: 'hidden', background: 'inherit', borderRadius: 'inherit', }, hidden: true, }, '⋮' ) ), h( 'div', { style: { overflow: 'auto', background: 'papayawhip', ...( isHorizontal ? { display: 'grid', gridAutoColumns: '5em', gridAutoFlow: 'column' } : { height: '200px' } ) }, }, ...Array .from( { length: 40 } ) .map( ( v, i ) => h( 'p', { key: 'u' + i, children:'p ' + ( i + 1 ) } ) ) ) ); return h( Fragment, null, controls, block ); }, } ); ```Verify the extraneous calls on trunk (optional)
It seems pretty clear just reading the source of the function but if your like me you want to run the experiment.
Diff to log iterations when the element is scrollable
```diff diff --git a/packages/block-editor/src/utils/dom.js b/packages/block-editor/src/utils/dom.js index 0603e9bbb1..027e81eeb0 100644 --- a/packages/block-editor/src/utils/dom.js +++ b/packages/block-editor/src/utils/dom.js @@ -167,6 +167,7 @@ export function getVisibleElementBounds( element ) { let childBounds = child.getBoundingClientRect(); // If the parent is scrollable, use parent's scrollable bounds. if ( isScrollable( currentElement ) ) { + console.log('using parent bounds', currentElement) childBounds = currentElement.getBoundingClientRect(); } bounds = rectUnion( bounds, childBounds ); @@ -174,6 +175,7 @@ export function getVisibleElementBounds( element ) { } } } + console.log('- - - - - - - -'); /* * Take into account the outer horizontal limits of the container in which diff --git a/packages/components/src/popover/index.tsx b/packages/components/src/popover/index.tsx index 3005f13c95..390385091d 100644 --- a/packages/components/src/popover/index.tsx +++ b/packages/components/src/popover/index.tsx @@ -290,7 +290,7 @@ const UnforwardedPopover = ( whileElementsMounted: ( referenceParam, floatingParam, updateParam ) => autoUpdate( referenceParam, floatingParam, updateParam, { layoutShift: false, - animationFrame: true, + animationFrame: false, } ), } ); ```Navigation block with child extending beyond the nav block's bounds
This will confirm that the original fix from https://github.com/WordPress/gutenberg/pull/62711#issuecomment-2413101197 still works.
Scrollable block toolbars
Follow the nice and detailed instructions on https://github.com/WordPress/gutenberg/issues/66112
TL;DR version
Block Popovers and Visualizers
Screenshots or screencast
Making sure the toolbar positioning works as expected on this branch
https://github.com/user-attachments/assets/2f78eea2-0345-48c0-88c4-6ce7e2fb72cd