Skip to content

docs(a11y): Add a11y docs for table, expansion panel, dialog, chips, autocomplete… #6751

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

Merged
merged 2 commits into from
Sep 6, 2017

Conversation

tinayuangao
Copy link
Contributor

…, and select

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 31, 2017
@tinayuangao tinayuangao added Accessibility This issue is related to accessibility (a11y) docs This issue is related to documentation pr: needs review labels Aug 31, 2017
@@ -79,3 +79,7 @@ provides a tab-like UI for navigating between routes.
The tab-nav-bar is not tied to any particular router; it works with normal `<a>` elements and uses
the `active` property to determine which tab is currently active. The corresponding
`<router-outlet>` can be placed anywhere in the view.

### Accessibility
`MdTanNav`s without text or labels should be given a meaningful label via `aria-label` or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: MdTabNav.

@@ -150,3 +150,6 @@ Here are the available global options:
| Name | Type | Description |
| ----------------- | -------- | ----------- |
| errorStateMatcher | Function | Returns a boolean specifying if the error should be shown |

### Accessibility
The `mdInput` directive works with native `<input>` to provide an accessible experience.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also mention that if you add a placeholder attribute to the input or a md-placeholder element in the form field that placeholder text will automatically be used as the label for the input. If you don't specify any placeholder you need to add aria-label aria-labeledby or <label for=...>

Possibly also worth mentioning that any md-error and md-hint are automatically added to the input's aria-describedby

@@ -92,3 +92,11 @@ Here are the available global options:
- <kbd>DOWN_ARROW</kbd>: Focus next option
- <kbd>UP_ARROW</kbd>: Focus previous option
- <kbd>ENTER</kbd> or <kbd>SPACE</kbd>: Select focused item

### Accessibility
The select component without text or label should be given a meaningful label via
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, the placeholder, hint, and error stuff will work the same way as input after the refactor. For now this is fine though

@@ -38,3 +38,7 @@ also gain focus when clicked, ensuring keyboard navigation starts at the appropr
The selected color of an `<md-chip>` can be changed by using the `color` property. By default, chips
use a neutral background color based on the current theme (light or dark). This can be changed to
`'primary'`, `'accent'`, or `'warn'`.

### Accessibility
Chips has `role="option"` while chip list has `role="listbox"`. The chip input should have a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chips have role="option", while the chip list has...

@@ -126,3 +126,7 @@ that the AOT compiler knows to create the `ComponentFactory` for it.
})
export class AppModule() {}
```

### Accessibility
The dialog has `role="dialog". The dialog will focus on the first tabbable element when opened,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should mention that the dialog role can be overwritten to alertdialog in the appropriate context.

The expansion panel header has `role="button"`. The expansion panel header has attribute
`aria-controls` with the expansion panel's id as value.

Users can use keyboard to activate the expansion panel header to switch between expanded state
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"can use keyboard" -> "can use the keyboard".

`aria-label` or `aria-labelledby`.

The select component has `role="listbox"` and options inside select has `role="option"`.
Autocomplete trigger sets `aria-owns` to the autocomplete's id, and sets `aria-activedescendant` to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one talks about autocomplete instead of select. Also select doesn't use aria-activedescendant, but it moves focus between the options.


### Accessibility
The `aria-label` for sort button can be set in `MdSortHeaderIntl`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the aria-label for sort button" -> "the aria-label for the sort button".

@tinayuangao
Copy link
Contributor Author

Comments addressed. Please take another look. Thanks!

Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, a couple of minor nits.

@@ -126,3 +126,10 @@ that the AOT compiler knows to create the `ComponentFactory` for it.
})
export class AppModule() {}
```

### Accessibility
The dialog has `role="dialog", and dialog role can be overwritten to `alertdialog` in the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you're missing a backtick after the role.

The select component without text or label should be given a meaningful label via
`aria-label` or `aria-labelledby`.

The select component has `role="listbox"` and options inside select has `role="option"`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has -> have

The expansion panel header has `role="button"`. The expansion panel header has attribute
`aria-controls` with the expansion panel's id as value.

Users can use the keyboard to activate the expansion panel header to switch between expanded state
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth noting that as the panel headers are buttons, they are able to be navigated to via keyboard as well?

@tinayuangao
Copy link
Contributor Author

Comments addressed. Please take another look. Thanks!

@mmalerba
Copy link
Contributor

LGTM for input

@@ -95,3 +95,10 @@ desired display value. Then bind it to the autocomplete's `displayWith` property
</md-optgroup>
</md-autocomplete>
```

### Accessibility
The input for autocomplete without text or labels should be given a meaningful label via
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would also be good to mention that the autocomplete trigger is given role="combobox"

@@ -38,3 +38,7 @@ also gain focus when clicked, ensuring keyboard navigation starts at the appropr
The selected color of an `<md-chip>` can be changed by using the `color` property. By default, chips
use a neutral background color based on the current theme (light or dark). This can be changed to
`'primary'`, `'accent'`, or `'warn'`.

### Accessibility
Chips have role="option", while the chip list has `role="listbox"`. The chip input should have a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd phrase this first sentence as

A chip-list behaves as a `role="listbox"`, with each chip being a `role="option"`.


### Accessibility
Chips have role="option", while the chip list has `role="listbox"`. The chip input should have a
placeholder or be given a meaningful label via `aria-label` or `aria-labelledby`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me wonder whether we should somehow associate the chip-list and the input (Maybe aria-controls on the input? Not sure what that would do.).

Also I wonder whether having an input inside of a role="listbox" is problematic- how does the screen reader treat it now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The screen reader treats chip input and chip list as two separate components now.


### Accessibility
The dialog has `role="dialog"`, and dialog role can be overwritten to `alertdialog` in the
appropriate context.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, each dialog has `role="dialog"` on the root element. The role can be changed to
`alertdialog` via the `MdDialogConfig` when opening.

The `aria-label`, `aria-labelledby`, and `aria-describedby` attributes can all be set to the
dialog element via the `MdDialogConfig` as well. Each dialog should typically have a label
set via `aria-label` or `aria-labelledby`.

#### Focus management
By default, the first tabbable element within the dialog will receive focus upon open. 

Tabbing through the elements of the dialog will keep focus inside of the dialog element, 
wrapping back to the first tabbable element when reaching the end of the tab sequence.

(ariaLabel being added in #6558)

We can expand this in a later PR to mention the focus control attributes from cdkFocusTrap once we have documentation for that

@@ -98,3 +98,10 @@ panel can be expanded at a given time:

</md-accordion>
```

### Accessibility
The expansion panel header has `role="button"`. The expansion panel header has attribute
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a sentence at the beginning like

The expansion-panel aims to mimic the experience of the native `<details>` and `<summary>` elements.

`aria-controls` with the expansion panel's id as value.

The expansion panel headers are buttons. Users can use the keyboard to activate the expansion panel
header to switch between expanded state and collapsed state.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add another sentence

Because the header acts as a button, additional interactive elements
should not be put inside of the header.

Tables without text or labels should be given a meaningful label via `aria-label` or
`aria-labelledby`.

Table's default role is 'grid', and it can be changed through `role` attribute.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should mention that it can be changed to treegrid specifically, since that's the only other role that makes sense (being that it contains rows and gridcells)


### Accessibility
Tables without text or labels should be given a meaningful label via `aria-label` or
`aria-labelledby`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should mention aria-readonly regardless of what we do with it.

(it defaults to true if it's not set, but most people probably make readonly tables)

Tables without text or labels should be given a meaningful label via `aria-label` or
`aria-labelledby`.

Table's default role is 'grid', and it can be changed through `role` attribute.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably also worth mentioning that the md-table does not manage any focus/keyboard interaction on its own; if users want to add this, it can be added in their application.

@@ -79,3 +79,7 @@ provides a tab-like UI for navigating between routes.
The tab-nav-bar is not tied to any particular router; it works with normal `<a>` elements and uses
the `active` property to determine which tab is currently active. The corresponding
`<router-outlet>` can be placed anywhere in the view.

### Accessibility
`MdTabNav`s without text or labels should be given a meaningful label via `aria-label` or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true for either tab variant, not just the nav-tabs.

For nav-tabs, the <nav> element should probably have a label as well.


### Accessibility
`MdTabNav`s without text or labels should be given a meaningful label via `aria-label` or
`aria-labelledby`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and we should mention the keyboard shortcuts for md-tab-group

The dialog has `role="dialog"`, and dialog role can be overwritten to `alertdialog` in the
appropriate context.

The dialog will focus on the first tabbable element when opened,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot about escape, too:

#### Keyboard interaction
By default pressing the escape key will close the dialog. While this behavior can
be turned off via the `disableClose` option, users should generally avoid doing so
as it breaks the expected interaction pattern for screen-reader users.

### Accessibility
The `mdInput` directive works with native `<input>` to provide an accessible experience.

If a placeholder attributed is added to the input, or a `md-placeholder` element is added
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: attributed -> attribute

@tinayuangao
Copy link
Contributor Author

Comments addressed. Please take another look. Thanks!

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one last nit; add "merge-ready" when ready

`aria-labelledby`. For `MdTabNav`, the `<nav>` element should have a label as well.


### Keyboard shortcuts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be level 4 header ####

@tinayuangao tinayuangao added the action: merge The PR is ready for merge by the caretaker label Sep 2, 2017
@tinayuangao tinayuangao merged commit 5527e22 into angular:master Sep 6, 2017
@tinayuangao tinayuangao deleted the a11y-docs2 branch September 6, 2017 17:12
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Accessibility This issue is related to accessibility (a11y) action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement docs This issue is related to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants