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

ToggleGroupControl: Improve backdrop component rendering #37507

Conversation

KevinBatdorf
Copy link
Contributor

Description

This changes the render implementation for toggle control backdrop. See here for issue report (including video): #37506

This implementation will poll the dom for a short time and try to determine if the element is in an animating state (by checking for sub pixel positioning). It gives up polling after 150ms and renders in the case the element happens to land on a sub pixel. The 150 is safer than 100 as a popover, for example has a 100ms animation duration time, then the browser needs about 16 ticks to grab the next frame before a CSS update.

Closes #37506

How has this been tested?

I tested it manually as in the video below.

Screenshots

Screen.Recording.2021-12-17.at.9.49.21.PM.mov

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

Sorry, something went wrong.

@KevinBatdorf
Copy link
Contributor Author

KevinBatdorf commented Dec 18, 2021

I don't believe the failing tests are related to this PR - edit: they passed when I ran it again

@mirka mirka added the [Package] Components /packages/components label Dec 21, 2021
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.

Hey @KevinBatdorf , thank you for working on this!

I had a look at the code left a few inline comments. Would you mind also adding a CHANGELOG entry ?


Overall, the implementation seems still quite brittle to me, as it's still relying on polling, magic numbers (like 150ms), and subpixel positioning. What if the animation duration of the popover changes? Or what if, by chance, a value mid-animation is an exact integer? I am also not a fan of the 150ms delay that the backdrop now has after an interaction — it makes the UI feel not responsive.

In my opinion, we should re-think our approach and come up with a more robust and less hacky solution, e.g:

  • we should rely on requestAnimationFrame rendering loops, instead of setInterval
  • we should try to measure the target x and width in a way that is relative to the backdrop's parent and unaffected by external factors (like the popover's transform). For example,getBoundingClientRect() keeps transforms into account, which is not ideal in this scenario.
  • what if we used CSS to set the position of the backdrop to the final position that we wanted, and then used JS to animated it from its current state to the final position? (taking inspiration from the FLIP technique)

Finally, I appreciate that we may want work on a new approach it in a follow-up PR, and still go ahead with the changes in this PR if they fix a bug.

Curious to hear what @mirka, @diegohaz and @ntsekouras think about this too.

@ciampo ciampo added the [Feature] Navigation Component A navigational waterfall component for hierarchy of items. label Jan 6, 2022
@ciampo ciampo changed the title Improve backdrop component rendering ToggleGroupControl: Improve backdrop component rendering Jan 6, 2022
@KevinBatdorf
Copy link
Contributor Author

hey @ciampo Thanks for the follow up and feedback!

Overall, the implementation seems still quite brittle to me, as it's still relying on polling, magic numbers (like 150ms), and subpixel positioning. What if the animation duration of the popover changes? Or what if, by chance, a value mid-animation is an exact integer? I am also not a fan of the 150ms delay that the backdrop now has after an interaction — it makes the UI feel not responsive.

I agree it's fragile. The animation of the popover, if that changes, shouldn't cause an issue since it's in a setInterval (that's the purpose of the interval), but there is a small chance the animation lands on a whole value when the DOM is polled. I don't know how likely though (Not saying it isn't likely, I just don't know the odds).

I am also not a fan of the 150ms delay that the backdrop now has after an interaction

This I think i can optimize. Do the first check on mount for the case when the parent isn't transitioning.

For some context, this was a PR for our FFTF last month and a lot of the details have left my brain already. I need to jump back in and refresh myself on the implementation choices I made and why. Some notes come to mind though:

  1. Under some circumstance you can't accurately get the position in a transitioning element, so getting the global position is necessary. Maybe?
  2. Waiting on an animation frame will take longer (I think 16 ticks on average vs 1?) and may cause more jittering. polling getBoundingClientRect will cause reflow, but given the nature of this code it may be desired over waiting for the next render? However, it is using a RAF when it eventually sets the left position (line 59), since it's to animate after that.

what if we used CSS to set the position of the backdrop to the final position

I think this is the main issue here. I don't think we can use CSS reliably in this case. I think the options are to not use the backdrop component at all, or try to nail the timing in a performance-friendly way..

Finally, I appreciate that we may want work on a new approach it in a follow-up PR, and still go ahead with the changes in this PR if they fix a bug.

I do think this PR is better than what's currently there, but. agree it's not the final implementation.

I'll try to jump back in after work tonight and follow up with some thoughts, and address the inline code suggestions you made.

Thanks again!

@ciampo
Copy link
Contributor

ciampo commented Jan 9, 2022

