fluentui icon indicating copy to clipboard operation
fluentui copied to clipboard

[Bug]: fluent v9 Spinner from react-components package uses main thread for animation, causes jank under CPU load

Open mgostisha opened this issue 1 year ago • 7 comments

Library

React Components / v9 (@fluentui/react-components)

System Info

N/A

Are you reporting Accessibility issue?

no

Reproduction

https://codesandbox.io/s/empty-sky-8dpd3l?file=/src/App.tsx

Bug Description

Actual Behavior

Under CPU load, the spinning animation produces jank and frames are dropped because the animation relies on the main thread to update.

Expected Behavior

The animation can be offloaded to the compositor and is free from jank regardless of the CPU load. (Note: this is how the spinner imported from fluentui/react@^8.0 behaves).

Let me know if it helps to provide profiles for both the good and the perf-impacted spinner. Happy to help where possible -- my alias is magostis. Thanks!

Logs

No response

Requested priority

Normal

Products/sites affected

No response

Are you willing to submit a PR to fix?

yes

Validations

  • [X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
  • [X] The provided reproduction is a minimal reproducible example of the bug.

mgostisha avatar Sep 12 '23 20:09 mgostisha

hey @mgostisha, thanks for report ! yeah please provide those profiles.

Hotell avatar Sep 27 '23 09:09 Hotell

Here's the profiles for the v8 and v9 spinners.

Fluent v9 Spinner: https://trace.cafe/t/3FOODKrKWC Fluent v8 Spinner: https://trace.cafe/t/hNFRUbN6S6

For v9, you can see that it needs to go through the browser's calc/paint/layout steps to update the animation, whereas the v8 spinner does not.

This is exacerbated when you click on the "Do Work" button, which will emulate heavy CPU work. In the repro codesandbox, the v9 spinner is above the v8 spinner. Clicking that button will show animation jank in the v9 spinner, but not in the v8 one. Thanks for looking into this!

mgostisha avatar Sep 28 '23 15:09 mgostisha

This is due to the v9 Spinner using svg for the animation, which means that the animation can't be offloaded to the compositor. I will update the animation to not use svg so that it can leverage the compositor.

tomi-msft avatar Oct 02 '23 19:10 tomi-msft

After further investigation of this issue, it is not as straightforward to change the animation from svg to something else while maintaining the same animation frames. @mgostisha You are welcome to submit a fix and contribute to the library if you would like.

tomi-msft avatar Jan 16 '24 21:01 tomi-msft

After further investigation of this issue, it is not as straightforward to change the animation from svg to something else while maintaining the same animation frames. @mgostisha You are welcome to submit a fix and contribute to the library if you would like.

The issue itself isn't specifically that an SVG element is animating, it's that an SVG attribute is animating. SVG element animations can be compositor-only, as long the attributes being animated are compositor-only. Unfortunately, it's not very transparent or obvious which attributes are compositor-only, but this switch in Chromium's code spells them out: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/compositor_animations.cc;drc=d2b5c17eff35da6ebff8ba20c99688b87e1bc752;l=308

Since this spinner animation is essentially a rotate, it should be possible to create a compositor-only by layering SVG's and using rotate animations.

KurtCattiSchmidt avatar Feb 14 '24 18:02 KurtCattiSchmidt

Reopening as this is under active investigation again

JustSlone avatar Feb 14 '24 22:02 JustSlone

@JustSlone is there anything else you need as far information to progress forward? happy to help look at a fix, it would just be a little bit.

mgostisha avatar Feb 21 '24 15:02 mgostisha

@mgostisha We are working on a fix, with this PR: #30567

tomi-msft avatar Feb 21 '24 21:02 tomi-msft