Skip to content

Commit 84f5def

Browse files
thetaPCthetaPC
authored
refactor(range): update value on touchEnd or drag (#29005)
Issue number: resolves #28487 --------- <!-- Please do not submit updates to dependencies unless it fixes an issue. --> <!-- Please try to limit your pull request to one type (bugfix, feature, etc). Submit multiple pull requests if needed. --> ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> There are two behaviors that need to be addressed. 1. The range value is updated when the gesture `onStart` event is fired. This can lead to the values being accidentally updated when the user is scrolling on the view. The user might tap on the range to scroll on the view, but the range value is updated instead. 2. The component prevents the view from scrolling while the user has touched any part of the range. The user might want to scroll and they happen to touch the range. This can lead to the user feeling disoriented because they can't scroll on the view anymore. These behaviors do not follow the native behavior of mobile devices. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> - The range value is updated on touch end or when the knob is being dragged. - The view can be scrolled while the user is not dragging the knob. - A new variable `isScrollingView` is used to determine if the user is scrolling on the view regardless of whether the user is dragging the knob or not. This determines what logic to apply. - The `pressedKnob` variable is no longer being set in the `onStart` event. It is now being set in the `onMove` and `onEnd` events. (the reason behind this can be found within the newly added comments) - The `initialContentScrollY` variable is no longer being set in the `onStart` event. It is now being set in the `onMove` event. (the reason behind this can be found within the newly added comments) I did not change the behavior of the range when the user is dragging the knob. The view should not scroll while the user is dragging the knob. ## Does this introduce a breaking change? - [ ] Yes - [x] No <!-- If this introduces a breaking change: 1. Describe the impact and migration path for existing applications below. 2. Update the BREAKING.md file with the breaking change. 3. Add "BREAKING CHANGE: [...]" to the commit description when merging. See https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer for more information. --> ## Other information <!-- Any other information that is important to this PR such as screenshots of how the component looks before and after the change. --> The new behavior aligns with the native mobile devices.
1 parent b2d636f commit 84f5def

File tree

2 files changed

+169
-26
lines changed

2 files changed

+169
-26
lines changed

‎core/src/components/range/range.tsx‎

Lines changed: 136 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -299,8 +299,14 @@ export class Range implements ComponentInterface {
299299
el: rangeSlider,
300300
gestureName: 'range',
301301
gesturePriority: 100,
302-
threshold: 0,
303-
onStart: (ev) => this.onStart(ev),
302+
/**
303+
* Provide a threshold since the drag movement
304+
* might be a user scrolling the view.
305+
* If this is true, then the range
306+
* should not move.
307+
*/
308+
threshold: 10,
309+
onStart: () => this.onStart(),
304310
onMove: (ev) => this.onMove(ev),
305311
onEnd: (ev) => this.onEnd(ev),
306312
});
@@ -418,42 +424,101 @@ export class Range implements ComponentInterface {
418424
this.ionChange.emit({ value: this.value });
419425
}
420426

