fix(homepage): a11y errors #1483

0618 · 2022-03-05T00:20:13Z

Description of changes

before	after
	~~`![image](https://user-images.githubusercontent.com/11983489/156858311-0bc87028-429e-4b9f-a10c-7879075b82f5.png)`~~
6 X Empty form label	✅
1 X Language missing or invalid	✅
6 X Empty button	✅
5 X Broken ARIA reference	✅

Other code change

fix type error
rename variables
change format. double quote to single quote.

Note

There are still some a11y alerts on the homepage and I didn't fix them because:

1 X Missing first level heading: I'm not sure what to use as h1.
Redundant link: both links on the bottom are "https://docs.amplify.aws/console/"
2 X Noscript element: those are empty tags

TODO:

There are several primitives/components related a11y errors are converted in dde5608 and will be done in another PR fix a11y errors for Signin and FieldDescription #1495

Issue #, if available

https://app.asana.com/0/1201736086077838/1201759796183705/f

Description of how you validated changes

ran yarn test

Checklist

PR description included
yarn test passes
Tests are updated
Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

- remove redundant lable - change label to span - refactor and rename

changeset-bot · 2022-03-05T00:20:15Z

🦋 Changeset detected

Latest commit: 7a40db3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@aws-amplify/ui-react	Patch
@aws-amplify/ui	Patch
@aws-amplify/ui-vue	Patch
@aws-amplify/ui-angular	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

0618 · 2022-03-05T00:24:58Z

docs/src/pages/index.page.tsx

-                }
-                value={frameworkInstallScript}
-              />
+              <code className="install-code__container">


<code> tag might be more proper here

0618 · 2022-03-05T00:25:51Z

docs/src/components/Copy.tsx

+        size={size}
+        variation={variation}
+        isLoading={copied}
+        className={className}


add className because of the modification on docs/src/pages/index.page.tsx

0618 · 2022-03-05T00:26:40Z

docs/src/components/Layout/Header.tsx