The animation of the popover, if that changes, shouldn't cause an issue since it's in a setInterval (that's the purpose of the interval)

An example that comes to mind is — what if the popover animation gets updated to last longer than 150ms? The current technique of polling for a predefined amount of time just doesn't seem like the best way of solving this problem

there is a small chance the animation lands on a whole value when the DOM is polled. I don't know how likely though (Not saying it isn't likely, I just don't know the odds).

Even with low odds, I still think that this doesn't look like an optimal solution. We should aim for a solution that doesn't rely on chance and differences in browser sub-pixel rendering.

but there is a small chance the animation lands on a whole value when the DOM is polled. I don't know how likely though (Not saying it isn't likely, I just don't know the odds).

For some context, this was a PR for our FFTF last month and a lot of the details have left my brain already. I need to jump back in and refresh myself on the implementation choices I made and why.

I definitely understand, and apologies if I may be coming across as too strict. Again, I do understand the value of this PR in fixing a very annoying glitch — I just want to make sure that, if we put out the fixes from this PR, we also look at a more solid fix that can be worked as a follow-up.

Under some circumstance you can't accurately get the position in a transitioning element, so getting the global position is necessary. Maybe?

What do you mean by "global position"? Do you mean the x and y relative to the whole viewport?

Waiting on an animation frame will take longer (I think 16 ticks on average vs 1?) and may cause more jittering. polling getBoundingClientRect will cause reflow, but given the nature of this code it may be desired over waiting for the next render? However, it is using a RAF when it eventually sets the left position (line 59), since it's to animate after that.

An animation frame lasts for a different amount of time depending on the system's refresh rate. With 60fps, that amounts to 16ms, with 120fps to 8ms and so on. It shouldn't really cause jittering, because the system would call the RAF's callback every time it's ready to paint the next frame on screen. Although it's true that calling getBoundingClientRect will cause a reflow every time it's called.

Instead of using getBoundingClientRect().{x,y}, have we considered using offsetLeft and offsetTop, which should be returning the offset with respect to the first positioned parent? This way there's a chance that the offset doesn't depend on external factors like a popover animation, since the positioning for each item would be all relative to the parent list. This potentially would remove the need for the interval polling?

I'll try to jump back in after work tonight and follow up with some thoughts, and address the inline code suggestions you made.

Thank you!

@diegohaz
Copy link
Member

After reading #37506, I understand that the issue is actually that ToggleGroupControl is animating on the first render/initial state when it shouldn't?

Isn't a better/simpler fix to prevent it from animating altogether on the initial state or guarantee that we're in fact setting the correct initial state?

@KevinBatdorf
Copy link
Contributor Author

Got behind last week but will open it back up tonight (for real this time!).

If I remember correctly the issue is that you cannot obtain the relative position until the parent transition animation stops, and the backdrop component doesn't even render until after the parent animation already has begun. So the solution will be either to not have the fancy backdrop animation, or include a hack of some sort that polls for when its safe to update.

But I'll explore some of the ideas above and see if there's something else.

@diegohaz
Copy link
Member

If the animation is done via CSS animation/transition, maybe we can also use the animationend/transitionend events.

@KevinBatdorf
Copy link
Contributor Author

So the issue was the parent animating as I remembered. You can't get the x,y position of the target node until that finishes, so we have to wait.

@diegohaz's suggestion to watch the transition end led me to a new implementation idea though (thanks!). This is still a little hacky but definitely much better.

How it works now is when it first renders it will align itself to the value of the container left position . This will trigger an animation that we can wait for to end. This is transition count 1.

Then after that transition completes it will compute the target node's actual x,y position, which is now available. It will then fire another transition. This is transition count 2.

The count is important IF we want to avoid the opening animation from occurring. This works by setting the backdrop's visibility property to hidden until transition count 2 happens. Here's how that looks (the visibility approach):

hidden.mov

It's not perfect as there's a noticeable delay.

Without keeping track of the count, it will show like this (the animating approach):

animating.mov

Personally, I don't know which is better. Maybe the visibility approach?

If someone wants to review it again I can probably spend some time on any requested changes again asap. Thanks everyone for looking at it.

@KevinBatdorf
Copy link
Contributor Author

Also, if this is something worth rushing, feel free to take over the PR too. I want to start contributing more regularly but my time is still a bit limited at the moment.

@diegohaz
Copy link
Member

I see. The backdrop needs to check the DOM in order to be positioned. Can we use the element background by default (so we don't need to dynamically get the element's position) and only use the backdrop element when it's animating? And, of course, make sure the initial state is not animated.

@ciampo
Copy link
Contributor

ciampo commented Jan 13, 2022

Just in case it got lost in my longer message before:

Instead of using getBoundingClientRect().{x,y}, have we considered using offsetLeft and offsetTop, which should be returning the offset with respect to the first positioned parent? This way there's a chance that the offset doesn't depend on external factors like a popover animation, since the positioning for each item would be all relative to the parent list. This potentially would remove the need for the interval polling?

Also, if this is something worth rushing, feel free to take over the PR too. I want to start contributing more regularly but my time is still a bit limited at the moment.

I'm also currently a bit constrained with my time, not sure I'd be able to take over this PR at the moment.

Maybe a more sensible approach would be to:

  • open a PR where we remove the animating/moving backdrop and replace it with each toggle option changing its own background when selected/unselected. This would allow us to fix the bug quickly
  • separately, investigate a better way of handling the animating/moving backdrop without the pressure of having to come up with a solution quickly to fix the bug

What do folks think?

@KevinBatdorf
Copy link
Contributor Author

Instead of using getBoundingClientRect().{x,y}, have we considered using offsetLeft and offsetTop

The issue here is these values aren't available until the parent animation has completed.

open a PR where we remove the animating/moving backdrop and replace it with each toggle option changing its own background when selected/unselected. This would allow us to fix the bug quickly

I do think this is the better option, but we'd lose the fancy backdrop animation.

I see. The backdrop needs to check the DOM in order to be positioned. Can we use the element background by default (so we don't need to dynamically get the element's position) and only use the backdrop element when it's animating? And, of course, make sure the initial state is not animated.

If I'm reading this correctly, I think you're suggesting to apply a static button BG color on load, then remove it when the backdrop is ready? That might be worth experimenting with.

I'll have time today to look into this some more. And more time over the weekend as well.

@ciampo
Copy link
Contributor

ciampo commented Jan 13, 2022

The issue here is these values aren't available until the parent animation has completed.

Mhh, I was hoping that offsetLeft and offsetTop wouldn't be affected by transitions etc. and that the ToggleGroupControl could "take measurements" for where the backdrop needs to animate to independently from the popover animation. But maybe I'm missing something

I do think this is the better option, but we'd lose the fancy backdrop animation.

Yes, but it would be a temporary loss, with the goal of re-adding it soon enough once we've figured out a better way.

I'll have time today to look into this some more. And more time over the weekend as well.

In case we decided to go with the solution above, would you be able to open a new PR with this temporary fix?

@KevinBatdorf
Copy link
Contributor Author

In case we decided to go with the solution above, would you be able to open a new PR with this temporary fix?

Yes, I'll give this PR another shot with an attempt to save the backdrop animation. Otherwise I'll prepare a new PR without the backdrop

@KevinBatdorf
Copy link
Contributor Author

This seems to work much better with the target bg color set temporarily until it's ready to animate.

Screen.Recording.2022-01-14.at.3.08.39.AM.mov

Take a look and let me know. If it's not up to par still I can just apply the bg color directly on press without any animations. Personally, I'm happy with this!

@ciampo
Copy link
Contributor

ciampo commented Jan 14, 2022

Hey @KevinBatdorf , thank you for iterating on this 🙇 Your approach was actually quite clever!

I also gave this a try and while it worked initially, but unfortunately it didn't work very well once I tested it with a throttled CPU:

HaA8jy.mp4

Overall, this implementation still seems very brittle to me (it relies on counting animations, it sets inline styles..), and the code is quite hard to understand for someone not familiar with this approach.

If it's not up to par still I can just apply the bg color directly on press without any animations.

Yes, I would suggest we go with the plan highlighted above:

Maybe a more sensible approach would be to:

  • open a PR where we remove the animating/moving backdrop and replace it with each toggle option changing its own background when selected/unselected. This would allow us to fix the bug quickly
  • separately, investigate a better way of handling the animating/moving backdrop without the pressure of having to come up with a solution quickly to fix the bug

Thank you for your patience!

@KevinBatdorf
Copy link
Contributor Author

I also gave this a try and while it worked initially, but unfortunately it didn't work very well once I tested it with a throttled CPU:

Are you sure the build didn't fail? Even without throttling you're seeing the behavior this PR is fixing. I can't reproduce it with 6x throttling either no matter how many times I try.

I agree it's still is a bit hacky though regardless. I'll go ahead and work on a new implementation without the backdrop animation over the weekend.

--

Your approach was actually quite clever!

Thanks for the kind words! I was inspired by all the very helpful input above.

I just added another tweak to wait for another frame before showing the backdrop. It's a bit smoother. I'll leave it here just for future reference just in case someone comes back to it.

@KevinBatdorf
Copy link
Contributor Author

KevinBatdorf commented Jan 15, 2022

open a PR where we remove the animating/moving backdrop and replace it with each toggle option changing its own background when selected/unselected. This would allow us to fix the bug quickly

Added here: #38008

@ciampo
Copy link
Contributor

ciampo commented Jan 19, 2022

I'm closing this PR in favour of #38008, which I believe represents a better short-term solution to fix #37506.

We can investigate a better, more reliable way to re-introduce an animated backdrop to ToggleGroupControl separately.

@ciampo ciampo closed this Jan 19, 2022
@KevinBatdorf KevinBatdorf deleted the fix/improve-group-control-backfrop-render-animation branch April 7, 2022 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Navigation Component A navigational waterfall component for hierarchy of items. [Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Color select component backdrop render and animation opportunities
4 participants