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

Jump to bottom

fix(Slider): onRelease not always firing (#2695) #5359

Merged
merged 14 commits into from Mar 3, 2020
Merged

fix(Slider): onRelease not always firing (#2695) #5359

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a5498b4
fix(Slider): onRelease not always firing (#2695)
jdharvey-ibm Feb 14, 2020
f3e84ba
test(windows): fixing failing unit tests
jdharvey-ibm Feb 14, 2020
cb04ebf
Merge branch 'master' into 2695-fix-onrelease-sometimes-doesnt-fire
joshblack Feb 17, 2020
2545e7a
refactor(Slider): reworking event logic
jdharvey-ibm Feb 24, 2020
2f7054f
Merge branch '2695-fix-onrelease-sometimes-doesnt-fire' of https://gi…
jdharvey-ibm Feb 24, 2020
dd3f739
test(Slider): updating/adding unit tests
jdharvey-ibm Feb 25, 2020
57827fd
fix(Slider): code review updates
jdharvey-ibm Feb 25, 2020
c38670e
fix(Slider): code review updates
jdharvey-ibm Feb 25, 2020
3600eb8
Merge branch 'master' into 2695-fix-onrelease-sometimes-doesnt-fire
tw15egan Feb 27, 2020
39e34c3
Merge branch 'master' into 2695-fix-onrelease-sometimes-doesnt-fire
abbeyhrt Feb 28, 2020
dcae226
refactor(Slider): use matches module
jdharvey-ibm Feb 28, 2020
34bde1f
Merge branch 'master' of github.com:carbon-design-system/carbon into …
jdharvey-ibm Feb 28, 2020
6729a2b
Merge branch 'master' into 2695-fix-onrelease-sometimes-doesnt-fire
joshblack Mar 3, 2020
7c6a641
Merge branch 'master' into 2695-fix-onrelease-sometimes-doesnt-fire
joshblack Mar 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Expand Up @@ -175,8 +175,8 @@
],
"testMatch": [
"<rootDir>/**/__tests__/**/*.js?(x)",
"<rootDir>/**/?(*.)(spec|test).js?(x)",
"<rootDir>/**/?(*-)(spec|test).js?(x)"
"<rootDir>/**/*.(spec|test).js?(x)",
"<rootDir>/**/*-(spec|test).js?(x)"
],
"transform": {
"^.+\\.(js|jsx)$": "./tasks/jest/jsTransform.js",
Expand Down
2 changes: 1 addition & 1 deletion packages/icon-build-helpers/src/search.js
View file
Open in desktop
Expand Up @@ -55,7 +55,7 @@ async function search(directory) {
const dirname = path.dirname(filepath);
const prefix = path
.relative(directory, dirname)
.split('/')
.split(path.sep)
.filter(Boolean);
return {
...file,
Expand Down
8 changes: 7 additions & 1 deletion packages/react/src/components/Slider/Slider-test.js
View file
Open in desktop
Expand Up @@ -150,10 +150,15 @@ describe('Slider', () => {

describe('user is holding the handle', () => {
it('does not call onRelease', () => {
const evt = {
type: 'mousedown',
clientX: '1000',
persist: jest.fn(),
};
handleRelease.mockClear();
expect(handleRelease).not.toHaveBeenCalled();

wrapper.instance().handleMouseStart();
wrapper.instance().handleMouseStart(evt);
wrapper.instance().updatePosition();
expect(handleRelease).not.toHaveBeenCalled();
});
Expand All @@ -165,6 +170,7 @@ describe('Slider', () => {
expect(handleRelease).not.toHaveBeenCalled();
wrapper.setState({
holding: false,
needsOnRelease: true,
});
wrapper.instance().updatePosition();
expect(handleRelease).toHaveBeenCalled();
Expand Down
178 changes: 124 additions & 54 deletions packages/react/src/components/Slider/Slider.js
View file
Open in desktop
Expand Up @@ -17,6 +17,38 @@ const defaultFormatLabel = (value, label) => {
return typeof label === 'function' ? label(value) : `${value}${label}`;
};

/**
* Types of events used to indicate that the slider's value should get recalculated.
*/
const CALC_VALUE_EVENT_TYPES = Object.freeze([
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick - Probably a Set?

'mousemove',
'mousedown',
'click',
'touchmove',
'touchstart',
]);

/**
* Notes on Slider event handling:
* - The Slider handles 6 types of events. Five of them are defined in the `CALC_VALUE_EVENT_TYPES`
* array, while the last one, `mouseup`, is added on-the-fly in response to a `mousedown` event.
* 'mouseup' will remove itself as an event listener when fired.
*
* - All of the event handlers eventually drop into `this.updatePosition`.
*
* - `this.updatePosition` serves 3 main roles: 1) Calculating a new value and thumb position; 2)
* Updating the state with the new values; 3) Requesting an animation frame from the browser,
* during which the `onChange` and `onRelease` callbacks are potentially called.
*
* - `requestAnimationFrame` is used to rate-limit the number of events that are sent back to the
* component user as the slider changes.
*
* - As the value/thumb position are updated, `this.state.needsOnChange` and
* `this.state.needsOnRelease` may be set to true, indicating that during the next requested
* animation frame, the `onChange` and/or `onRelease` callbacks should be called.
* - Events fired with types other than those listed in `CALC_VALUE_EVENT_TYPES` will not result in
* the value/thumb position being recalculated.
*/
export default class Slider extends PureComponent {
static propTypes = {
/**
Expand Down Expand Up @@ -143,10 +175,11 @@ export default class Slider extends PureComponent {
};

state = {
dragging: false,
holding: false,
value: this.props.value,
left: 0,
needsOnChange: false,
needsOnRelease: false,
};

static getDerivedStateFromProps({ value, min, max }, state) {
Expand Down Expand Up @@ -176,41 +209,56 @@ export default class Slider extends PureComponent {
evt.persist();
}

if (this.state.dragging) {
return;
}
this.setState({ dragging: true });

this.handleDrag();

requestAnimationFrame(() => {
this.setState((prevState, props) => {
// Note: In FF, `evt.target` of `mousemove` event can be `HTMLDocument` which doesn't have `classList`.
// One example is dragging out of browser viewport.
const fromInput =
evt &&
evt.target &&
evt.target.classList &&
evt.target.classList.contains('bx-slider-text-input');
const { left, newValue: newSliderValue } = this.calcValue(
evt,
prevState,
props
);
const newValue = fromInput ? Number(evt.target.value) : newSliderValue;
if (prevState.left === left && prevState.value === newValue) {
return { dragging: false };
const setStateUpdater = (prevState, props) => {
// Note: In FF, `evt.target` of `mousemove` event can be `HTMLDocument` which doesn't have
// `classList`. One example is dragging out of browser viewport.
const fromInput =
evt &&
evt.target &&
evt.target.classList &&
evt.target.classList.contains('bx-slider-text-input');

const { left, newValue: newSliderValue } = this.calcValue(
evt,
prevState,
props
);

const newValue = fromInput ? Number(evt.target.value) : newSliderValue;

if (prevState.left === left && prevState.value === newValue) {
return;
}

return {
left,
value: newValue,
needsOnChange: true,
};
};

const setStateCallback = () => {
this.handleDragComplete();

requestAnimationFrame(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the call to rAF is moved from enqueuing the state update to now calling props.onChange, was there any particular reason for this change? Super curious 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

I did this primarily because the way rAF was being used was the cause of the bug, but also because it was hard to determine exactly why rAF was being used. I ran under the assumption that it was being used to rate limit the number of onChange and onRelease callbacks that are fired, since when we chatted on Webex, I believe you indicated that this type of rAF usage wasn't a common paradigm across components.

I got a message earlier from @asudoh indicating that the reason for this rAF usage was in fact to throttle the number of mousemove events that are considered for state updates, but in spite of this intent, I don't believe this is actually the way the component was behaving. Since pretty much every mousemove event ultimately resulted in a call to rAF, these events were simply being queued for batch execution at the next animation frame; none of them would actually be discarded from consideration altogether. In theory, the net number of state updates would be the same, except in the old implementation, the setState calls would be slightly delayed until the next animation frame picked them up for execution. This delayed state update is what causes the bug, since a mouseup event could actually fire and complete before the next animation frame had a chance to update the state in the proper way to cause the onRelease callback to fire.

so I guess my question is:

What is the Carbon-wide paradigm here? Are callbacks and events typically rate limited? If so, are there typical ways in which components accomplish this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your thoughts @jdharvey-ibm! Basically, we use lodash.debounce or lodash.throttle whenever handling every event does not make sense; One of the cases is stale event, e.g. if we have two events to open timer-based tooltip in a short amount of time, the earlier one is stale (given the timer should be based on the latter). Another is many same events (e.g. mousemove or resize) in short amount of time, especially if the event handler involves heavy operation, which is the case of <Slider> (more below).

The current <Slider> code cancels running the logic of moving the thumb if one for earlier event is pending, which means an optimization similar to throttling:
https://github.com/carbon-design-system/carbon/blob/v10.9.0/packages/react/src/components/Slider/Slider.js#L169-L171

The direct cause of the problem is that running onRelease() callback is canceled if the operation of moving the thumb is canceled, even though calling onRelease() is not considered heavy. That said, moving handleDrag() to before that line at least ensures that onRelease() is called. Wondering it's something you want to try instead of doing some refactoring.

An indirect cause of the problem that such throttling logic is written in a way that may confuse people who try to jump in and maintain the code that the logic is for managing "dragging state" instead. That caused the one who introduced onRelease() to put his code into a wrong place. That said, in future, I think we need to put part of mousemove handler code (only the logic calculating the position) into lodash.throttle callback rather than maintaining our own logic of throttling.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately making that particular change you suggest (I did indeed try that earlier on) regresses the deterministic behavior of the order in which the events are fired. Since onChange is still fired within the rAF call, it becomes possible to have onRelease called before onChange. If that is acceptable, then we can go with that instead, but I feel like it's kind of weird to change that behavior.

If the long-term goal is to convert this to use _.throttle, then I could probably take a stab at that over the next week or two, but I'd like your feedback on that before going off in that direction.

Copy link
Contributor

Choose a reason for hiding this comment

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

onChange/onRelease order is a great point. A quick fix may be enqueueing onRelease call instead of calling it directly at https://github.com/carbon-design-system/carbon/blob/v10.9.0/packages/react/src/components/Slider/Slider.js#L347.

And you are right that the long-term goal is converting the code to use _.throttle.

if (this.state.needsOnChange) {
if (typeof this.props.onChange === 'function') {
this.props.onChange({ value: this.state.value });
}
}
if (typeof props.onChange === 'function') {
props.onChange({ value: newValue });
if (this.state.needsOnRelease) {
if (typeof this.props.onRelease === 'function') {
this.props.onRelease({ value: this.state.value });
}
}
return {
dragging: false,
left,
value: newValue,
};
this.setState({
needsOnChange: false,
needsOnRelease: false,
});
});
});
};

this.setState(setStateUpdater, setStateCallback);
};

calcValue = (evt, prevState, props) => {
Expand Down Expand Up @@ -248,7 +296,8 @@ export default class Slider extends PureComponent {
newValue = Number(value) + stepMultiplied * direction;
}
}
if (type === 'mousemove' || type === 'click' || type === 'touchmove') {
// If the event type indicates that we should update, then recalculate the value.
if (CALC_VALUE_EVENT_TYPES.includes(type)) {
const clientX = evt.touches ? evt.touches[0].clientX : evt.clientX;
const track = this.track.getBoundingClientRect();
const ratio = (clientX - track.left) / track.width;
Expand All @@ -270,10 +319,38 @@ export default class Slider extends PureComponent {
return { left, newValue };
};

handleMouseStart = () => {
this.setState({
holding: true,
});
/**
* Check if dragging is done. If so, request an `onRelease` by setting `needsOnRelease` in the
* component state.
*/
handleDragComplete = () => {
if (
typeof this.props.onRelease === 'function' &&
!this.props.disabled &&
!this.state.holding
) {
this.setState({
needsOnRelease: true,
});
}
};

handleMouseStart = evt => {
if (!evt) {
return;
} else {
// Persist the synthetic event so it can be accessed below in setState
evt.persist();
}

this.setState(
{
holding: true,
},
() => {
this.updatePosition(evt);
}
);

this.element.ownerDocument.addEventListener(
'mousemove',
Expand Down Expand Up @@ -301,9 +378,12 @@ export default class Slider extends PureComponent {
};

handleTouchStart = () => {
this.setState({
holding: true,
});
this.setState(
{
holding: true,
},
this.updatePosition
);
this.element.ownerDocument.addEventListener(
'touchmove',
this.updatePosition
Expand Down Expand Up @@ -350,16 +430,6 @@ export default class Slider extends PureComponent {
this.updatePosition(evt);
};

handleDrag = () => {
if (
typeof this.props.onRelease === 'function' &&
!this.props.disabled &&
!this.state.holding
) {
this.props.onRelease({ value: this.state.value });
}
};

render() {
const {
ariaLabelInput,
Expand Down Expand Up @@ -433,6 +503,9 @@ export default class Slider extends PureComponent {
}}
onClick={this.updatePosition}
onKeyPress={this.updatePosition}
onMouseDown={this.handleMouseStart}
onTouchStart={this.handleTouchStart}
onKeyDown={this.updatePosition}
role="presentation"
tabIndex={-1}
{...other}>
Expand All @@ -445,9 +518,6 @@ export default class Slider extends PureComponent {
aria-valuemin={min}
aria-valuenow={value}
style={thumbStyle}
onMouseDown={this.handleMouseStart}
onTouchStart={this.handleTouchStart}
onKeyDown={this.updatePosition}
/>
<div
className={`${prefix}--slider__track`}
Expand Down
2 changes: 1 addition & 1 deletion packages/test-utils/scss.js
View file
Open in desktop
Expand Up @@ -51,7 +51,7 @@ function createImporter(cwd) {
},
pathFilter(pkg, path, relativePath) {
// Transforms `scss/filename` to `scss/_filename.scss`
return relativePath.replace(/^(scss\/)([a-z-]+)/, '$1_$2.scss');
return relativePath.replace(/^(scss[\\/])([a-z-]+)/, '$1_$2.scss');
},
});
done({ file });
Expand Down