@@ -113,6 +113,7 @@ export const Header = ({ platform, colorMode, setColorMode }) => {
        <Button
          className="docs-header-menu-button"
          onClick={() => setExpanded(!expanded)}
+          aria-label="Docs header menu button"


we may eventually use variables for aria-label for i18n.

Please use ariaLabel instead

0618 · 2022-03-05T00:27:15Z

docs/src/pages/_document.page.tsx

@@ -45,7 +45,7 @@ const getCSPContent = (context: Readonly<HtmlProps>) => {
 class MyDocument extends Document {
  render() {
    return (
-      <Html>
+      <Html lang="en-us">


we may eventually use variables for lang

Can you create a ticket for the docs for a language switcher if it doesn't already exist?

ticket cut: https://app.asana.com/0/1201840995372760/1201937080448144/f

0618 · 2022-03-05T00:30:14Z

packages/react/src/primitives/Menu/Menu.tsx

@@ -33,7 +33,7 @@ const MenuPrimitive: Primitive<MenuProps, 'div'> = (
    <DropdownMenuTrigger asChild={true}>
      {trigger ?? (
        <MenuButton
-          role="button"


not sure why we need to add it for a button, so removed it

Did it cause an accessibility error? I vaguely remember that without role="button" I couldn't use my own button with the Radix UI Dropdown Menu. Can you make sure removing this didn't break the Menu?

Nope, it doesn't cause any a11y error because it is a button. It would cause error if it's a <div> or something but uses as a button

Tested it and the menu button still works correctly. Proceed 👍

0618 · 2022-03-05T00:30:48Z

packages/react/src/primitives/Rating/RatingIcon.tsx

@@ -15,11 +15,9 @@ export const RatingIcon: React.FC<RatingIconProps> = ({
  className,
 }) => {
  return (
-    <View as="span" className={classNames(`amplify-rating-icon-container`)}>
-      <View as="label" className={classNames(`amplify-rating-label`)}>


not sure what's the label for, so removed it

Yeah, I'm not sure why there's a label here either. @jacoblogan Do you remember why we used a label here? We should take another look at the accessibility of this component. If we test this in VoiceOver does it read out the visually hidden text? Is there potentially a better way to label a rating component that we haven't seen yet?

I believe the label tag was a remnant of earlier designs when this component was not read only. Looking at some other rating components out there they use a label and hidden checkbox to mark user selections. @reesscot brought up a good point that we could have customers who are targeting this label for styling and so if we don't need to remove it, then it would be best to leave it in. @0618 other than the extra clutter, was the label causing any problems? If not then I agree that we should leave it in as to not create a potentially breaking change for customers.

@jacoblogan yes, "empty label" is a type of a11y error, so I removed it but kept the class selector in this PR. Is that ok?

Makes sense. If it's causing a11y error let's remove it.

0618 · 2022-03-05T00:31:11Z

packages/react/src/primitives/Rating/RatingMixedIcon.tsx

@@ -19,29 +19,29 @@ export const RatingMixedIcon: React.FC<RatingMixedIconProps> = ({
 }) => {
  const widthPercentage = `${(value % 1) * 100}%`;
  return (
-    <View as="span" className={classNames(`amplify-rating-icon-container`)}>


not sure why we need classNames here, so removed it

0618 · 2022-03-05T00:32:28Z

packages/ui/src/theme/css/component/rating.scss

@@ -35,7 +35,7 @@
  width: 1em;
 }

-.amplify-rating-label {
+.amplify-rating-icon-place {


since the label tag is changed to span, rename the class

docs/src/pages/index.page.tsx

reesscot

Thanks for fixing these issues. Have some change requests below:

reesscot · 2022-03-07T16:31:23Z

packages/ui/src/theme/css/component/rating.scss

@@ -35,7 +35,7 @@
  width: 1em;
 }

-.amplify-rating-label {
+.amplify-rating-icon-place {


I don't think we can rename classnames without a breaking change, since customers could be depending on them. These files affect customer apps and not just the docs.

Yes, making changes to class name are breaking changes. I believe my demo app has use some of the classnames for custom styling

Reverted back.

reesscot · 2022-03-07T16:32:37Z

packages/react/src/primitives/types/button.ts

+  /**
+   * Changes href of the button.
+   */
+  href?: string;


Not sure about this change. Most buttons are not a tags, so it seems misleading to offer an href prop for them. Where are we using a button with an href?

We are using it in some places on the docs site where we do <Button as={Link} or <Button as="a". I agree it is a bit weird to have href on the button type. I wonder if there is a better way to get a link that looks like a button?

+1.. Should we just use an a tag in the case?

Good point! I think we should not use buttons as anchor or vice versa. ~~The best way is to make button-link and link-button CSS styles.~~

I'll remove the type change here and add a ticket for refactor.

Updated. Revert change.

Cut a ticket to use Link to refactor the "button-links"

reesscot · 2022-03-07T16:34:48Z

packages/ui/src/theme/css/component/card.scss

@@ -7,17 +7,18 @@
  box-shadow: var(--amplify-components-card-box-shadow);
  display: inline-block;


Anyone know why cards are inline-block? Seems like they should just be display: block, then you wouldn't need the width:100%. @dbanksdesign thoughts?

Makes sense to me.

Changing this is a breaking change as well I think

Done. Changed to block

reesscot · 2022-03-07T16:51:08Z

packages/react/src/primitives/TextField/TextField.tsx

@@ -46,6 +46,8 @@ const TextFieldPrimitive = <Multiline extends boolean>(

  const fieldId = useStableId(id);
  const descriptionId = useStableId();
+  const labelId = useStableId();
+  const ariaDescribedBy = labelHidden ? labelId : descriptionId;


I'm guessing this is indented to fix an accessibility error where the description text is missing when labelHidden is true. I don't think it makes sense for a field to have the same text for label and description since it would be redundant for screen reader customers (might be read twice).

I'm seeing two accessibility issues here:

Current we hide the entire description if labelHidden is true (meaning that we hide valuable accessibility descriptions app developers could provide for users, even if it's visually hidden). We might as well still render the description, but hide it if labelHidden is true.

When app devs don't provide a description (which is most of the time), we still set ariaDescribedBy to an id that doesn't match an element in the DOM. I think this is the "5 X Broken ARIA reference" you are referring to, right?

I think the behavior should actually be changed to something like the following to fix these issues:

Update FieldDescription component to not return null when the labelHidden is true (it should still have a description, just be wrapped in VisuallyHidden component). We'll need to verify this doesn't break any styling, it may affect the field when flex direction is row (thinking it may leave extra room).

Change above logic to set ariaDescribedBy to undefined when description is empty (there's no reason to have an ariaDescribedBy prop id set if there's no description text. And there is usually no description text.

I agree @reesscot's fixes

Yup, agree! Thanks for the findings here and fixing this

Updated! Converted the change and hide FieldDescription if labelHidden is true, instead of FieldDescription returning null.

reesscot · 2022-03-07T16:58:15Z

packages/react/src/primitives/Rating/RatingIcon.tsx

@@ -15,11 +15,9 @@ export const RatingIcon: React.FC<RatingIconProps> = ({
  className,
 }) => {
  return (
-    <View as="span" className={classNames(`amplify-rating-icon-container`)}>
-      <View as="label" className={classNames(`amplify-rating-label`)}>


Yeah, I'm not sure why there's a label here either. @jacoblogan Do you remember why we used a label here? We should take another look at the accessibility of this component. If we test this in VoiceOver does it read out the visually hidden text? Is there potentially a better way to label a rating component that we haven't seen yet?

reesscot · 2022-03-07T17:14:40Z

packages/react/src/components/Authenticator/SignIn/SignIn.tsx

@@ -61,6 +61,7 @@ export function SignIn() {
            className="amplify-flex"
            disabled={isPending}
          >
+            <legend>Sign in</legend>


Will this be translated? Also, this changes the design of the Authenticator. Should this be visually hidden @ErikCH @wlee221

Yes this will change the design of Authenticator. It would be better if you can ping them to take a look at

Does this fix an accessibility issue?

Yes, there's an alert that fieldset missing legend

I forgot to add a amplify-visually-hidden class...I'll do that

Yep, adding visually-hidden should be sufficient.

Updated. Added amplify-visually-hidden

docs/src/pages/index.page.tsx

docs/src/pages/HomePrimitivePreview.tsx

reesscot · 2022-03-07T17:21:26Z

docs/src/components/Layout/Header.tsx

@@ -113,6 +113,7 @@ export const Header = ({ platform, colorMode, setColorMode }) => {
        <Button
          className="docs-header-menu-button"
          onClick={() => setExpanded(!expanded)}
+          aria-label="Docs header menu button"


Please use ariaLabel instead

reesscot · 2022-03-07T17:23:25Z

packages/react/src/primitives/Rating/RatingMixedIcon.tsx

-        as="label"
-        className={classNames(`amplify-rating-label`)}
+        as="span"
+        className="amplify-rating-icon-place"


Please don't change this classname

zchenwei

I am just half way through it. Publishing a few comments first

zchenwei · 2022-03-07T19:13:43Z

docs/src/pages/index.page.tsx

@@ -25,6 +25,8 @@ import { HomePrimitivePreview } from './HomePrimitivePreview';
 import { Copy } from '@/components/Copy';
 import { Footer } from '@/components/Layout/Footer';

+import type { SandpackThemeProp } from '@codesandbox/sandpack-react';


FYI, we are going to remove sandpack in favor of react-live. #1395 @dbanksdesign

potentially... there are some potential security issues with it since it uses eval, still up for debate

zchenwei · 2022-03-07T19:22:57Z

packages/react/src/components/Authenticator/SignIn/SignIn.tsx

@@ -61,6 +61,7 @@ export function SignIn() {
            className="amplify-flex"
            disabled={isPending}
          >
+            <legend>Sign in</legend>


Yes this will change the design of Authenticator. It would be better if you can ping them to take a look at

Co-authored-by: Scott Rees <[email protected]>

…o fix-a11y-home

dbanksdesign

LGTM! Just one non-blocking comment

dbanksdesign · 2022-03-08T19:47:43Z

packages/react/src/primitives/Field/FieldDescription.tsx

+}) => (
+  <Text
+    className={classNames(ComponentClassNames.FieldDescription, {
+      'amplify-visually-hidden': labelHidden,


This could be ComponentClassNames.VisuallyHidden

jacoblogan · 2022-03-08T19:54:45Z

.changeset/quiet-impalas-chew.md

@@ -0,0 +1,8 @@
+---
+'@aws-amplify/ui-react': patch


with the new attributes being added and the structure of some of the components being updated do we want this to be a minor bump instead of a patch?

Good point...

I checked the semver doc

Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API. It MUST be incremented if any public API functionality is marked as deprecated. It MAY be incremented if substantial new functionality or improvements are introduced within the private code. It MAY include patch level changes. Patch version MUST be reset to 0 when minor version is incremented.

Since we don't have any new features in this PR, I think it's not a MUST.

…fy-visually-hidden

jacoblogan

LGTM

reesscot · 2022-03-08T21:09:02Z

packages/react/src/components/Authenticator/SignIn/SignIn.tsx

@@ -61,6 +61,9 @@ export function SignIn() {
            className="amplify-flex"
            disabled={isPending}
          >
+            <VisuallyHidden>
+              <legend>Sign in</legend>


Please split this change out into a separate PR and include the Vue and Angular SignIn screens as well. I'd like this to be reviewed in a non-docs change PR since it's more about Authenticator accessibility than strictly the docs.

reesscot · 2022-03-08T21:19:41Z

packages/react/src/primitives/SliderField/SliderField.tsx

@@ -50,6 +50,7 @@ const SliderFieldPrimitive: Primitive<SliderFieldProps, typeof Root> = (
  const fieldId = useStableId(id);
  const labelId = useStableId();
  const descriptionId = useStableId();
+  const ariaDescribedBy = labelHidden ? labelId : descriptionId;


Suggested change

const ariaDescribedBy = labelHidden ? labelId : descriptionId;

const ariaDescribedBy = descriptiveText ? descriptionId : undefined; // only set `ariaDescribedBy` if descriptiveText is provided

Please also add a unit test for this.

~~Weird...I removed the code in this commit 94bb127 but it still shows up in the latest PR...~~

NVM, I missed SliderField.tsx, only changed TextField.tsx

Actually, can we please split these changes out into a separate PR, as we will need to make these changes in all the Field components that use the FieldDescription component.

reesscot · 2022-03-08T21:27:16Z

packages/react/src/primitives/TextField/TextField.tsx

@@ -98,7 +99,7 @@ const TextFieldPrimitive = <Multiline extends boolean>(
      testId={testId}
      {...flexContainerStyleProps}
    >
-      <Label htmlFor={fieldId} visuallyHidden={labelHidden}>
+      <Label htmlFor={fieldId} visuallyHidden={labelHidden} id={labelId}>


Why do we need this? the htmlFor is handling the link to the input field.

reesscot · 2022-03-08T21:30:35Z

packages/react/src/primitives/Rating/RatingIcon.tsx

@@ -15,11 +15,9 @@ export const RatingIcon: React.FC<RatingIconProps> = ({
  className,
 }) => {
  return (
-    <View as="span" className={classNames(`amplify-rating-icon-container`)}>
-      <View as="label" className={classNames(`amplify-rating-label`)}>


Makes sense. If it's causing a11y error let's remove it.

reesscot · 2022-03-08T21:31:00Z

packages/react/src/primitives/Menu/Menu.tsx

@@ -33,7 +33,7 @@ const MenuPrimitive: Primitive<MenuProps, 'div'> = (
    <DropdownMenuTrigger asChild={true}>
      {trigger ?? (
        <MenuButton
-          role="button"


Tested it and the menu button still works correctly. Proceed 👍

reesscot · 2022-03-08T21:32:01Z

packages/react/src/primitives/SliderField/SliderField.tsx

@@ -50,6 +50,7 @@ const SliderFieldPrimitive: Primitive<SliderFieldProps, typeof Root> = (
  const fieldId = useStableId(id);
  const labelId = useStableId();
  const descriptionId = useStableId();
+  const ariaDescribedBy = labelHidden ? labelId : descriptionId;


Actually, can we please split these changes out into a separate PR, as we will need to make these changes in all the Field components that use the FieldDescription component.

lgtm-com · 2022-03-08T22:07:40Z

This pull request introduces 1 alert when merging dde5608 into 8ead973 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

reesscot · 2022-03-08T22:37:42Z

packages/react/src/primitives/Field/FieldDescription.tsx

@@ -1,4 +1,5 @@
 import * as React from 'react';
+import classNames from 'classnames';


remove unused import

Thanks for catching! removed

reesscot

LGTM, just one small change (removing a now unused import)

0618 added 7 commits March 4, 2022 18:27

fix (homepage): a11y empty buttons

9a73792

fix(docs homepage): a11y for install copy

d6eeaf1

fix(homepage): a11y issue of the empty label from install code

73dc138

fix (homepage): a11y for rating icons

e9a2636

- remove redundant lable - change label to span - refactor and rename

fix some type error

1d2b644

fix(homepage): a11y ARIA reference

2ee9a60

fix(homepage): a11y langguage missing

212de8a

fix(homepage): a11y fieldset description

365f6eb

0618 force-pushed the fix-a11y-home branch from 43d1ce3 to 365f6eb Compare March 5, 2022 00:29

0618 had a problem deploying to ci March 5, 2022 00:40 Error

0618 had a problem deploying to ci March 5, 2022 00:41 Error

update changeset

2c07563

0618 commented Mar 5, 2022

View reviewed changes

0618 temporarily deployed to ci March 5, 2022 00:55 Inactive

reesscot suggested changes Mar 7, 2022

View reviewed changes

zchenwei reviewed Mar 7, 2022

View reviewed changes

Update docs/src/pages/HomePrimitivePreview.tsx

b1222e5

Co-authored-by: Scott Rees <[email protected]>

0618 temporarily deployed to ci March 8, 2022 00:48 Inactive

0618 added 2 commits March 8, 2022 04:32

resolve review comments

1a16af5

Merge branch 'fix-a11y-home' of github.com:aws-amplify/amplify-ui int…

ca834d4

…o fix-a11y-home

0618 temporarily deployed to ci March 8, 2022 05:13 Inactive

0618 temporarily deployed to ci March 8, 2022 18:43 Inactive

add ariaLabel to MenuButton

b948437

0618 temporarily deployed to ci March 8, 2022 19:41 Inactive

dbanksdesign approved these changes Mar 8, 2022

View reviewed changes

jacoblogan reviewed Mar 8, 2022

View reviewed changes

refactor: use ComponentClassNames.VisuallyHidden var to replace ampli…

20f84aa

…fy-visually-hidden

jacoblogan approved these changes Mar 8, 2022

View reviewed changes

0618 temporarily deployed to ci March 8, 2022 20:23 Inactive

reesscot reviewed Mar 8, 2022

View reviewed changes

reesscot suggested changes Mar 8, 2022

View reviewed changes

convert primitive related changes. Will do in another PR

dde5608

0618 temporarily deployed to ci March 8, 2022 22:11 Inactive

0618 mentioned this pull request Mar 8, 2022

fix a11y errors for Signin and FieldDescription #1495

Merged

4 tasks

reesscot reviewed Mar 8, 2022

View reviewed changes

reesscot approved these changes Mar 8, 2022

View reviewed changes

remove unused import

7a40db3

0618 merged commit b97cb91 into main Mar 8, 2022

0618 deleted the fix-a11y-home branch March 8, 2022 23:09

github-actions bot mentioned this pull request Mar 8, 2022

Version Packages #1477

Merged

		@@ -7,17 +7,18 @@
		box-shadow: var(--amplify-components-card-box-shadow);
		display: inline-block;

	const ariaDescribedBy = labelHidden ? labelId : descriptionId;
	const ariaDescribedBy = descriptiveText ? descriptionId : undefined; // only set `ariaDescribedBy` if descriptiveText is provided

		@@ -1,4 +1,5 @@
		import * as React from 'react';
		import classNames from 'classnames';

fix(homepage): a11y errors #1483

fix(homepage): a11y errors #1483

Conversation

0618 commented Mar 5, 2022 • edited Loading

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

changeset-bot bot commented Mar 5, 2022 • edited Loading

🦋 Changeset detected

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

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

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

reesscot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zchenwei Mar 7, 2022 • 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

0618 Mar 8, 2022 • 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

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

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

zchenwei left a 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

dbanksdesign left a 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

jacoblogan left a 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

0618 Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0618 commented Mar 5, 2022 •

edited

Loading

changeset-bot bot commented Mar 5, 2022 •

edited

Loading

zchenwei Mar 7, 2022 •

edited

Loading

0618 Mar 8, 2022 •

edited

Loading

0618 Mar 8, 2022 •

edited

Loading