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

feat(live-announcer): add ability to clear live element #11996

Merged
merged 1 commit into from
Nov 9, 2018

Conversation

crisbeto
Copy link
Member

Currently when we announce a message, we leave the element in place, however this can cause the element to be read out again when the user continues navigating using the arrow keys. These changes add an API where the consumer can clear the element manually or after a duration. Doing this automatically is tricky, because we'd have to make a lot of assumptions that might not be correct in all cases or may break some screen readers. These are some alternatives that were considered:

  • Removing the element from the a11y tree via aria-hidden after the screen reader has started announcing it. This works, but because of the politeness, we can't know exactly when the screen reader will announce the text, which could end up hiding it too early.
  • Clearing the element on the next keydown/click/focus. This could be flaky and might end up interrupting the screen reader when it doesn't have to (e.g. clicking on a non-focusable element).
  • Moving the element to a different place in the DOM (e.g. prepending it to the body instead of appending). This works, but we'd have to keep moving the element based on the focus direction.

Fixes #11991.

@crisbeto crisbeto added needs: discussion Further discussion with the team is needed before proceeding Accessibility This issue is related to accessibility (a11y) target: minor This PR is targeted for the next minor release labels Jun 30, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 30, 2018
@crisbeto
Copy link
Member Author

Setting this one as "needs discussion" since I'm open to alternate ways where we could do this automatically. Otherwise if we agree on the approach, it's good to go.

@@ -19,6 +19,6 @@ export class LiveAnnouncerDemo {
constructor(private live: LiveAnnouncer) {}

announceText(message: string) {
this.live.announce(message);
this.live.announce(message, undefined, 1500);
Copy link
Member

Choose a reason for hiding this comment

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

Should we try having a method overload? That way we can have a nicer API.

this.live.announce(message, 1500)
this.live.announce(message);
this.live.announce(message, 'polite', 1500);
this.live.announce(message, 'polite');

We did the same for the MatRipple launch method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. I'll add it once we agree on whether this is the way to go.

* @param politeness The politeness of the announcer element
* @param message Message to be announced to the screenreader.
* @param politeness The politeness of the announcer element.
* @param duration Time in milliseconds after which to clear out the announcer element.
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 it would be good to mention that this duration is being taken into account once the 100ms timeout finished.

@devversion
Copy link
Member

devversion commented Jun 30, 2018

Sounds good. From my perspective, the API and behavior looks fine. I think it's right to not assume that there is a perfect time to automatically clear the message.

We never know how long the screenreader takes to announce the actual message.

@crisbeto crisbeto force-pushed the 11991/aria-live-clear branch 2 times, most recently from 2a35827 to d88eb6f Compare June 30, 2018 10:57
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

Seems reasonable to me

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jul 9, 2018
@ngbot
Copy link

ngbot bot commented Jul 9, 2018

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure forbidden label detected: pr: needs*

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@jelbourn jelbourn removed the needs: discussion Further discussion with the team is needed before proceeding label Jul 9, 2018
@josephperrott josephperrott removed their request for review July 31, 2018 15:18
@ngbot
Copy link

ngbot bot commented Aug 1, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@crisbeto crisbeto force-pushed the 11991/aria-live-clear branch 2 times, most recently from 15e4549 to 22a3a69 Compare August 6, 2018 20:04
@vivian-hu-zz
Copy link
Contributor

@crisbeto need to rebase again.

Currently when we announce a message, we leave the element in place, however this can cause the element to be read out again when the user continues navigating using the arrow keys. These changes add an API where the consumer can clear the element manually or after a duration. Doing this automatically is tricky, because we'd have to make a lot of assumptions that might not be correct in all cases or may break some screen readers. These are some alternatives that were considered:

* Removing the element from the a11y tree via `aria-hidden` after the screen reader has started announcing it. This works, but because of the politeness, we can't know exactly when the screen reader will announce the text, which could end up hiding it too early.
* Clearing the element on the next `keydown`/`click`/`focus`. This could be flaky and might end up interrupting the screen reader when it doesn't have to (e.g. clicking on a non-focusable element).
* Moving the element to a different place in the DOM (e.g. prepending it to the `body` instead of appending). This works, but we'd have to keep moving the element based on the focus direction.

Fixes angular#11991.
@crisbeto
Copy link
Member Author

crisbeto commented Nov 8, 2018

Rebased.

@vivian-hu-zz vivian-hu-zz merged commit 4a1c8ed into angular:master Nov 9, 2018
@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 10, 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 target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CDK LiveAnnouncer region not hidden from screen reader
5 participants