Skip to content
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

docs(a11y): Adding accessibility sections to datepicker, menu, slide … #6710

Merged
merged 5 commits into from
Sep 1, 2017

Conversation

tinayuangao
Copy link
Contributor

…toggle, slider, snackbar and toolbar

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 29, 2017
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

One nit, otherwise LGTM

@@ -72,3 +72,7 @@ export class MessageArchivedComponent {
constructor(@Inject(MD_SNACK_BAR_DATA) public data: any) { }
}
```
### Accessibility
A Snackbar automatically goes away after a short time, so you can't count on the user seeing the
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: To be consistent with the rest of the doc, use snack-bar instead of Snackbar ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to snack-bar. Thanks for review.

@kara kara added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Aug 29, 2017
@kara
Copy link
Contributor

kara commented Aug 29, 2017

LGTM

@@ -228,3 +228,6 @@ application root module.
})
export class MyApp {}
```
### Accessibility
The input box for datepicker should have a placeholder or 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.

I'd like to expand this to describe what the datepicker does in general (input with aria-haspopup that triggers a role="dialog") along with the keyboard shortcuts, but that can go in another PR.

@@ -228,3 +228,6 @@ application root module.
})
export class MyApp {}
```
### Accessibility
The input box for datepicker 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.

We should mention that MdDatepickerIntl includes strings that are used for aria-labels.

@@ -72,3 +72,7 @@ export class MessageArchivedComponent {
constructor(@Inject(MD_SNACK_BAR_DATA) public data: any) { }
}
```
### Accessibility
A snack-bar automatically goes away after a short time, so you can't count on the user seeing the
Copy link
Member

Choose a reason for hiding this comment

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

I would phrase this as

Snack-bar messages are announced via an `aria-live` region. Focus is not moved to
the snack-bar element, as this would be disruptive to a user in the middle of a
workflow. For any action offered in the snack-bar, the application should offer the
user an alternative way to perform the action (typically via keyboard shortcut).

@@ -49,3 +49,7 @@ easily accomplished with `display: flex`:
The color of a `<md-toolbar>` can be changed by using the `color` property. By default, toolbars
use a neutral background color based on the current theme (light or dark). This can be changed to
`'primary'`, `'accent'`, or `'warn'`.

### Accessibility
Toolbar items 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.

I think this isn't quite right. Similar to list and grid-list, we treat toolbar as decorative-only by default. Generally, the toolbar is used as a header where role="header" would be appropriate.

Only if the use-case of the toolbar match that of role="toolbar", the user should add the role and an appropriate label.

@jelbourn jelbourn removed the action: merge The PR is ready for merge by the caretaker label Aug 30, 2017
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

@tinayuangao
Copy link
Contributor Author

Comments addressed. Please take another look. Thanks!


#### Keyboard shortcuts
The keyboard shortcuts to handle datepicker are:
`ALT` + `DOWN_ARROR`: trigger calendar dialog
Copy link
Member

Choose a reason for hiding this comment

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

ARROR -> ARROW

should have a placeholder or be given a meaningful label via `aria-label`, `aria-labelledby` or
`MdDatepickerIntl`.

#### 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.

It would be good to put these in a table, e.g.

| Shortcut             | Action                    |
|----------------------|---------------------------|
| `ALT` + `DOWN_ARROW` | Open the calendar pop-up  |
| `ESCAPE`             | Close the calendar pop-up |

#### Keyboard shortcuts
The keyboard shortcuts to handle datepicker are:

`ALT` + `DOWN_ARROR`: trigger calendar dialog
Copy link
Member

Choose a reason for hiding this comment

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

ARROR -> ARROW

@mmalerba
Copy link
Contributor

LGTM on datepicker and slider

@tinayuangao
Copy link
Contributor Author

Put shortcuts in tables. Please take another look. Thanks!

| `LEFT_ARROW` | Goes to previous day |
| `RIGHT_ARROW` | Goes to next day |
| `UP_ARROW` | Goes to previous 7 days |
| `DOWN_ARROW` | Goes to next 7 days |
Copy link
Member

Choose a reason for hiding this comment

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

Goes -> Go for each of these

For up and down arrow, I'd say Go to same day in the next week

| `UP_ARROW` | Goes to previous 7 days |
| `DOWN_ARROW` | Goes to next 7 days |
| `HOME` | Focus the first day of the month |
| `END` | Focus the last day of the month |
Copy link
Member

Choose a reason for hiding this comment

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

Why say Focus for home/end and Goes / Go everywhere else?

@tinayuangao
Copy link
Contributor Author

Changed to Go to everywhere. 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

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Aug 31, 2017
@jelbourn jelbourn merged commit 1b6b270 into angular:master Sep 1, 2017
josephperrott pushed a commit to josephperrott/components that referenced this pull request Sep 5, 2017
josephperrott pushed a commit to josephperrott/components that referenced this pull request Sep 5, 2017
josephperrott pushed a commit to josephperrott/components that referenced this pull request Sep 6, 2017
josephperrott pushed a commit to josephperrott/components that referenced this pull request Sep 12, 2017
josephperrott pushed a commit to josephperrott/components that referenced this pull request Sep 15, 2017
josephperrott pushed a commit to josephperrott/components that referenced this pull request Sep 29, 2017
josephperrott pushed a commit to josephperrott/components that referenced this pull request Oct 5, 2017
josephperrott pushed a commit to josephperrott/components that referenced this pull request Oct 12, 2017
josephperrott pushed a commit to josephperrott/components that referenced this pull request Oct 16, 2017
@tinayuangao tinayuangao deleted the a11y-docs branch March 9, 2018 23:32
@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 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants