Skip to content

Commit

Permalink
fix(material/slider): don't interrupt pointer dragging when keyboard …
Browse files Browse the repository at this point in the history
…is pressed (#22849)

The slider is currently set up to "slide" either with the keyboard or the pointer, but we don't dinstinguish between the two. This means that when a `keyup` fires, it'll interrupt pointer scrolling as well.

These changes add some logic to prevent the two from interfering with each other.

Fixes #22719.
  • Loading branch information
crisbeto committed Jun 2, 2021
1 parent 3aa65ba commit 23dfbbb
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 10 deletions.
23 changes: 23 additions & 0 deletions src/material/slider/slider.spec.ts
Expand Up @@ -9,6 +9,7 @@ import {
PAGE_UP,
RIGHT_ARROW,
UP_ARROW,
A,
} from '@angular/cdk/keycodes';
import {
createMouseEvent,
Expand Down Expand Up @@ -139,6 +140,28 @@ describe('MatSlider', () => {
expect(sliderNativeElement.classList).not.toContain('mat-slider-sliding');
});

it('should not interrupt sliding by pressing a key', () => {
expect(sliderNativeElement.classList).not.toContain('mat-slider-sliding');

dispatchSlideStartEvent(sliderNativeElement, 0);
fixture.detectChanges();

expect(sliderNativeElement.classList).toContain('mat-slider-sliding');

// Any key code will do here. Use A since it isn't associated with other actions.
dispatchKeyboardEvent(sliderNativeElement, 'keydown', A);
fixture.detectChanges();
dispatchKeyboardEvent(sliderNativeElement, 'keyup', A);
fixture.detectChanges();

expect(sliderNativeElement.classList).toContain('mat-slider-sliding');

dispatchSlideEndEvent(sliderNativeElement, 0.34);
fixture.detectChanges();

expect(sliderNativeElement.classList).not.toContain('mat-slider-sliding');
});

it('should stop dragging if the page loses focus', () => {
const classlist = sliderNativeElement.classList;

Expand Down
21 changes: 12 additions & 9 deletions src/material/slider/slider.ts
Expand Up @@ -326,10 +326,10 @@ export class MatSlider extends _MatSliderBase
private _percent: number = 0;

/**
* Whether or not the thumb is sliding.
* Whether or not the thumb is sliding and what the user is using to slide it with.
* Used to determine if there should be a transition for the thumb and fill track.
*/
_isSliding: boolean = false;
_isSliding: 'keyboard' | 'pointer' | null = null;

/**
* Whether or not the slider is active (clicked or sliding).
Expand Down Expand Up @@ -569,7 +569,8 @@ export class MatSlider extends _MatSliderBase
}

_onKeydown(event: KeyboardEvent) {
if (this.disabled || hasModifierKey(event)) {
if (this.disabled || hasModifierKey(event) ||
(this._isSliding && this._isSliding !== 'keyboard')) {
return;
}

Expand Down Expand Up @@ -619,12 +620,14 @@ export class MatSlider extends _MatSliderBase
this._emitChangeEvent();
}

this._isSliding = true;
this._isSliding = 'keyboard';
event.preventDefault();
}

_onKeyup() {
this._isSliding = false;
if (this._isSliding === 'keyboard') {
this._isSliding = null;
}
}

/** Called when the user has put their pointer down on the slider. */
Expand All @@ -642,7 +645,7 @@ export class MatSlider extends _MatSliderBase

if (pointerPosition) {
const oldValue = this.value;
this._isSliding = true;
this._isSliding = 'pointer';
this._lastPointerEvent = event;
event.preventDefault();
this._focusHostElement();
Expand All @@ -665,7 +668,7 @@ export class MatSlider extends _MatSliderBase
* starting to drag. Bound on the document level.
*/
private _pointerMove = (event: TouchEvent | MouseEvent) => {
if (this._isSliding) {
if (this._isSliding === 'pointer') {
const pointerPosition = getPointerPositionOnPage(event, this._touchId);

if (pointerPosition) {
Expand All @@ -685,14 +688,14 @@ export class MatSlider extends _MatSliderBase

/** Called when the user has lifted their pointer. Bound on the document level. */
private _pointerUp = (event: TouchEvent | MouseEvent) => {
if (this._isSliding) {
if (this._isSliding === 'pointer') {
if (!isTouchEvent(event) || typeof this._touchId !== 'number' ||
// Note that we use `changedTouches`, rather than `touches` because it
// seems like in most cases `touches` is empty for `touchend` events.
findMatchingTouch(event.changedTouches, this._touchId)) {
event.preventDefault();
this._removeGlobalEvents();
this._isSliding = false;
this._isSliding = null;
this._touchId = undefined;

if (this._valueOnSlideStart != this.value && !this.disabled) {
Expand Down
2 changes: 1 addition & 1 deletion tools/public_api_guard/material/slider.d.ts
Expand Up @@ -4,7 +4,7 @@ export declare class MatSlider extends _MatSliderBase implements ControlValueAcc
_animationMode?: string | undefined;
protected _document: Document;
_isActive: boolean;
_isSliding: boolean;
_isSliding: 'keyboard' | 'pointer' | null;
readonly change: EventEmitter<MatSliderChange>;
get displayValue(): string | number;
displayWith: (value: number) => string | number;
Expand Down

0 comments on commit 23dfbbb

Please sign in to comment.