[On-hold] RSP-1396 [Accessibility] Asterisk has ariaLabel and role presentation #85

majornista · 2019-12-06T00:54:25Z

NOTE: this PR includes a commit to address the related issue for labelling and setting the role on Icon and UIIcon components RSP-1360

✅ Pull Request Checklist:

Included link to corresponding RSP-1396 and RSP-1360.
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 exist for this component).
Looked at the Accessibility Standard and/or talked to @mijordan about Accessibility for this feature.

📝 Test Instructions:

Open http://localhost:9003/?path=/story/fieldlabel--required-styles
Verify spacing on FieldLabel with necessityLabel and Required Asterix icon
Using web inspector, verify that for the field labelled "Required label (required)", the neccessityLabel span has aria-hidden="true" and the related textfield input has aria-required="true"
Using web inspector, verify that for the field labelled "Optional label (options)", the neccessityLabel span does not have aria-hidden="true" and the related textfield input does not have aria-required="true"
Using web inspector, verify that for the field labelled "React *", the icon svg does not have an aria-label and has aria-hidden="true" and the related textfield input has aria-required="true"
Using web inspector, verify that for the field labelled "isRequired on child", the icon svg is rendered, does not have an aria-label and has aria-hidden="true" and the related textfield input has aria-required="true". This is the case where a single child with isRequired forces isRequired on the parent label.

🧢 Your Team:

Accessibility

adobe-bot · 2019-12-06T01:02:53Z

Build successful! View the storybook

packages/@react-spectrum/form/intl/ja-JP.json

packages/@react-aria/label/src/useLabel.ts

packages/@react-spectrum/form/src/LabelBase.tsx

devongovett · 2019-12-06T21:53:07Z

packages/@react-spectrum/form/src/LabelBase.tsx

-      {necessityIndicator && ' '}
-      {necessityIndicator === 'label' && <span>{necessityLabel}</span>}
+      {necessityIndicator ? `${label} ` : label}
+      {necessityIndicator === 'label' && <span className={classNames(styles, 'react-spectrum-FieldLabel-necessityLabel')} aria-hidden={isRequired && childArray.length === 1}>{necessityLabel}</span>}


Do we need this extra span? Looks like spectrum-css doesn't have it. https://opensource.adobe.com/spectrum-css/components/fieldlabel/

The span allows me to hide the necessityIndicator from AT when the component isRequired, in order to prevent double voicing of 'required' on the input that has aria-required.

devongovett · 2019-12-06T21:55:30Z

packages/@react-spectrum/icon/src/UIIcon.tsx

-    role: 'presentation',
+    'aria-label': ariaLabel || alt,
+    'aria-hidden': (ariaLabel || alt ? ariaHidden : true),
+    role: 'img',


Are we sure we want UIIcon to have role=img? That has effects across many components. Seems more likely that most UIIcons should be aria-hidden by default.

Always having a role is useful for the unit tests, and @jnurthen asked that we avoid role="presentation" with aria-hidden="true". Either way, it's something of a moot point when the UIIcon has no aria-label or alt text. With aria-label or alt text, the role should be img unless explicitly hidden with aria-hidden.

ktabors · 2019-12-06T22:42:57Z

packages/@react-aria/label/src/useLabel.ts

