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.

Sign up for GitHub

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

fix(material/slider): fix track animation #25924

Merged
merged 8 commits into from
Nov 11, 2022
Merged

fix(material/slider): fix track animation #25924

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8d76750
fix(material/slider): fix track animation
wagnermaciel Nov 3, 2022
2403169
fixup! fix(material/slider): fix track animation
wagnermaciel Nov 3, 2022
70a47e4
fixup! fix(material/slider): fix track animation
wagnermaciel Nov 3, 2022
49eb3a2
fixup! fix(material/slider): fix track animation
wagnermaciel Nov 7, 2022
e1fdf6c
fixup! fix(material/slider): fix track animation
wagnermaciel Nov 7, 2022
1409cd3
fixup! fix(material/slider): fix track animation
wagnermaciel Nov 9, 2022
244c1e3
fixup! fix(material/slider): fix track animation
wagnermaciel Nov 9, 2022
f6cc42f
fixup! fix(material/slider): fix track animation
wagnermaciel Nov 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/material/slider/slider.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ $mat-slider-horizontal-margin: 8px !default;
.mdc-slider__track--active_fill {
transition-duration: 80ms;
}

&.mat-mdc-slider-disable-track-animation {
.mdc-slider__track--active_fill {
transition-duration: 0ms;
}
}
}

// We need to repeat these styles to override discrete slider styling.
Expand All @@ -69,6 +75,12 @@ $mat-slider-horizontal-margin: 8px !default;
.mdc-slider__track--active_fill {
transition-duration: 80ms;
}

&.mat-mdc-slider-disable-track-animation {
.mdc-slider__track--active_fill {
transition-duration: 0ms;
}
}
}

.mdc-slider__track,
Expand All @@ -93,7 +105,8 @@ $mat-slider-horizontal-margin: 8px !default;
background-color: var(--mat-mdc-slider-hover-ripple-color, transparent);
}

.mat-mdc-slider-focus-ripple, .mat-mdc-slider-active-ripple {
.mat-mdc-slider-focus-ripple,
.mat-mdc-slider-active-ripple {
background-color: var(--mat-mdc-slider-focus-ripple-color, transparent);
}
}
Expand Down
88 changes: 70 additions & 18 deletions src/material/slider/slider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import {
} from '@angular/material/core';
import {ANIMATION_MODULE_TYPE} from '@angular/platform-browser/animations';
import {Subscription} from 'rxjs';
import {take} from 'rxjs/operators';
import {
_MatThumb,
_MatTickMark,
Expand Down Expand Up @@ -388,6 +389,8 @@ export class MatSlider
/** Whether the slider is rtl. */
_isRtl: boolean = false;

private _hasViewInitialized: boolean = false;

/**
* The width of the tick mark track.
* The tick mark track width is different from full track width
Expand Down Expand Up @@ -425,6 +428,7 @@ export class MatSlider
if (this._platform.isBrowser) {
this._updateDimensions();
}

const eInput = this._getInput(_MatThumb.END);
const sInput = this._getInput(_MatThumb.START);
this._isRange = !!eInput && !!sInput;
Expand All @@ -439,30 +443,42 @@ export class MatSlider

const thumb = this._getThumb(_MatThumb.END);
this._rippleRadius = thumb._ripple.radius;

this._inputPadding = this._rippleRadius - this._knobRadius;
this._inputOffset = this._knobRadius;

if (eInput) {
eInput.initProps();
eInput.initUI();
}
if (sInput) {
sInput.initProps();
sInput.initUI();
}
if (this._isRange) {
(eInput as _MatSliderRangeThumb)._updateMinMax();
(sInput as _MatSliderRangeThumb)._updateMinMax();
}
this._updateTrackUI(eInput!);
this._updateTickMarkUI();
this._updateTickMarkTrackUI();
this._isRange
? this._initUIRange(eInput as _MatSliderRangeThumb, sInput as _MatSliderRangeThumb)
: this._initUINonRange(eInput!);

this._observeHostResize();
this._cdr.detectChanges();
}

private _initUINonRange(eInput: _MatSliderThumb): void {
eInput.initProps();
eInput.initUI();

this._updateValueIndicatorUI(eInput);

this._hasViewInitialized = true;
}

private _initUIRange(eInput: _MatSliderRangeThumb, sInput: _MatSliderRangeThumb): void {
eInput.initProps();
eInput.initUI();

sInput.initProps();
sInput.initUI();

eInput._updateMinMax();
sInput._updateMinMax();

this._updateValueIndicatorUI(eInput);
this._updateValueIndicatorUI(sInput);

this._hasViewInitialized = true;
}

ngOnDestroy(): void {
this._dirChangeSubscription.unsubscribe();
this._resizeObserver?.disconnect();
Expand Down Expand Up @@ -555,10 +571,22 @@ export class MatSlider
transformOrigin: string;
}): void {
const trackStyle = this._trackActive.nativeElement.style;
const animationOriginChanged =
styles.left !== trackStyle.left && styles.right !== trackStyle.right;

trackStyle.left = styles.left;
trackStyle.right = styles.right;
trackStyle.transform = styles.transform;
trackStyle.transformOrigin = styles.transformOrigin;

if (animationOriginChanged) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

this._elementRef.nativeElement.classList.add('mat-mdc-slider-disable-track-animation');
Copy link
Member

Choose a reason for hiding this comment

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

This part seems a little complicated to me. Does the issue happen when the user is dragging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

this._ngZone.onStable.pipe(take(1)).subscribe(() => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, let's leave it for now. The alternatives would be a setTimeout or a Promise.resolve() which aren't much better.

this._elementRef.nativeElement.classList.remove('mat-mdc-slider-disable-track-animation');
trackStyle.transform = styles.transform;
});
} else {
trackStyle.transform = styles.transform;
}
}

/** Returns the translateX positioning for a tick mark based on it's index. */
Expand All @@ -571,6 +599,10 @@ export class MatSlider
// Handlers for updating the slider ui.

_onTranslateXChange(source: _MatSliderThumb): void {
if (!this._hasViewInitialized) {
return;
}

this._updateThumbUI(source);
this._updateTrackUI(source);
this._updateOverlappingThumbUI(source as _MatSliderRangeThumb);
Expand All @@ -580,23 +612,39 @@ export class MatSlider
input1: _MatSliderRangeThumb,
input2: _MatSliderRangeThumb,
): void {
if (!this._hasViewInitialized) {
return;
}

input1._updateThumbUIByValue();
input2._updateThumbUIByValue();
}

_onValueChange(source: _MatSliderThumb): void {
if (!this._hasViewInitialized) {
return;
}

this._updateValueIndicatorUI(source);
this._updateTickMarkUI();
this._cdr.detectChanges();
}

_onMinMaxOrStepChange(): void {
if (!this._hasViewInitialized) {
return;
}

this._updateTickMarkUI();
this._updateTickMarkTrackUI();
this._cdr.markForCheck();
}

_onResize(): void {
if (!this._hasViewInitialized) {
return;
}

this._updateDimensions();
if (this._isRange) {
const eInput = this._getInput(_MatThumb.END) as _MatSliderRangeThumb;
Expand Down Expand Up @@ -693,7 +741,11 @@ export class MatSlider
}

const valuetext = this.displayWith(source.value);
source._valuetext = valuetext;

if (this._hasViewInitialized) {
source._valuetext = valuetext;
}

if (this.discrete) {
source.thumbPosition === _MatThumb.START
? (this.startValueIndicatorText = valuetext)
Expand Down