Skip to content
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

Merged
merged 19 commits into from
Sep 11, 2024

Conversation

DaniGuardiola
Copy link
Contributor

@DaniGuardiola DaniGuardiola commented Aug 30, 2024

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 to useResizeObserver) 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)

@DaniGuardiola DaniGuardiola marked this pull request as ready for review August 30, 2024 15:47
@DaniGuardiola DaniGuardiola requested review from ciampo, jsnajdr and a team August 30, 2024 15:47
Copy link

github-actions bot commented Aug 30, 2024

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: DaniGuardiola <daniguardiola@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@ciampo ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Aug 30, 2024
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loving the cleanup!

In my tests, the indicator is not showing when switching the writing direction to RTL. We need to RTL-proof some settings, I guess

Screenshot 2024-08-30 at 18 27 48

packages/components/src/tabs/styles.ts Outdated Show resolved Hide resolved
@DaniGuardiola
Copy link
Contributor Author

DaniGuardiola commented Aug 30, 2024

@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>
Copy link

github-actions bot commented Aug 30, 2024

Flaky tests detected in 522d816.
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/10814115022
📝 Reported issues:

@ciampo
Copy link
Contributor

ciampo commented Aug 31, 2024

Trunk looks fine to me, so it seems like something introduced by this PR.

Screenshot 2024-08-31 at 10 51 17

I haven't looked deeply into it, but here's where I'd look:

  • the first suspect that I see is the transform-origin ?
  • in general, we need to flip anything that is explicitly or implicitly anchored to the left and scaling to the right
  • when the tabs don't "fill" the whole tablist width, that may affect the positioning of the indicator in RTL screen? Maybe we could consider changing from display: flex to display: inline-flex ?

@ciampo
Copy link
Contributor

ciampo commented Sep 2, 2024

Update: we should look at @t-hamano 's proposed fixes in #64965 as an inspiration for what tweaks to make to support the animation for RTL writing directions

@t-hamano
Copy link
Contributor

t-hamano commented Sep 2, 2024

Curiously, the visual width of the indicator seems to change depending on the display and browser zoom. Is transform-origin or transform: scaleX() affecting it? I can't see this issue in trunk.

✅ Display scale: 100%

image

❌ Display scale: 125%

image

❌ Display scale: 100%, Browser zoom: 150%

image

This issue occurs in Chrome but not in Firefox.

I'm testing on Windows OS, can you reproduce this issue on MacOS?

@tyxla
Copy link
Member

tyxla commented Sep 2, 2024

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:

Screenshot 2024-09-02 at 12 49 01

Can't reproduce in trunk:

Screenshot 2024-09-02 at 12 49 54

@DaniGuardiola
Copy link
Contributor Author

DaniGuardiola commented Sep 2, 2024

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 ToggleGroupControl, since that one will need to use the previous positioning-based strategy, so thank you!

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.

@DaniGuardiola
Copy link
Contributor Author

Do y'all think this is changelog-worthy? I'm not really sure since it's a minor internal detail.

@DaniGuardiola DaniGuardiola requested a review from ciampo September 2, 2024 10:17
@t-hamano
Copy link
Contributor

t-hamano commented Sep 2, 2024

@DaniGuardiola

Regarding indicator size, I cannot reproduce in any browser, including Chrome in Mac OS.

Can you reproduce the issue by changing the zoom level of the Chrome browser? My Chrome version is 128.0.6613.114, but I can reproduce the issue by changing the browser zoom level to something other than 100%.

@ciampo
Copy link
Contributor

ciampo commented Sep 2, 2024

I think it needs a CHANGELOG entry under the "Bug fixes" section since it implicitly fixes the RTL bug flagged by @t-hamano

@ciampo
Copy link
Contributor

ciampo commented Sep 2, 2024

Tested again on a fresh install — I can still reproduce on latest Chrome (version 128.0.6613.114), Firefox and Safari MacOs 🤷

@WordPress/gutenberg-components am I the only one?

@t-hamano
Copy link
Contributor

t-hamano commented Sep 2, 2024

The latest commit hash for this PR is 4db2b35, is the issue occurring as of this commit?

@ciampo
Copy link
Contributor

ciampo commented Sep 2, 2024

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

@DaniGuardiola
Copy link
Contributor Author

@ciampo tested in Mac.

@t-hamano
Copy link
Contributor

t-hamano commented Sep 2, 2024

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

@DaniGuardiola
Copy link
Contributor Author

Tested on Windows now using the playground, still can't repro.

@ciampo
Copy link
Contributor

ciampo commented Sep 2, 2024

I can 🤷

Kapture.2024-09-02.at.21.28.33.mp4

@ciampo
Copy link
Contributor

ciampo commented Sep 2, 2024

I also tested on latest chrome on MacOS Sequoia via browserstack (free trial), and still managed to repro 🤷

Screenshot 2024-09-02 at 21 32 02

@DaniGuardiola
Copy link
Contributor Author

HOW

@tyxla
Copy link
Member

tyxla commented Sep 2, 2024

Very easy to repro with the latest Chrome in macOS:

Screen.Recording.2024-09-02.at.22.59.38.mov

@DaniGuardiola
Copy link
Contributor Author

AH! Needed to set a gradient, I was missing that. I'm onto something now...

image

@t-hamano
Copy link
Contributor

t-hamano commented Sep 3, 2024

I looked into the cause of the problem and found that when the Tabs component is rendered in the Popover component, rect.width and rect.height becomes zero here.

This may lead to some CSS variables becoming NaN as a result.

Perhaps the problem is related to the animation. If I enable prefers-reduced-motion to disable the popover animation, the issue does not occur. Is there any way to solve this issue..?

1db84e28328a320bd6fa05b63f9ebe06.mp4

@DaniGuardiola
Copy link
Contributor Author

DaniGuardiola commented Sep 9, 2024

I've updated the PR with two details:

  • Synced the useResizeObserver hook with the latest version from Add useEvent and revamped useResizeObserver to @wordpress/compose #64943, as the ultimate goal is to remove it from here and use it from @wordpress/compose.
  • Added a system to useTrackElementOffsetRect for resilience to an edge case (like the one we're running into with the background color popover) by attempting to measure again after a frame, and if that fails, polling until it succeeds. Thanks @ciampo for pairing with me to look into this, and @t-hamano for helping out as well.

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.

@DaniGuardiola
Copy link
Contributor Author

Update: I have now removed the duplicated utility since #64943 has been merged.

@tyxla tyxla requested a review from a team September 10, 2024 12:29
Copy link
Contributor

@ciampo ciampo left a 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)

packages/components/src/tabs/styles.ts Outdated Show resolved Hide resolved
packages/components/src/tabs/styles.ts Outdated Show resolved Hide resolved
@DaniGuardiola DaniGuardiola enabled auto-merge (squash) September 11, 2024 14:50
@DaniGuardiola DaniGuardiola merged commit e1ad8c2 into trunk Sep 11, 2024
61 checks passed
@DaniGuardiola DaniGuardiola deleted the improve/tabs-indicator-animation-and-utils branch September 11, 2024 15:24
@github-actions github-actions bot added this to the Gutenberg 19.3 milestone Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants