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

[core] Remove opinionated will-change usage #14036

Merged
merged 3 commits into from
Jan 1, 2019

Conversation

joshwooding
Copy link
Member

@joshwooding joshwooding commented Dec 30, 2018

Closes #10831

Affects the following components:

  • Slider
  • ToggleButton
  • CircularProgress
  • Fade
  • LinearProgress
  • TabIndicator
  • Zoom

I'm not sure what component prefix should be added to the title but, I didn't list all the components to improve the readability

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Important: will-change is intended to be used as a last resort, in order to try to deal with existing performance problems. It should not be used to anticipate performance problems.
-- will-change | MDN

It's probably safe to say that this property should only be used at the application level and not component level i.e. application authors should add this not library authors.

Maybe add a recommendation to the docs if users experience performance issues?

@eps1lon
Copy link
Member

eps1lon commented Dec 30, 2018

Argos picked up some very small diffs on the shadow of the speed dial it seems. Visual regressions might be due to

Be aware, that will-change may actually influence the visual appearance of elements, when used with property values, that create a stacking context (e.g. will-change: opacity), as the stacking context is created up front.
-- will-change | MDN

@joshwooding
Copy link
Member Author

Maybe add a recommendation to the docs if users experience performance issues?

I mentioned this to @oliviertassinari, maybe there should be an Optimization section under Guides?

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 30, 2018

I have resolved the conflict. The will change thing is such a broad topic that I'm unsure it's our role to evangelize it. I try to think of a place where people could find it. Maybe if we had a performance guide documentation page?

@oliviertassinari oliviertassinari changed the title Remove opinionated will-change usage [core] Remove opinionated will-change usage Dec 30, 2018
@@ -43,6 +43,7 @@ export const styles = theme => ({
width: '100%',
animation: 'buffer 3s infinite linear',
animationName: '$buffer',
willChange: 'opacity, background-position',
Copy link
Member

Choose a reason for hiding this comment

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

So how do we determine when to use it? "Opinionated" is to vague IMO. I thought we simply remove every usage.

Copy link
Member

Choose a reason for hiding this comment

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

So far, we use it for all the continuously running animations. Does it sound correct?

Copy link
Member

Choose a reason for hiding this comment

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

What does "continuously running" entail? will-change got removed in Slider too which would fit that description.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should remove it everywhere then?

Copy link
Member

Choose a reason for hiding this comment

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

I think so too after reading that documentation page. It's a "last resort" and should not be applied prematurely. I would hope that a single component hits 60fps anyway which is why application authors need to benchmark their ui not library authors a single component WRT to will-change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Slider only has transformations when active, The indeterminate progress components have animations that continuously loop

Copy link
Contributor

Choose a reason for hiding this comment

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

Running animations are promoted to a layer automatically and thus does not need will-change. Will-change is, as it's name implies for things that are not currently changing but is about to so that the animation doesn't have to start with a costly layer promotion.

Copy link
Member

@oliviertassinari oliviertassinari Dec 31, 2018

Choose a reason for hiding this comment

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

@joshwooding What do you think of removing all the will-change occurrences?

For the slider, we could apply a will-change ahead of time, based on the hover state. But it's quite an advanced logic. Does it worth the bundle size overhead?

Copy link
Member Author

@joshwooding joshwooding Dec 31, 2018

Choose a reason for hiding this comment

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

@oliviertassinari I'm fine with that, users can add them back in if necessary, I was thinking the same for the slider but it's hard to tell if it will make a difference without a benchmark

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants