-
Notifications
You must be signed in to change notification settings - Fork 89
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.
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
feat(forms): back MultiThumbSlider
with new useSlider
hook
#1431
Conversation
@@ -438,7 +456,7 @@ describe('MultiThumbRange', () => { | |||
const { getAllByTestId } = render(<MultiThumbRange minValue={50} maxValue={40} />); | |||
const thumb = getAllByTestId('thumb')[1]; | |||
|
|||
expect(thumb).toHaveStyle('left: 50px'); | |||
expect(thumb).toHaveStyle('left: 40px'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't have expected this to change, @jzempel do you know why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in the new hook we picked a consistent approach for applying out-of-bounds logic – that is, start-to-end (the minimum will determine where the maximum can start rather than the other way around). The previous logic was inconsistent in MultiThumbRange
.
( | ||
{ | ||
min = MIN, | ||
max = MAX, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to define a default value for min
and max
here if the defaultProps
were set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helps avoid type-checked conditional logic for calculations that follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jzempel — overall, the code looks great; however, when I did some basic testing on the latest build, I ran into a couple of issues.
Dragging thumbs makes track fill disappear at ends of range
This happens both at the min and max:
Screen.Recording.2022-10-11.at.11.47.29.AM.mov
VoiceOver doesn't stay on thumb(s) when trying to increment or decrement with the arrow keys
I had observed similar behavior when I was working on the container originally, and I specifically noted that it went away with your version. Not totally sure what could be happening here:
Screen.Recording.2022-10-11.at.11.45.55.AM.mov
Wondering if maybe it has to do with the DOM structure, now that it's inside of a Field?
Keyboard functionality does appear to be working fine, when VoiceOver isn't involved. 👍
@@ -159,12 +159,14 @@ export interface IMultiThumbRangeProps extends Omit<HTMLAttributes<HTMLDivElemen | |||
min?: number; | |||
/** Sets the maximum permitted value */ | |||
max?: number; | |||
/** Sets the minimum thumb input value */ | |||
/** Sets the minimum thumb value */ | |||
minValue?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to pull in all of these types from the container types? To ensure a 1:1 match.
e.g.,
minValue?: IUseSliderProps['minValue']
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a bad thought. But given that we've never done this in the past, I'd rather not establish a new pattern without added consideration and intention.
I'm not able to reproduce. What are the characteristics of your environment? Also, do you have the same issues with the current https://zendeskgarden.github.io/react-components/?path=/story/packages-forms-multithumbrange--multi-thumb-range
Are you able to reproduce with https://zendeskgarden.github.io/react-components/?path=/story/packages-forms-multithumbrange--multi-thumb-range If there are no regressions, we may want to pick these up as follow-on issues. |
p.s. just checking... are you using the VO+arrow keys? For example, on this page – https://garden.zendesk.com/components/multi-thumb-range – I need opt+rightarrow in order to operate the slider when VO is active. |
@jzempel I'll double-check and let you know. 👍 Totally agree that if this isn't something new, we shouldn't necessarily address it now.
I was using the arrow keys by themselves. This is how I had tested the Slider container, too. |
Hey @jzempel — following up on the two behaviors I flagged earlier this week. Dragging thumbs makes track fill disappear at ends of rangeJust tested the odd dragging behavior again on staging. Here's a fresh video of what I was seeing: Drag.functionality.working.not.as.expected.on.staging.Storybook.movInterestingly enough, I experienced the same odd behavior on prod: Reproducing.drag.functionality.on.prod.Storybook.movHowever, I wasn't seeing this behavior at all on the Garden website, on prod: Drag.functionality.working.as.expected.on.Garden.website.movThis leads me to think that, maybe, the issue is with the Storybook demo itself. tl;dr: This probably is something we can revisit in the future. VoiceOver doesn't stay on thumb(s) when trying to increment or decrement with the arrow keysJust tested this again on staging —both with and without the VO keys. In this video, I'm interacting with the min value thumb with only the arrow keys, and then I'm interacting with the max value thumb with the arrow keys + VO keys: Arrow.keys.with.and.without.VO.keys.have.strange.behavior.on.staging.movOn prod, I was able to navigate the existing Multi-thumb range sample using only the arrow keys: Arrow.keys.work.without.VO.keys.on.Storybook.prod.movSame for the Garden website, on prod: Arrow.keys.work.without.VO.keys.on.Garden.website.prod.movThis leads me to think that something is going on with the keyboard functionality code, or, perhaps, the DOM structure when the Multi-thumb range lives inside of a Field. tl;dr: We may want to look into this a bit more now, as it potentially is a regression in functionality. 💥 |
As a point of comparison, here's what I experienced when I tested the component on staging, via Assistiv Labs. In each of these tests, I had the screen reader on, and navigated using the left and right arrow keys only. Chrome x JAWS (Windows)Multi-thumb.range.-.Chrome.x.JAWS.movChrome x NVDA (Windows)Multi-thumb.range.-.Chrome.x.NVDA.mov |
@jgorfine-zendesk I applied the 0.1.1 container fix and VO+arrow keys seem to be working as expected, on Safari, without screenreader drift. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jzempel, I have some good news and some bad news.
The good news
The Multi-thumb range appears to be working as intended on the following browser and screen reader combinations:
VoiceOver x Safari
Multi-thumb.range.staging.Safari.x.VoiceOver.mov
NVDA x Chrome (via AssistivLabs)
Multi-thumb.range.staging.Chrome.x.NVDA.mov
JAWS x Chrome (via AssistivLabs)
Multi-thumb.range.staging.Chrome.x.JAWS.mov
The bad news
When I tested the staging instance on Chrome with VoiceOver on my local machine, I was completely unable to operate either slider with the keyboard —both with and without the VO keys.
Here's a video in which the keyboard navigation is working just fine, without VoiceOver...and then becomes completely inoperable with it:
Multi-thumb.range.staging.Chrome.x.VoiceOver.mov
Moving forward
Since it's likely that someone using VoiceOver will be using it with Safari (per the WebAIM Screen Reader Survey #9, conducted last year), and Safari + VoiceOver appears to be working as expected on both of our machines, I say we go ahead and merge these changes in. We could always loop in @zendesk-lzingarelli for additional testing and QA and do a fast followup if the bug rears its ugly head again.
Screenreader drift addressed in container code
@jgorfine-zendesk at some point we might want to do a setup comparison. Chrome works fine for me with VO activated – as long as I hold down the |
Description
Event and accessibility logic are now contained in
@zendeskgarden/container-slider
. This includes feature 🚀 support for the newjump
prop that supports WAI-ARIA page up/down keyboard navigation.Detail
Consideration was given to adding uncontrolled component support (offered by the
useSlider
hook), but without backing inputs, this makes no sense for the current component. Props for future consideration include:defaultMinValue
anddefaultMaxValue
uncontrolled values connected to backing<input hidden>
elements (see reverted 8e20ec4)minValueText
andmaxValueText
for optional clarification ofaria-valuenow
(see https://www.w3.org/WAI/ARIA/apg/patterns/slidertwothumb/#wai-aria-roles-states-and-properties-18)Checklist
design updates will be Garden Designer approved (add the designer as a reviewer)yarn start
)?bedrock
)