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

jdharvey-ibm · 2020-02-14T18:38:35Z

Closes #2695

This PR makes a few updates/fixes to the Slider component and also addresses some unit test failures on Windows.

Changelog

New

You can now click anywhere on the Slider track to begin dragging. I believe this to be an improvement over the previous behavior (when clicking/dragging on the track, the thumb would only update once you released the mouse).
Old:

New:

Changed

Fixes a bug with the Slider where fast dragging/release could sometimes neglect to fire the onRelease event.
Old:

New:
Fixed some issues with running the unit test suite on Windows.
- Fixed 2 issues relating to forward-slash vs back-slash in paths.
- There are still 3 outstanding test failures in packages/upgrade/src/migrations/carbon-icons-react/10.3.0/__tests__/update-icon-import-path-test.js because the test fixtures used in these tests contain \n line endings and the jscodeshift transform() operation (on Windows) appears to be generating output with \r\n line endings.
  - My recommendation would be to ignore these tests on Windows, but I haven't implemented anything to that effect.
- I noticed that many of the unit tests weren't running on Windows because of the way the testMatch micromatch expressions were written in package.json (see: testMatch on Windows jestjs/jest#7914)

Testing / Reviewing

The above GIFs should give a good indication of how to test this functionality in the react storybook.

* Updating jest `testMatch` micromatch expressions (jestjs/jest#7914) * Replacing `/` with `path.sep` in icon-build-helpers search.js * Replacing regex using `/` with `[\\/]` in test-utils scss.js

netlify · 2020-02-14T18:57:35Z

Deploy preview for carbon-elements ready!

Built with commit 7c6a641

https://deploy-preview-5359--carbon-elements.netlify.com

netlify · 2020-02-14T18:58:11Z

Deploy preview for carbon-components-react ready!

Built with commit 7c6a641

https://deploy-preview-5359--carbon-components-react.netlify.com

aagonzales

Looks good from a visual standpoint

abbeyhrt · 2020-02-17T18:30:29Z

Thanks for taking the time to contribute! Any thoughts on this, @joshblack?

joshblack · 2020-02-17T23:13:59Z

packages/react/src/components/Slider/Slider.js

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


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 👀

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?

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.

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.

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.

asudoh · 2020-02-18T09:37:50Z

packages/react/src/components/Slider/Slider.js

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


Nitpick - Probably a Set?

asudoh · 2020-02-19T01:13:24Z

packages/react/src/components/Slider/Slider.js

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


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.

- Adding lodash.throttle as project dependency for use in Slider - Reworking Slider component's event handling - Including throttling in Slider event handling - Adjusting CSS for input element on Slider

…thub.com/jdharvey-ibm/carbon into 2695-fix-onrelease-sometimes-doesnt-fire

jdharvey-ibm · 2020-02-24T19:13:02Z

This PR is not yet ready for merging, but I am pushing it for two reasons. 1) so I can work on it on my other machine :) and 2) to potentially give you guys an early look at it for review purposes. I need to update the unit tests and also go through the code again to make sure everything looks okay.

packages/react/src/components/Slider/Slider.js

- Increasing (line) test coverage to 100% - Slight refactor of functions to be more testable - Correctly handling hidden text input - Correctly handling touchmove events - Gracefully handling edge case of zero-width bounding rect

asudoh

@jdharvey-ibm The new code looks great basically, and thank you for the updated code. Just a couple of things:

Seems that the new code has usage of browser APIs that don't have IE11, etc. support. Please check e.g. MDN for those.
Though dragging requires throttling, I don't think arrow keys requires throttling. (We rather want to capture every gesture of increment/decrement in this case)

Looking forward to seeing the finished code!

asudoh · 2020-02-24T22:11:10Z

packages/react/src/components/Slider/Slider.js

