fix(#1231) fix(#1070) A11y: add minimum/maximum labels, use output for displayValue #1148

majornista · 2020-10-06T23:48:44Z

Pressing the displayValue should focus the corresponding input.

Closes #1231
Relates to #1070

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issues #1231 and #1070.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

open http://localhost:9003/?path=/story/rangeslider--custom-width
Mouse down on "Label"
Arrow keys should change value of "Minimum" slider thumb
Mouse down on display value text for the "Maximum" slider, to the right of the en dash.
Arrow keys should change value of "Maximum" slider thumb
Mouse down on display value text for the "Minimum" slider, to the left of the en dash.
Arrow keys should change value of "Minimum" slider thumb
Inspect accessibility properties for input within minimum slider thumb.
Accessibility name should be "Label Minimum"
Inspect accessibility properties for input within maximum slider thumb.
Accessibility name should be "Label Maximum"
use mouse to drag Minimum slider on top of Maximum slider, so that they both have the same value
Starting to drag the Minimum slider again, should select the Minimum slider, that was on top last.
use mouse to drag Maximum slider on top of Minimum slider, so that they both have the same value
Starting to drag the Maximum slider again, should select the Maximum slider, that was on top last.

🧢 Your Project:

ktabors

It didn't create an artifact and when I use c3e17c6d951594fa4f4c99312bde4a97d948a6e1 in an existing url it doesn't work.

ktabors · 2020-10-12T18:10:28Z

packages/@react-aria/slider/intl/index.js

+ * governing permissions and limitations under the License.
+ */
+
+import csCZ from './cs-CZ.json';


I thought we did import intlMessages from '../intl/*.json'; instead of index.js files?
Is this related to having translation files that aren't in this index.js?

In this case, I import intlMessages into @react-spectrum/slider/src/SliderBase.tsx at line 15, rather than useSliderThumb, because there doesn't seem to be a way to return the appropriate aria-label for the minimum or maximum thumb from useSliderThumb. Each thumb knows nothing about the other thumbs.

I've refactored so that the i18n gets added in useSliderThumb.

packages/@react-spectrum/slider/src/RangeSlider.tsx

packages/@react-spectrum/slider/src/SliderBase.tsx

…layValue Pressing the displayValue should focus the corresponding input. fix(adobe#1070) A11y: updates per code review Co-Authored-By: Robert Snow <[email protected]>

…pdate Slider (hook) stories Co-Authored-By: Robert Snow <[email protected]>

…necessary Co-Authored-By: Robert Snow <[email protected]>

LFDanLu

I also noticed that the accessibility name (and what was announced by VoiceOver) for the thumbs was "Slider [value of slider]", no "Maximum" or "Minimum" (the aria label was correct though, and aria-labelledby correctly had the input id appended to the end). Confirmed that the rest of what you noted in the repro steps was correct though.

Is this correct? Screenshot below shows the aria-label crossed out in the accessibility dev tool section

Otherwise, just some small things/questions

LFDanLu · 2020-12-15T23:30:39Z

packages/@react-aria/slider/intl/ar-AE.json

+{
+  "maximum": "الحد الأقصى",
+  "minimum": "الحد الأدنى"
+}


I presume these translations were vetted by the localization team or came from a prior source that was vetted?

LFDanLu · 2020-12-15T23:48:05Z

packages/@react-aria/slider/src/useSliderThumb.ts

+  if (state.values.length === 2 && !ariaLabel) {
+    if (index === 0) {
+      ariaLabel = formatMessage('minimum');
+    } else if (index === 1) {
+      ariaLabel = formatMessage('maximum');
+    }
+  }


Perhaps overkill for now, but should this account for cases that have more than 2 slider thumbs? I guess it would just need to check that index === state.value.length - 1 for maximum.

Not sure if it is appropriate though, perhaps these aria messages are for range sliders only

I'm also not sure if we should put this here as it kinda assumes that all two-thumbed sliders are range sliders. IMO the labels should go in Spectrum RangeSlider since we don't have a range specific aria hook.

LFDanLu · 2020-12-16T00:14:20Z

packages/@react-aria/slider/stories/story-slider.css

+div[style='isolation: isolate; color-scheme: dark;'] .label {
+  color: #b0b0b0;
+}
+
+div[style='isolation: isolate; color-scheme: dark;'] .value {
+  color: #9a9a9a;
+}
+
+div[style='isolation: isolate; color-scheme: dark;'] .label {
+  color: #b0b0b0;
+}
+
+div[style='isolation: isolate; color-scheme: dark;'] .value {
+  color: #9a9a9a;
+}


Looks like you have two sets of the same style here

packages/@react-aria/slider/intl/el-GR.json

packages/@react-aria/slider/intl/lt-LT.json

packages/@react-aria/slider/intl/lv-LV.json

devongovett · 2020-12-16T15:33:42Z

I'm going to refactor this slightly and send a separate PR later today. Three issues:

Moving the minimum/maximum labels into React Spectrum's RangeSlider component rather than in useSliderThumb, as it currently makes the assumption that all two-handle sliders represent a range.
Both Slider and RangeSlider support a custom valueLabel, and currently this is not wrapped in an <output> element. Therefore, in the general case, we can't have a separate <output> element for each thumb as we don't know how the value will be formatted. I think having a single <output> element to wrap the entire value label makes sense. This does mean clicking on the individual number values won't focus the corresponding handle but that seems a little hard to discover anyway.
The props for the output element should be returned from useSlider rather than left to each implementation.

Co-authored-by: Daniel Lu <[email protected]>

majornista · 2020-12-16T20:28:25Z

I also noticed that the accessibility name (and what was announced by VoiceOver) for the thumbs was "Slider [value of slider]", no "Maximum" or "Minimum" (the aria label was correct though, and aria-labelledby correctly had the input id appended to the end). Confirmed that the rest of what you noted in the repro steps was correct though.

Is this correct? Screenshot below shows the aria-label crossed out in the accessibility dev tool section

Otherwise, just some small things/questions

This seems to be a Chromium bug, that I can reproduce: https://codepen.io/majornista/pen/QWKpYrR. Safari seems to be behaving correctly. I can work around it by referencing another element, other than the input, like the handle container element for example, to provide the aria-label for the input.

Per adobe#1148 (review) and https://bugs.chromium.org/p/chromium/issues/detail?id=1159567

#1389)" This reverts commit f8e2bee.

…" (#1397) * Revert "Slider accessibility name calculation fixes for Chrome (#1148) (#1389)" This reverts commit f8e2bee. * Update packages/@react-spectrum/color/test/ColorSlider.test.tsx Co-authored-by: Danni <[email protected]>

…#1148)" (adobe#1397) * Revert "Slider accessibility name calculation fixes for Chrome (adobe#1148) (adobe#1389)" This reverts commit f8e2bee. * Update packages/@react-spectrum/color/test/ColorSlider.test.tsx Co-authored-by: Danni <[email protected]>

majornista requested review from dannify, devongovett and snowystinger as code owners October 6, 2020 23:48

ktabors reviewed Oct 12, 2020

View reviewed changes

devongovett mentioned this pull request Oct 14, 2020

Color Slider followup #1162

Closed

3 tasks

fix(adobe#1070) A11y: add minimum/maximum labels, use output for disp…

2ff4fdb

…layValue Pressing the displayValue should focus the corresponding input. fix(adobe#1070) A11y: updates per code review Co-Authored-By: Robert Snow <[email protected]>

majornista force-pushed the a11y-slider branch from a0f9151 to 2ff4fdb Compare November 3, 2020 15:14

majornista requested a review from ktabors November 3, 2020 15:15

majornista and others added 2 commits November 3, 2020 16:39

fix(adobe#1070) refactor minimum/maximum labels into useSliderThumb/u…

b84c6f9

…pdate Slider (hook) stories Co-Authored-By: Robert Snow <[email protected]>

fix(adobe#1070) A11y: remove slider/intl/index.js which is no longer …

20dab74

…necessary Co-Authored-By: Robert Snow <[email protected]>

majornista mentioned this pull request Nov 4, 2020

Slider Accessibility Audit #1231

Closed

majornista changed the title ~~fix(#1070) A11y: add minimum/maximum labels, use output for displayValue~~ fix(#1231) fix(#1070) A11y: add minimum/maximum labels, use output for displayValue Nov 4, 2020

majornista added 4 commits November 12, 2020 10:53

Merge branch 'main' into a11y-slider

c1ca2a4

Merge branch 'main' into a11y-slider

5523a4c

Merge branch 'main' into a11y-slider

5f68a43

Merge branch 'main' into a11y-slider

bd6db84

LFDanLu reviewed Dec 16, 2020

View reviewed changes

packages/@react-aria/slider/intl/el-GR.json Outdated Show resolved Hide resolved

LFDanLu reviewed Dec 16, 2020

View reviewed changes

packages/@react-aria/slider/intl/lt-LT.json Outdated Show resolved Hide resolved

LFDanLu reviewed Dec 16, 2020

View reviewed changes

packages/@react-aria/slider/intl/lv-LV.json Outdated Show resolved Hide resolved

devongovett mentioned this pull request Dec 16, 2020

Slider refactor and a11y improvements #1387

Merged

fix(i18n): Apply suggestions from code review

6175bbd

Co-authored-by: Daniel Lu <[email protected]>

refactor(per code review): remove duplicate style declaration

bc8e421

dannify closed this in #1387 Dec 16, 2020

majornista added a commit to majornista/react-spectrum-v3 that referenced this pull request Dec 17, 2020

Slider accessibility name calculation fixes for Chrome (adobe#1148)

9bab2de

Per adobe#1148 (review) and https://bugs.chromium.org/p/chromium/issues/detail?id=1159567

majornista mentioned this pull request Dec 17, 2020

Slider accessibility name calculation fixes for Chrome (#1148) #1389

Merged

3 tasks

devongovett pushed a commit that referenced this pull request Dec 17, 2020

Slider accessibility name calculation fixes for Chrome (#1148) (#1389)

f8e2bee

devongovett added a commit that referenced this pull request Dec 17, 2020

Revert "Slider accessibility name calculation fixes for Chrome (#1148) (

8544994

#1389)" This reverts commit f8e2bee.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#1231) fix(#1070) A11y: add minimum/maximum labels, use output for displayValue #1148

fix(#1231) fix(#1070) A11y: add minimum/maximum labels, use output for displayValue #1148

majornista commented Oct 6, 2020 •

edited

Loading

ktabors left a comment

ktabors Oct 12, 2020

majornista Nov 3, 2020 •

edited

Loading

majornista Nov 4, 2020

LFDanLu left a comment

LFDanLu Dec 15, 2020

LFDanLu Dec 15, 2020

devongovett Dec 16, 2020

LFDanLu Dec 16, 2020

devongovett commented Dec 16, 2020

majornista commented Dec 16, 2020

fix(#1231) fix(#1070) A11y: add minimum/maximum labels, use output for displayValue #1148

fix(#1231) fix(#1070) A11y: add minimum/maximum labels, use output for displayValue #1148

Conversation

majornista commented Oct 6, 2020 • edited Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

ktabors left a comment

Choose a reason for hiding this comment

ktabors Oct 12, 2020

Choose a reason for hiding this comment

majornista Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

majornista Nov 4, 2020

Choose a reason for hiding this comment

LFDanLu left a comment

Choose a reason for hiding this comment

LFDanLu Dec 15, 2020

Choose a reason for hiding this comment

LFDanLu Dec 15, 2020

Choose a reason for hiding this comment

devongovett Dec 16, 2020

Choose a reason for hiding this comment

LFDanLu Dec 16, 2020

Choose a reason for hiding this comment

devongovett commented Dec 16, 2020

majornista commented Dec 16, 2020

majornista commented Oct 6, 2020 •

edited

Loading

majornista Nov 3, 2020 •

edited

Loading