+  isRequired?: boolean
+}
+
+export function useLabel(labelProps: LabelProps & DOMProps = {}, labelledComponentProps: LabelComponentAriaProps = {}) {


why is labelledComponentProps on both the input and output of this function?

The descendant component may already have some relevant aria-label or aria-labelledby props, that when wrapped in a LabelBase may need to be updated.

packages/@react-aria/label/src/useLabel.ts

ktabors · 2019-12-06T22:46:52Z

packages/@react-spectrum/button/test/ActionButton.test.js

@@ -35,16 +35,31 @@ describe('ActionButton', function () {
    expect(button).toHaveAttribute('aria-hidden', 'true');
  });

+  // TODO: update v2 to use `role="img" aria-hidden="true"` for hold affordance icon, and then recombine following two tests.


Why do we have a todo about v2? I know this is a note about improving the tests but it seems like the v2 story should have a note about updating v3 tests.

That makes sense. We don't have a v2 story for this issue yet, but I'll create one.

https://jira.corp.adobe.com/browse/RSP-1434

PR against v2: #716

packages/@react-spectrum/button/test/ActionButton.test.js

packages/@react-spectrum/icon/test/Icon.test.js

packages/@react-spectrum/icon/test/UIIcon.test.js

adobe-bot · 2019-12-09T21:08:29Z

Build successful! View the storybook

adobe-bot · 2019-12-09T22:07:11Z

Build successful! View the storybook

packages/@adobe/spectrum-css-temp/components/fieldlabel/index.css

packages/@react-spectrum/button/test/ActionButton.test.js

adobe-bot · 2019-12-10T17:55:09Z

Build successful! View the storybook

adobe-bot · 2019-12-10T17:55:41Z

Build successful! View the storybook

adobe-bot · 2019-12-10T18:02:53Z

Build successful! View the storybook

adobe-bot · 2019-12-10T19:48:56Z

Build successful! View the storybook

devongovett · 2019-12-11T01:25:51Z

packages/@react-spectrum/form/src/LabelBase.tsx

+    <Asterisk
+      className={classNames(styles, 'spectrum-FieldLabel-requiredIcon')}
+      size="S"
+      alt={childArray.length === 1 ? undefined : formatMessage('(required)').replace(/[(（(](.+?)[)）)]/g, '$1')} />


Should we remove this alt completely and set aria-hidden to true on the icon instead (second option in ticket)? Seems like it would be simpler, and avoid duplicating required text in label and aria-required on input. I forget why the second option was discarded.

Also noticed below that when necessity indicator is a label and not an icon, it is aria-hidden, but not when it's an icon?

We want the icon to have alt text when the childArray.length > 1. Icons are hidden by default when alt is undefined.

Why though? The input would have aria-required on it so would be announced as required already. Wouldn't the extra alt text on the icon just be redundant? I'm not sure about this case with > 1 child. Wouldn't all the inputs have aria-required on them?

I don’t think that a fieldset of checkboxes requiring one or more selections would have aria-required on each checkbox, but the fieldset should read as required.

Ah. Wondering if there should be a separate component for groups of fields as opposed to a single one. The logic in here is getting really complicated and hard to follow. Not sure we nailed the API on this one yet.

We could have a Fieldset component, but I'm not sure that that prevents users from using FieldLabel in this way. Would always adding the alt attribute and only explicitly setting aria-hidden="true" when childArray.length === 1 make this more clear?

adobe-bot · 2019-12-11T18:56:29Z

Build successful! View the storybook

adobe-bot · 2019-12-12T23:00:58Z

Build successful! View the storybook

adobe-bot · 2019-12-13T15:39:04Z

Build successful! View the storybook

adobe-bot · 2019-12-13T21:26:15Z

Build successful! View the storybook

adobe-bot · 2019-12-16T18:55:49Z

Build successful! View the storybook

RSP-1360 Use `toBeTruthy` rather than `not.toBeNull`

- Fix gap between icon or necessityLabel With one child, and isRequired on the LabelBase: - remove the alt text from the required icon or set aria-hidden={true} to the necessityLabel span element. - ensure that the child also receives isRequired={true} so that it will communicate required state by adding aria-required="true" to the input. Otherwise, include alt text on the required icon or don't hide the necessityLabel with aria-hidden. If the LabelBase does not have isRequired, but isRequired is true for the child component, set isRequired on the LabelBase, and also set necessityIndicator = 'icon' if necessityIndicator is undefined on the LabelBase. RSP-1396 Use zero-width space to force whitespace before necessity label RSP-1396 Improve comments regarding aria-hidden on required label or icon Improve aria-required propagation to using Provider.

adobe-bot · 2019-12-16T21:34:02Z

Build successful! View the storybook

majornista requested review from dannify, devongovett and snowystinger as code owners December 6, 2019 00:54

majornista requested review from jnurthen and ktabors December 6, 2019 00:54

devongovett requested changes Dec 6, 2019

View reviewed changes

ktabors reviewed Dec 6, 2019

View reviewed changes

majornista force-pushed the RSP-1396 branch from 7d1dedd to 035ffc0 Compare December 9, 2019 20:52

majornista force-pushed the RSP-1396 branch from 035ffc0 to 75316a7 Compare December 9, 2019 22:00

majornista requested review from ktabors and devongovett December 9, 2019 22:01

LFDanLu requested changes Dec 10, 2019

View reviewed changes

packages/@adobe/spectrum-css-temp/components/fieldlabel/index.css Outdated Show resolved Hide resolved

packages/@react-spectrum/button/test/ActionButton.test.js Outdated Show resolved Hide resolved

majornista force-pushed the RSP-1396 branch from 75316a7 to a5eed3d Compare December 10, 2019 17:34

majornista force-pushed the RSP-1396 branch from 605432e to 182c37a Compare December 10, 2019 19:42

devongovett reviewed Dec 11, 2019

View reviewed changes

majornista requested review from devongovett and LFDanLu December 11, 2019 18:46

majornista force-pushed the RSP-1396 branch 3 times, most recently from 469d34e to 47f5e3e Compare December 12, 2019 22:55

dannify changed the title ~~RSP-1396 [Accessibility] Asterisk has ariaLabel and role presentation~~ [On-hold] RSP-1396 [Accessibility] Asterisk has ariaLabel and role presentation Dec 14, 2019

devongovett mentioned this pull request Dec 16, 2019

Builtin labels for TextField, TextArea, and SearchField #118

Merged

majornista added 2 commits December 16, 2019 16:22

RSP-1360 [Accessibility] v3 Icon should support role override

2d757bb

RSP-1360 Use `toBeTruthy` rather than `not.toBeNull`

majornista force-pushed the RSP-1396 branch from 37a0fb2 to 9a64ae4 Compare December 16, 2019 21:29

majornista closed this Dec 19, 2019

snowystinger deleted the RSP-1396 branch January 8, 2020 23:28

devongovett added a commit that referenced this pull request Jul 25, 2024

Build icons with parcel (#85)

ccd8619

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[On-hold] RSP-1396 [Accessibility] Asterisk has ariaLabel and role presentation #85

[On-hold] RSP-1396 [Accessibility] Asterisk has ariaLabel and role presentation #85

majornista commented Dec 6, 2019 •

edited

Loading

adobe-bot commented Dec 6, 2019

devongovett Dec 6, 2019

majornista Dec 6, 2019

devongovett Dec 6, 2019

majornista Dec 6, 2019 •

edited

Loading

ktabors Dec 6, 2019

majornista Dec 6, 2019

ktabors Dec 6, 2019

majornista Dec 6, 2019

majornista Dec 10, 2019

majornista Dec 10, 2019

adobe-bot commented Dec 9, 2019

adobe-bot commented Dec 9, 2019

adobe-bot commented Dec 10, 2019

adobe-bot commented Dec 10, 2019

adobe-bot commented Dec 10, 2019

adobe-bot commented Dec 10, 2019

devongovett Dec 11, 2019

devongovett Dec 11, 2019

majornista Dec 11, 2019

devongovett Dec 12, 2019

majornista Dec 12, 2019

devongovett Dec 12, 2019

majornista Dec 12, 2019

adobe-bot commented Dec 11, 2019

adobe-bot commented Dec 12, 2019

adobe-bot commented Dec 13, 2019

adobe-bot commented Dec 13, 2019

adobe-bot commented Dec 16, 2019

adobe-bot commented Dec 16, 2019

[On-hold] RSP-1396 [Accessibility] Asterisk has ariaLabel and role presentation #85

[On-hold] RSP-1396 [Accessibility] Asterisk has ariaLabel and role presentation #85

Conversation

majornista commented Dec 6, 2019 • edited Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Team:

adobe-bot commented Dec 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majornista Dec 6, 2019 • edited Loading

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

adobe-bot commented Dec 9, 2019

adobe-bot commented Dec 9, 2019

adobe-bot commented Dec 10, 2019

adobe-bot commented Dec 10, 2019

adobe-bot commented Dec 10, 2019

adobe-bot commented Dec 10, 2019

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

adobe-bot commented Dec 11, 2019

adobe-bot commented Dec 12, 2019

adobe-bot commented Dec 13, 2019

adobe-bot commented Dec 13, 2019

adobe-bot commented Dec 16, 2019

adobe-bot commented Dec 16, 2019

majornista commented Dec 6, 2019 •

edited

Loading

majornista Dec 6, 2019 •

edited

Loading