fix(material/slider): fix track animation #25924
Conversation
wagnermaciel
commented
Nov 3, 2022
wagnermaciel
commented
- fixes an issue where the slider track active would flicker when switching between thumbs on a range slider
* fixes an issue where the slider track active would flicker when switching between thumbs on a range slider
| trackStyle.transformOrigin = styles.transformOrigin; | ||
|
|
||
| if (styles.transformOrigin !== trackStyle.transformOrigin) { | ||
| this._elementRef.nativeElement.classList.add('mat-mdc-slider-disable-track-animation'); |
There was a problem hiding this comment.
This part seems a little complicated to me. Does the issue happen when the user is dragging?
There was a problem hiding this comment.
This is kind of a gross problem so apologies for the long description
The issue happens when a user alternates clicks between the two thumbs. This causes the track to change which thumb it's animating from, which is whichever thumb is not moving. If the animation is on, there will be a flicker as this change is made. BUT we don't want to disable animations entirely because then the thumb animation will be disabled, too
Another layer of complexity is that the track "origin" changing needs to happen immediately to avoid the flicker, but the change in transform still needs to have an animation so it moves along with the thumb. This is why I'm using onStable so that once the track animation origin has been changed we can do the actual translateX animation
There was a problem hiding this comment.
This is firing quite frequently though. If I add a console.log, I can see it firing 20 times just on init and then once for each pixel I drag. Toggling the class so frequently can cause style recalculations which affect performance.
There was a problem hiding this comment.
You're right, code was bad. It still fires a lot initially, but I think it's firing for all of the sliders that are in the other tabs of the demo, too
wagnermaciel
added
the
target: rc
|
@crisbeto This is ready for another look when you're ready |
|
|
||
| if (animationOriginChanged) { | ||
| this._elementRef.nativeElement.classList.add('mat-mdc-slider-disable-track-animation'); | ||
| this._ngZone.onStable.pipe(take(1)).subscribe(() => { |
There was a problem hiding this comment.
I'm not sure whether we want to use NgZone.onStable here since we don't know if the zone is stable. Maybe a requestAnimationFrame would be more appropriate.
There was a problem hiding this comment.
I tried switching to using requestAnimationFrame but it seems to have issues with the timing causing the active tracks animation to be inconsistent with the thumb. Is there another way to know when the changes have been rendered so I can be sure to get the timing right?
There was a problem hiding this comment.
Okay, let's leave it for now. The alternatives would be a setTimeout or a Promise.resolve() which aren't much better.
| trackStyle.transformOrigin = styles.transformOrigin; | ||
|
|
||
| if (animationOriginChanged) { |
There was a problem hiding this comment.
This still fires for all the sliders on init. Can we avoid it? My understanding is that the problem only happens when the user is interacting which when it's happening.
There was a problem hiding this comment.
It's a little ugly but I got this working. The issue with the slider is that changes to properties automatically triggers UI updates. I added a boolean for checking whether the props has been initialized so that those UI updates don't get automatically triggered until the properties are done getting set up
* fix(material/slider): fix track animation * fixes an issue where the slider track active would flicker when switching between thumbs on a range slider * fixup! fix(material/slider): fix track animation * fixup! fix(material/slider): fix track animation * fixup! fix(material/slider): fix track animation * fixup! fix(material/slider): fix track animation * fixup! fix(material/slider): fix track animation * fixup! fix(material/slider): fix track animation * fixup! fix(material/slider): fix track animation (cherry picked from commit ded4bab)
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
