-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Improve Tabs indicator animation and related utils #64926
Improve Tabs indicator animation and related utils #64926
Conversation
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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.
@ciampo thanks for the review! Do you know if the RTL issue is pre-existing? Because if so, this can be merged and that can be treated as a separate bug. Though I can't really think of a reason RTL would break things, must be missing something 🤔 |
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Flaky tests detected in 522d816. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10814115022
|
Trunk looks fine to me, so it seems like something introduced by this PR. I haven't looked deeply into it, but here's where I'd look:
|
Curiously, the visual width of the indicator seems to change depending on the display and browser zoom. Is ✅ Display scale: 100% ❌ Display scale: 125% ❌ Display scale: 100%, Browser zoom: 150% This issue occurs in Chrome but not in Firefox. I'm testing on Windows OS, can you reproduce this issue on MacOS? |
Good find @t-hamano 👍 Can reproduce in the latest Chrome on MacOS: Can't reproduce in |
Just added RTL support. Thanks @ciampo and @t-hamano for detecting the issue and @t-hamano for attempting a fix, although now obsolete due to the change to the transform strategy. Your PR will be helpful to me in the follow-up I'm working on for the Regarding indicator size, I cannot reproduce in any browser, including Chrome in Mac OS. Can y'all double-check that you're in latest? Either way, it seems like a bug to me that's probably just been fixed so I think we'll be fine. |
Do y'all think this is changelog-worthy? I'm not really sure since it's a minor internal detail. |
Can you reproduce the issue by changing the zoom level of the Chrome browser? My Chrome version is |
I think it needs a CHANGELOG entry under the "Bug fixes" section since it implicitly fixes the RTL bug flagged by @t-hamano |
Tested again on a fresh install — I can still reproduce on latest Chrome (version @WordPress/gutenberg-components am I the only one? |
The latest commit hash for this PR is 4db2b35, is the issue occurring as of this commit? |
Yup, that's the commit. Are you both testing changes on Windows, or is @DaniGuardiola testing on MacOS? Just trying to find what could be the difference. Kapture.2024-09-02.at.18.28.38.mp4 |
@ciampo tested in Mac. |
Can you reproduce this issue in Playground? If you enter the PR number, we all should be able to test it in the same environment. https://playground.wordpress.net/gutenberg.html 6097f088b7e903af4008fec1bd1b314d.mp4 |
Tested on Windows now using the playground, still can't repro. |
I can 🤷 Kapture.2024-09-02.at.21.28.33.mp4 |
HOW |
Very easy to repro with the latest Chrome in macOS: Screen.Recording.2024-09-02.at.22.59.38.mov |
I looked into the cause of the problem and found that when the This may lead to some CSS variables becoming Perhaps the problem is related to the animation. If I enable 1db84e28328a320bd6fa05b63f9ebe06.mp4 |
I've updated the PR with two details:
If you look at the popover now, you'll see that it's properly sized. In this case, the first fallback (requestAnimationFrame) succeeds. Polling is really just a last resort fallback, in case there's a similar case where there's also an animation delay. It should be really rare, if it ever happens at all, but at least we can rest assured it will never break the indicator. It also should not be too heavy on the CPU, 10 measurements per second seems like a good way to balance performance with speed so that there's no significant flashes or delays for the user. |
…mprove/tabs-indicator-animation-and-utils
Update: I have now removed the duplicated utility since #64943 has been merged. |
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.
LGTM 🚀
Changes tested well on my machine, both in Storybook and in the editor, LTR and RTL, horizontal and vertical, automatic or manual tab activation, in sidebars and in popovers.
Left a couple of minor comments, but they're not blocking.
Also, this needs an updated CHANGELOG entry (likely under "internal", since Tabs
is still private)
What?
This PR simplifies the utils used in animating the indicator, which is also the base for the upcoming ToggleGroupControl animation which is done but will be submitted in a follow-up.
Additionally, it switches to a
transform
-based strategy, which should speed up the animation even further.This work is also related to the
useResizeObserver
refactoring efforts (#64820). Here, the utility is radically simplified (and renamed touseResizeObserver
) with the goal of unifying with that separate effort in a separate PR.Why?
We had to make some compromises to prevent overflow due to the previous method of measuring positions of elements relative to their offset parent, which resulted in a sub-0.5px error margin (see #61979). This updated approach has exact precision which eliminates some critical issues in the upcoming ToggleGroupControl animation work.
I made a playground to illustrate this: https://stackblitz.com/edit/solidjs-templates-grt7kw?file=src%2FApp.tsx
The switch to transform provides better animation performance.
The refactor of
useResizeObserver
aims for upcoming unification in relation to #64820.How?
See diff, it's all extensively documented.
Testing Instructions
Try the tabs indicator animation in the Storybook, plus a few Tabs instances in Gutenberg for good measure (there's a complete list of instances and where to find them in the description of #64371 if that's useful).
Testing Instructions for Keyboard
Interact with tabs through keyboard.
Screenshots or screencast
n/a (just check the storybook, there are no appreciable changes other than the improved subpixel precision)