+    if (Number.isNaN(targetValue)) {
+      this.setState({ value: evt.target.value });
+    } else {
+      targetValue = Math.clamp(targetValue, this.props.min, this.props.max);


Chrome-only Math API: https://developer.mozilla.org/en-US/docs/Web/CSS/clamp, we should replace with something else.

asudoh · 2020-02-24T22:12:19Z

packages/react/src/components/Slider/Slider.js

-      this.updatePosition
-    );
+    // Avoid calling calcValue for invaid numbers, but still update the state
+    if (Number.isNaN(targetValue)) {


IE11 doesn't support Number.isNaN(). We tend to use legacy/global isNaN() for now.

- Removing usage of functions that aren't available in all browsers - Adding unit test for display:none style on hidden text input

- Removing throttling from keydown handling - Updating unit tests accordingly

asudoh · 2020-02-26T23:48:45Z

@jdharvey-ibm Let us know when you are ready for re-review - Thanks!

jdharvey-ibm · 2020-02-27T00:51:26Z

Oops! Sorry, though I'd already hit the "request re-review" button for everyone, but I'm not sure if that worked. It's all set for re-review! No further commits planned at this time. I re-requested a review from @aagonzales, and it looks like she already re-approved the changes from a visual perspective (thanks!).

asudoh · 2020-02-27T08:27:09Z

Thank you for clarifying @jdharvey-ibm - The automatic way in GH of requesting re-review does not tend to tell what change has been done and how reviewers should act on. That's why I asked for the latest state. Sorry if it was confusing.

asudoh

LGTM 👍 - Thanks @jdharvey-ibm for finishing up the work!

jdharvey-ibm · 2020-02-27T16:39:53Z

👍 Thanks for the feedback and guidance along the way!

abbeyhrt · 2020-02-28T18:08:27Z

@joshblack not sure if you want to take a second look but the PR is ready for it! 👍

joshblack

Looks amazing! 🎉 Just a quick comment to help out with keyboard stuff 👀

joshblack · 2020-02-28T18:26:33Z

packages/react/src/components/Slider/Slider.js

+/**
+ * Constants used during keydown event processing.
+ */
+const ARROW_KEYS = Object.freeze({


We have matches and match in our keyboard module to help out with keyboard events 👀

https://github.com/carbon-design-system/carbon/blob/master/packages/react/src/internal/keyboard/match.js#L15-L32

Using `matches(...)` for arrow key event matching instead of custom impl

…2695-fix-onrelease-sometimes-doesnt-fire

joshblack

🔥

…arbon-design-system#5359) * fix(Slider): onRelease not always firing (carbon-design-system#2695) * test(windows): fixing failing unit tests * Updating jest `testMatch` micromatch expressions (jestjs/jest#7914) * Replacing `/` with `path.sep` in icon-build-helpers search.js * Replacing regex using `/` with `[\\/]` in test-utils scss.js * refactor(Slider): reworking event logic - Adding lodash.throttle as project dependency for use in Slider - Reworking Slider component's event handling - Including throttling in Slider event handling - Adjusting CSS for input element on Slider * test(Slider): updating/adding unit tests - Increasing (line) test coverage to 100% - Slight refactor of functions to be more testable - Correctly handling hidden text input - Correctly handling touchmove events - Gracefully handling edge case of zero-width bounding rect * fix(Slider): code review updates - Removing usage of functions that aren't available in all browsers - Adding unit test for display:none style on hidden text input * fix(Slider): code review updates - Removing throttling from keydown handling - Updating unit tests accordingly * refactor(Slider): use matches module Using `matches(...)` for arrow key event matching instead of custom impl Co-authored-by: Josh Black <josh@josh.black> Co-authored-by: TJ Egan <tw15egan@gmail.com> Co-authored-by: Abbey Hart <abbeyhrt@gmail.com>

jdharvey-ibm added 2 commits February 14, 2020 12:24

fix(Slider): onRelease not always firing (carbon-design-system#2695)

a5498b4

test(windows): fixing failing unit tests

f3e84ba

* Updating jest `testMatch` micromatch expressions (jestjs/jest#7914) * Replacing `/` with `path.sep` in icon-build-helpers search.js * Replacing regex using `/` with `[\\/]` in test-utils scss.js

jdharvey-ibm requested a review from a team as a code owner February 14, 2020 18:38

ghost requested review from abbeyhrt and asudoh February 14, 2020 18:38

asudoh added the status: visual review 🔍 label Feb 17, 2020

asudoh requested review from a team and aagonzales and removed request for a team February 17, 2020 09:19

aagonzales approved these changes Feb 17, 2020

View reviewed changes

laurenmrice removed the status: visual review 🔍 label Feb 17, 2020

Merge branch 'master' into 2695-fix-onrelease-sometimes-doesnt-fire

cb04ebf

joshblack reviewed Feb 17, 2020

View reviewed changes

asudoh reviewed Feb 19, 2020

View reviewed changes

jdharvey-ibm added 2 commits February 24, 2020 14:05

refactor(Slider): reworking event logic

2545e7a

- Adding lodash.throttle as project dependency for use in Slider - Reworking Slider component's event handling - Including throttling in Slider event handling - Adjusting CSS for input element on Slider

Merge branch '2695-fix-onrelease-sometimes-doesnt-fire' of https://gi…

2f7054f

…thub.com/jdharvey-ibm/carbon into 2695-fix-onrelease-sometimes-doesnt-fire

jdharvey-ibm commented Feb 24, 2020

View reviewed changes

packages/react/src/components/Slider/Slider.js Outdated Show resolved Hide resolved

test(Slider): updating/adding unit tests

dd3f739

- Increasing (line) test coverage to 100% - Slight refactor of functions to be more testable - Correctly handling hidden text input - Correctly handling touchmove events - Gracefully handling edge case of zero-width bounding rect

asudoh reviewed Feb 25, 2020

View reviewed changes

fix(Slider): code review updates

57827fd

- Removing usage of functions that aren't available in all browsers - Adding unit test for display:none style on hidden text input

jdharvey-ibm requested review from aagonzales and asudoh February 25, 2020 16:21

fix(Slider): code review updates

c38670e

- Removing throttling from keydown handling - Updating unit tests accordingly

aagonzales approved these changes Feb 25, 2020

View reviewed changes

jdharvey-ibm requested a review from joshblack February 27, 2020 00:51

asudoh approved these changes Feb 27, 2020

View reviewed changes

Merge branch 'master' into 2695-fix-onrelease-sometimes-doesnt-fire

3600eb8

asudoh added the status: ready to merge 🎉 label Feb 27, 2020

Merge branch 'master' into 2695-fix-onrelease-sometimes-doesnt-fire

39e34c3

abbeyhrt removed the status: ready to merge 🎉 label Feb 28, 2020

joshblack suggested changes Feb 28, 2020

View reviewed changes

jdharvey-ibm added 2 commits February 28, 2020 17:33

refactor(Slider): use matches module

dcae226

Using `matches(...)` for arrow key event matching instead of custom impl

Merge branch 'master' of github.com:carbon-design-system/carbon into …

34bde1f

…2695-fix-onrelease-sometimes-doesnt-fire

jdharvey-ibm requested a review from joshblack February 28, 2020 22:35

joshblack approved these changes Mar 3, 2020

View reviewed changes

Merge branch 'master' into 2695-fix-onrelease-sometimes-doesnt-fire

6729a2b

joshblack added the status: ready to merge 🎉 label Mar 3, 2020

Merge branch 'master' into 2695-fix-onrelease-sometimes-doesnt-fire

7c6a641

joshblack merged commit c89b128 into carbon-design-system:master Mar 3, 2020

This was referenced Mar 10, 2020

chore(release): v10.10.2-rc.0 #5586

Closed

chore(release): v10.10.2-rc.0 #5587

Closed

joshblack mentioned this pull request Mar 31, 2020

chore(release): v10.11.0-rc.0 #5762

Closed

jdharvey-ibm deleted the 2695-fix-onrelease-sometimes-doesnt-fire branch September 23, 2021 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jdharvey-ibm commented Feb 14, 2020 •

edited

netlify bot commented Feb 14, 2020 •

edited

netlify bot commented Feb 14, 2020 •

edited

aagonzales left a comment

abbeyhrt commented Feb 17, 2020

joshblack Feb 17, 2020

jdharvey-ibm Feb 18, 2020

asudoh Feb 19, 2020

jdharvey-ibm Feb 19, 2020

asudoh Feb 19, 2020

asudoh Feb 18, 2020

asudoh Feb 19, 2020

jdharvey-ibm commented Feb 24, 2020

asudoh left a comment •

edited

asudoh Feb 24, 2020

jdharvey-ibm Feb 25, 2020

asudoh Feb 24, 2020

jdharvey-ibm Feb 25, 2020

asudoh commented Feb 26, 2020

jdharvey-ibm commented Feb 27, 2020

asudoh commented Feb 27, 2020

asudoh left a comment

jdharvey-ibm commented Feb 27, 2020

abbeyhrt commented Feb 28, 2020

joshblack left a comment

joshblack Feb 28, 2020

jdharvey-ibm Feb 28, 2020

joshblack left a comment

Navigation Menu

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

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

Conversation

jdharvey-ibm commented Feb 14, 2020 • edited

Changelog

Testing / Reviewing

netlify bot commented Feb 14, 2020 • edited

netlify bot commented Feb 14, 2020 • edited

aagonzales left a comment

Choose a reason for hiding this comment

abbeyhrt commented Feb 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdharvey-ibm commented Feb 24, 2020

asudoh left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asudoh commented Feb 26, 2020

jdharvey-ibm commented Feb 27, 2020

asudoh commented Feb 27, 2020

asudoh left a comment

Choose a reason for hiding this comment

jdharvey-ibm commented Feb 27, 2020

abbeyhrt commented Feb 28, 2020

joshblack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshblack left a comment

Choose a reason for hiding this comment

jdharvey-ibm commented Feb 14, 2020 •

edited

netlify bot commented Feb 14, 2020 •

edited

netlify bot commented Feb 14, 2020 •

edited

asudoh left a comment •

edited