421-
private onStart(detail: GestureDetail) {
422-
const { contentEl } = this;
423-
if (contentEl) {
424-
this.initialContentScrollY = disableContentScrollY(contentEl);
425-
}
427+
/**
428+
* The value should be updated on touch end or
429+
* when the component is being dragged.
430+
* This follows the native behavior of mobile devices.
431+
*
432+
* For example: When the user lifts their finger from the
433+
* screen after tapping the bar or dragging the bar or knob.
434+
*/
435+
private onStart() {
436+
this.ionKnobMoveStart.emit({ value: this.ensureValueInBounds(this.value) });
437+
}
426438

427-
const rect = (this.rect = this.rangeSlider!.getBoundingClientRect() as any);
439+
/**
440+
* The value should be updated while dragging the
441+
* bar or knob.
442+
*
443+
* While the user is dragging, the view
444+
* should not scroll. This is to prevent the user from
445+
* feeling disoriented while dragging.
446+
*
447+
* The user can scroll on the view if the knob or
448+
* bar is not being dragged.
449+
*
450+
* @param detail The details of the gesture event.
451+
*/
452+
private onMove(detail: GestureDetail) {
453+
const { contentEl, pressedKnob } = this;
428454
const currentX = detail.currentX;
429455

430-
// figure out which knob they started closer to
431-
let ratio = clamp(0, (currentX - rect.left) / rect.width, 1);
432-
if (isRTL(this.el)) {
433-
ratio = 1 - ratio;
456+
/**
457+
* Since the user is dragging on the bar or knob, the view should not scroll.
458+
*
459+
* This only needs to be done once.
460+
*/
461+
if (contentEl && this.initialContentScrollY === undefined) {
462+
this.initialContentScrollY = disableContentScrollY(contentEl);
434463
}
435464

436-
this.pressedKnob = !this.dualKnobs || Math.abs(this.ratioA - ratio) < Math.abs(this.ratioB - ratio) ? 'A' : 'B';
437-
438-
this.setFocus(this.pressedKnob);
465+
/**
466+
* The `pressedKnob` can be undefined if the user just
467+
* started dragging the knob.
468+
*
469+
* This is necessary to determine which knob the user is dragging,
470+
* especially when it's a dual knob.
471+
* Plus, it determines when to apply certain styles.
472+
*
473+
* This only needs to be done once since the knob won't change
474+
* while the user is dragging.
475+
*/
476+
if (pressedKnob === undefined) {
477+
this.setPressedKnob(currentX);
478+
}
439479

440-
// update the active knob's position
441480
this.update(currentX);
442-
443-
this.ionKnobMoveStart.emit({ value: this.ensureValueInBounds(this.value) });
444481
}
445482

446-
private onMove(detail: GestureDetail) {
447-
this.update(detail.currentX);
448-
}
449-
450-
private onEnd(detail: GestureDetail) {
483+
/**
484+
* The value should be updated on touch end:
485+
* - When the user lifts their finger from the screen after
486+
* tapping the bar.
487+
*
488+
* @param detail The details of the gesture or mouse event.
489+
*/
490+
private onEnd(detail: GestureDetail | MouseEvent) {
451491
const { contentEl, initialContentScrollY } = this;
452-
if (contentEl) {
492+
const currentX = (detail as GestureDetail).currentX || (detail as MouseEvent).clientX;
493+
494+
/**
495+
* The `pressedKnob` can be undefined if the user never
496+
* dragged the knob. They just tapped on the bar.
497+
*
498+
* This is necessary to determine which knob the user is changing,
499+
* especially when it's a dual knob.
500+
* Plus, it determines when to apply certain styles.
501+
*/
502+
if (this.pressedKnob === undefined) {
503+
this.setPressedKnob(currentX);
504+
}
505+
506+
/**
507+
* The user is no longer dragging the bar or
508+
* knob (if they were dragging it).
509+
*
510+
* The user can now scroll on the view in the next gesture event.
511+
*/
512+
if (contentEl && initialContentScrollY !== undefined) {
453513
resetContentScrollY(contentEl, initialContentScrollY);
454514
}
455515

456-
this.update(detail.currentX);
516+
// update the active knob's position
517+
this.update(currentX);
518+
/**
519+
* Reset the pressed knob to undefined since the user
520+
* may start dragging a different knob in the next gesture event.
521+
*/
457522
this.pressedKnob = undefined;
458523

459524
this.emitValueChange();
@@ -485,6 +550,19 @@ export class Range implements ComponentInterface {
485550
this.updateValue();
486551
}
487552

553+
private setPressedKnob(currentX: number) {
554+
const rect = (this.rect = this.rangeSlider!.getBoundingClientRect() as any);
555+
556+
// figure out which knob they started closer to
557+
let ratio = clamp(0, (currentX - rect.left) / rect.width, 1);
558+
if (isRTL(this.el)) {
559+
ratio = 1 - ratio;
560+
}
561+
this.pressedKnob = !this.dualKnobs || Math.abs(this.ratioA - ratio) < Math.abs(this.ratioB - ratio) ? 'A' : 'B';
562+
563+
this.setFocus(this.pressedKnob);
564+
}
565+
488566
private get valA() {
489567
return ratioToValue(this.ratioA, this.min, this.max, this.step);
490568
}
@@ -799,7 +877,39 @@ Developers can dismiss this warning by removing their usage of the "legacy" prop
799877
}
800878

801879
return (
802-
<div class="range-slider" ref={(rangeEl) => (this.rangeSlider = rangeEl)}>
880+
<div
881+
class="range-slider"
882+
ref={(rangeEl) => (this.rangeSlider = rangeEl)}
883+
/**
884+
* Since the gesture has a threshold, the value
885+
* won't change until the user has dragged past
886+
* the threshold. This is to prevent the range
887+
* from moving when the user is scrolling.
888+
*
889+
* This results in the value not being updated
890+
* and the event emitters not being triggered
891+
* if the user taps on the range. This is why
892+
* we need to listen for the "pointerUp" event.
893+
*/
894+
onPointerUp={(ev: PointerEvent) => {
895+
/**
896+
* If the user drags the knob on the web
897+
* version (does not occur on mobile),
898+
* the "pointerUp" event will be triggered
899+
* along with the gesture's events.
900+
* This leads to duplicate events.
901+
*
902+
* By checking if the pressedKnob is undefined,
903+
* we can determine if the "pointerUp" event was
904+
* triggered by a tap or a drag. If it was
905+
* dragged, the pressedKnob will be defined.
906+
*/
907+
if (this.pressedKnob === undefined) {
908+
this.onStart();
909+
this.onEnd(ev);
910+
}
911+
}}
912+
>
803913
{ticks.map((tick) => (
804914
<div
805915
style={tickStyle(tick)}

‎core/src/components/range/test/range-events.e2e.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,39 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
6767
expect(rangeEnd).toHaveReceivedEventDetail({ value: 21 });
6868
});
6969

70+
test('should emit end event on tap', async ({ page }, testInfo) => {
71+
testInfo.annotations.push({
72+
type: 'issue',
73+
description: 'https://github.com/ionic-team/ionic-framework/issues/28487',
74+
});
75+
76+
await page.setContent(`<ion-range aria-label="Range" value="20"></ion-range>`, config);
77+
78+
const range = page.locator('ion-range');
79+
const rangeEndSpy = await page.spyOnEvent('ionKnobMoveEnd');
80+
const rangeBoundingBox = await range.boundingBox();
81+
/**
82+
* Coordinates for the click event.
83+
* These need to be near the end of the range
84+
* (or anything that isn't the current value).
85+
*
86+
* The number 50 is arbitrary, but it should be
87+
* less than the width of the range.
88+
*/
89+
const x = rangeBoundingBox!.width - 50;
90+
// The y coordinate is the middle of the range.
91+
const y = rangeBoundingBox!.height / 2;
92+
93+
// Click near the end of the range.
94+
await range.click({
95+
position: { x, y },
96+
});
97+
98+
await rangeEndSpy.next();
99+
100+
expect(rangeEndSpy.length).toBe(1);
101+
});
102+
70103
// TODO FW-2873
71104
test.skip('should not scroll when the knob is swiped', async ({ page, skip }) => {
72105
skip.browser('webkit', 'mouse.wheel is not available in WebKit');

0 commit comments

Comments
 (0)