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

fix(material/datepicker): fix duplicate nav stop with Voiceover #24085

Merged

Conversation

zarend
Copy link
Contributor

@zarend zarend commented Dec 10, 2021

Fixes double nav stops when navigating the calendar-body with voiceover
. Fixes this by putting aria-hidden="true" on the
.mat-calendar-body-cell-preview, since that element is only visual,
and is not for giving semantic info to the a11y tree.

fixes #24082

Fixes double nav stops when navigating the calendar-body with voiceover
. Fixes this by putting `aria-hidden="true"` on the
`.mat-calendar-body-cell-preview`, since that element is only visual,
and is not for giving semantic info to the a11y tree.

fixes angular#24082
@zarend zarend added Accessibility This issue is related to accessibility (a11y) area: material/datepicker labels Dec 10, 2021
@@ -60,6 +60,6 @@
[class.mat-calendar-body-today]="todayValue === item.compareValue">
{{item.displayValue}}
</div>
<div class="mat-calendar-body-cell-preview"></div>
<div class="mat-calendar-body-cell-preview" aria-hidden="true"></div>
Copy link
Member

Choose a reason for hiding this comment

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

Does this behavior alternatively go away if we remove the content css property from .mat-calendar-body-cell-preview?

@crisbeto do you remember why this rule has the content property?

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing it's because I wanted to reuse the same styles with some ::before and ::after selectors and I didn't want to split it out only due to the content: https://github.com/angular/components/blob/master/src/material/datepicker/calendar-body.scss#L54.

This is a little bit surprising to me, I assumed that content only works on pseudo elements.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting note from MDN on the content property:

CSS-generated content is not included in the DOM. Because of this, it will not be represented in the accessibility tree and certain assistive technology/browser combinations will not announce it. If the content conveys information that is critical to understanding the page's purpose, it is better to include it in the main document.

https://developer.mozilla.org/en-US/docs/Web/CSS/content#accessibility_concerns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I tried removing the CSS content in #24095, but that did not fix the duplicate nav stop with VoiceOver

Copy link
Member

Choose a reason for hiding this comment

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

Weird, I wouldn't have expected an empty div to be picked up by a screen reader. I don't mind going with the aria-hidden here, but it would be good to understand why it happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To look for an explanation, narrowed it down to a minimally reproducing case. For some reason, the empty div must be absolutely positioned and the gridcell needs to have at least 2px of padding to reproduce.

<html lang="en">
  <head>
    <meta charset="utf-8" />
    <title>Basic datepicker</title>
    <style>
      .mat-calendar-body-cell-preview {
        position: absolute;
      }
      [role="gridcell"]       {
        padding: 2px;
      }
    </style>
  </head>
  <body>
    <table role="grid">
      <tbody>
        <tr role="row">
          <td role="gridcell" tabindex="-1" aria-label="December 12, 2021">
            <div>12</div>
            <div class="mat-calendar-body-cell-preview"></div>
          </td>
          <td role="gridcell" tabindex="-1" aria-label="December 13, 2021">
            <div>13</div>
            <div class="mat-calendar-body-cell-preview"></div>
          </td>
          <td role="gridcell" tabindex="-1" aria-label="December 18, 2021">
            <div>18</div>
            <div class="mat-calendar-body-cell-preview"></div>
          </td>
        </tr>
      </tbody>
    </table>
  </body>
</html>

Chrome has an unnecessary navigation stop with VoiceOver, but Firefox and Safari only have a single navigation stop.

Firefox a11y tree does not include a node for the empty div
image

but chrome does
image

The best explanation I can offer at this time is that we hit an edge case regarding empty a11y tree nodes being produced. I recommend going with this solution to add aria-hidden="true".

OS: macos 12.0.1 (21A559)
Chrome Version 96.0.4664.93 (Official Build) (x86_64)
Firefox 95.0 (64-bit)
Safari Version 15.1 (17612.2.9.1.20)

crisbeto added a commit to crisbeto/material2 that referenced this pull request Dec 12, 2021
Removes the `content` declaration from the tab chevrons. Usually it doesn't have an effect, but according to the discussion on angular#24085, it may cause an unnecessary tab stop.
@zarend zarend added the target: patch This PR is targeted for the next patch release label Dec 13, 2021
@zarend zarend requested a review from jelbourn December 14, 2021 21:50
@zarend
Copy link
Contributor Author

zarend commented Dec 14, 2021

@jelbourn , I took another pass at investigating why the unnecessary nav stop was happening in the first place, and the best explanation I can come up with at this time is that we hit a weird browser edge case. More details at #24085 (comment).

this is ready for you to review again pls.

amysorto pushed a commit that referenced this pull request Dec 14, 2021
Removes the `content` declaration from the tab chevrons. Usually it doesn't have an effect, but according to the discussion on #24085, it may cause an unnecessary tab stop.

(cherry picked from commit 6b79ea4)
amysorto pushed a commit that referenced this pull request Dec 14, 2021
Removes the `content` declaration from the tab chevrons. Usually it doesn't have an effect, but according to the discussion on #24085, it may cause an unnecessary tab stop.
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, good work creating a minimal reproduction

Can you file this as a Chrome bug with your repro?
https://bugs.chromium.org/p/chromium/issues/entry?components=UI%3EAccessibility&labels=Type-Bug,Pri-2

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Dec 15, 2021
@mmalerba mmalerba merged commit 795d849 into angular:master Dec 17, 2021
mmalerba pushed a commit that referenced this pull request Dec 17, 2021
Fixes double nav stops when navigating the calendar-body with voiceover
. Fixes this by putting `aria-hidden="true"` on the
`.mat-calendar-body-cell-preview`, since that element is only visual,
and is not for giving semantic info to the a11y tree.

fixes #24082

(cherry picked from commit 795d849)
@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 Jan 17, 2022
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 area: material/datepicker target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(datepicker): two navigation stops with Voiceover
4 participants