-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix TabPanel initial rendering #49368
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flaky tests detected in 0b7e6d3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4532014019
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! The reasoning to switch to useLayoutEffect
sounds good to me, and with this PR applied, I haven't been able to reproduce the popover positioning issue 👍. I wasn't able to reproduce the issue of focus not going to the correct position within the popover on trunk, though.
LGTM! ✨
Oh, before this lands, it'd be good to add a changelog entry since this affects a component in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great for me, thanks!
✅ useLayoutEffect logic is sound
✅ Focus works as advertised
✅ Color popovers are now positioned correctly as well
+1 for the changelog before landing.
Nice work ✨
Thanks for fixing this ❤️ I can confirm it works in my testing (I needed to use safari/voiceover to repro), so will merge. |
Addresses #48893 (comment)
What?
When a TabPanel component is initially rendering there's a first rendering that happens with "no tab being actually rendered" then an effect triggers to actually set the active tab.
This behavior creates an issue for UIs that are supposed to be rendered right away. For instance if a "DropdownMenu" holds the TabPanel, the focus is not moved to the right position within the dropdown when it opens because at the tab is first rendered as empty.
How?
The solution here uses a
useLayoutEffect
to address this as this hook is supposed to be used for all effects that have an effect on UI causing React internally to only render to the DOM once.Testing Instructions
1- Open a paragraph inspector control
2- Click the "background" color in the style inspector controls.
3- The dropdown should open and the "focus" should be within the dropdown.
I also suspect that this PR could fix the popover bug discovered here #49349 but there might be still a race condition with the Popover component.