Directly handle shift-stepping in RangeControl #34719
Conversation
|
Size Change: +126 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
stokesman
requested review from
ajitbohra and
chrisvanpatten
as code owners
stokesman
force-pushed
the
fix/range-control-shift-tab-reset
branch
2 times, most recently
from
327d73e to
ac6500b
Compare
There was a problem hiding this comment.
Thank you for working on this, @stokesman !
In general, I'm always very cautious about moving away from native browser validation/behaviour, but I do understand that this may be required — and I appreciate that we are preventDefault-ing the keyDown event only when shift-stepping.
I tested the component in Storybook, and the issue explained in #34363 seems to have been addressed.
Production: After 95, shift+increment doesn't update the value to 100
range-control-shift-increment-prod.mp4
This PR: The value always gets updated to 100 when shift-incrementing
range-control-shift-increment-pr.mp4
Finally, I think we should add a few unit tests around the stepping and shift-stepping behaviour of the component. It would be best to add them in a separate PR, so that we can first write tests tailored to what we currently have in production. We can then leverage those tests in this and future PRs (like #34542) to get more confidence about the changes that are being introduced.
Aside: we seem to have style issues with RTL and RangeControl
Do you mind opening a separate issue to track this (and link back here)?
| const { min, max, step } = props; | ||
| const modifiedStep = shiftStep * step; | ||
| const nextValue = operationList[ keyCode ]( value, modifiedStep ); | ||
| onShiftStep( roundClamp( nextValue, min, max, modifiedStep ) ); |
There was a problem hiding this comment.
Is the clamping here necessary?
If onShiftStep is assigned to the handleOnChange function, the clamping should already happen in there when calling setValue (which comes from the useControlledRangeValue hook).
There was a problem hiding this comment.
Good eye! roundClamp was added only for its rounding. If it's preferred I could look at changing this to round from lodash or maybe inlining the expression we need. Or perhaps this makes a case for having the useControlledRangeValue hook made step-aware so it could handle the rounding. Though that might be better explored as part of a PR for #34817.
| ref={ ref } | ||
| step={ jumpStep } |
There was a problem hiding this comment.
Just noting that the step prop is now being passed directly when spreading { ...props }
ciampo
added
[Feature] UI Components
|
I agree that we shouldn't mutate the I tested the component on Storybook and noticed just one issue: if the first user interaction that changes the value of the range control uses Shift, it'll be updated as the initial value was Another thing that I think is worth considering as an alternative solution: do we really need this shift interaction on the range control? I couldn't find any standards or conventions regarding this. If this may cause more problems in the future, I'd say we should consider removing this non-standard feature from the component (I'm not sure if this would be considered a breaking change, but I can't think of a code or even user interface that would break because the range control stopped doing something when The native range control already provides a standard way to move with a larger step, which is by using PageUp and PageDown (or fn+ArrowUp and fn+ArrowDown). This works slightly different from what we currently have with the shift step: it always increments the current value by 10 instead of updating it to the nearest multiple of 10. So, pressing PageUp when the current value is |
|
Thanks for testing and for your feedback @diegohaz. Great catch of that issue. I've pushed a commit to fix it. In the process, I realized the number input next to the range doesn't share that behavior. If the value is empty and you shift-step there it starts from 0. If that's an issue, it's an existing one. I'll await feedback before worrying about it.
That's good to know. I was unaware of the feature. Trying it out in Brave/Chromium, I happened to notice that, it steps by a tenth of the range, i.e. always by 10 percent. That is handy, thought it also seems nice that @ciampo, thanks for reviewing. I added an issue for the RTL style. Also (assuming we don't end up going with the alternative of dropping the feature), it makes sense to add the unit tests for (shift)stepping as well. The issue just caught by @diegohaz is a good example. |
|
Thank you @stokesman for addressing the feedback (and opening a separate issue for the RTL issue)! I tend to agree with the points made by @diegohaz — probably the better solution here is to remove the custom "shift-stepping" functionality completely, and rely more on what's natively supported by the browser. Not only this will offer a more standard experience to the users, but it will also simplify our code and make it easier to maintain. I also don't think this would be considered as a breaking change. We could therefore remove the shift-stepping logic from the component, and mark the |
|
While I'd agree it's should be a safe (though technically breaking) change, I'm not favoring the removal. Also, I don't think it's quite right to say it "will offer a more standard experience to the users" as it makes it sound like the feature is altering part of the standard experience when it does not. Still, I'd be glad to move forward with either decision. It would console me to see more opinions on it is all.
Should we consider leaving them in to allow configuration of the composed |
Your point here makes sense, I expressed myself poorly. What I was trying to say here is that, by removing the custom
Hey @gziolo (sorry for the ping!), do you have any opinions on potentially dropping the custom
This is an example of how we've "soft-deprecated" some props in the past — Depending on what decision will be taken in this PR, I'd argue that we should then apply a similar decision to |
I wasn't even aware that this behavior is supported in |
@stokesman , what are the reasons behind your opposition to the removal?
Thank you Greg for chiming in! Personally, I agree with what you said regarding the removal of this functionality not being a breaking change for the users (although it technically is, as mentioned above). As mentioned before, I also find the points made by @diegohaz quite relevant — this interaction is not standard, and if we keep supporting it may cause more bugs in the future.
Do you have any option about the |
I like and use the feature in
Number inputs don't seem to have a UA provided alternative for larger steps like range inputs do. So while at least that component is still experimental, I'd rather EDIT: accidentally submitted before finishing. Removed the part I'd not finished or had even decided to include. |
|
I'd also like to say I am all for removing code and improving maintainability. I believe there are larger gains that can be made with regards to that without even removing features. I feel like we are eyeing this due to happenstance and it's not really going to be a burden. |
There was a problem hiding this comment.
Nice work here @stokesman, and thanks for digging deep into the issue! I see some of the comments sought to get some more opinions, so I thought I'd jump in and test out the PR and add a couple of thoughts.
I think the way this is implemented currently introduces a regression. In the useJumpStep hook, it's a fairly naive approach and updates the step value when the shift key is held. This means that it applies to click+drag (in additional to the direction keys) which was highlighted in the PR that introduced the feature.
Production: shift-clicking allows the jump step to be used for the RangeControl
In Storybook, shift-clicking and dragging shifts the value by increments of 10 in production. (In the screengrab, the shift key is released for a moment to show that it toggles while dragging)
Current behaviour
In Storybook, shift-clicking and dragging doesn't affect the step.
Without the shift-click-drag feature, I think I'm leaning toward the feedback some of the others have given, that it might be worth removing the feature from the range control instead of adding additional complexity to the component. In the naive approach that the useJumpStep hook uses, it doesn't need to worry about any other key than the shift key, and it also doesn't need to account for RTL.
While it's often good to hone in on the exact behaviour we'd like to control, I think unless we have a concrete example of users taking advantage of the interaction and depending on it for common tasks, I'd lean toward seeing if we can remove the additional complexity, and fall back to browser behaviour where we can. Though, I agree, this would mean that there'd be a bit of a mismatch between RangeControl and NumberControl.
To take an iterative approach, would it be worth exploring removing the shift step behaviour from the InputRange (but retain it for the range control's NumberControl), so that we can fix #34363 in the short-term, and then look at adding back in the shift step behaviour in a separate PR if it's still needed? It'd also be good to see if we can continue to contain the logic in a hook rather than directly on the component, but I'm aware this might be challenging to get working properly with the step logic.
I'm not sure how much that helps, but I'm happy to help out with further testing!
|
You have my thanks as well @stokesman for your efforts on this, nice work! 👍 I initially found my way here via your other work on the Firstly, while testing this I also came across the issue @andrewserong highlighted. That being, holding down shift while dragging the slider is no longer supported. If we are to retain the shift-stepping behaviour, I'd expect that we'd maintain current functionality as well. Some of my thoughts around the discussion so far include:
My vote would be to explore dropping the shift stepping support from the I agree also with the sentiment that this does not have to be the end of the process. We can revisit the feature when or if it is proven desired. Hope that helps a little. |
|
Thank you @andrewserong and @aaronrobertshaw for taking a look at this PR and sharing your perspective. After reading everyone's feedback, the most reasonable approach to me is to remove the We can then open a new issue to:
|
stokesman
force-pushed
the
fix/range-control-shift-tab-reset
branch
from
903241c to
0b8034e
Compare
|
I appreciate the well considered comments left by @andrewserong and @aaronrobertshaw. Thank you both and again for testing this out. More for a little challenge than anything else, I've made and pushed a change to knock out that regression. The whole approach was reworked and it some ways (lack of needing to think about RTL) it is less complex but roughly the same I think. I'm not strongly opposed to removing the feature and my main thing was it seems like we lacked a strong reason to do so. I'll agree, in this circumstance, we don't need a strong reason because we are confident this can't break anything besides user expectation (and we believe that to be practically nil). Anyway, now we have #35020 which I believe does what folks are encouraging here. |
To fix: #34363
The linked issue is prevented by using our own logic to shift-step the input instead of the current approach which modifies the DOM
stepattribute and relies on the UA stepping behavior. Thestepattribute is a factor in validation so modifying it while a value is already present is likely to invalidate the value. An invalid value may get rounded to the nearest valid value by the UA. One (and maybe only) such UA is Firefox and that is why our current approach is problematic.How has this been tested?
Manually, verifying that the linked issue can no longer be reproduced and that shift-stepping of range controls still works.
Screenshots
Shift+tab no longer causing the columns value to jump back to 1.
Also shows shift-step working toward the end of the video
after-range-control-shift-tab-reset.mp4
Shift-step in RTL
Aside: we seem to have style issues with RTL and RangeControl
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).