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): change calendar cells to buttons #24098

Closed
wants to merge 1 commit into from

Conversation

zarend
Copy link
Contributor

@zarend zarend commented Dec 14, 2021

Makes changes to the DOM structure of calendar cells. Previous, the DOM
structure looksed like this

Existing DOM structure of each calendar body cell

<td
 role="gridcell"
 aria-disabled="false"
 aria-current="date"
 aria-selected="true"
 <!-- ... -->
>
 <!-- additional details ommited -->
</>

Using the gridcell role allows screenreaders to use table specific
navigation and some screenreaders would announce that the cells are
interactible because of the presence of aria-selected. However, some
screenreaders did not announce the cells as interactable and treated it
the same as a cell in a static table (e.g. VoiceOver announces element
type incorrectly #23476).

This changes the DOM structure to use buttons
inside of a gridcell to make it more explicit that the table cells can
be interacted with and are not static content. The gridcell role is
still present to table navigation will continue to work, but the td
elements are now buttons. The gridcell wrapper is only for adding
information to the a11y tree and not used for visual purposes.

Updated DOM structure:

<div
  role="gridcell"
  style="display: contents;"
>
  <td
   role="button"
   aria-disabled="false"
   aria-current="date"
   aria-selected="true"
   <!-- ... -->
  >
   <!-- additional details ommited -->
  </>
<button>

I also tried another approach of keeping the <td/> as a
gridcell, and rendering a button inside of it. This turned out to be
much more complicated with getting the keyboard navigation and focusing
logic to work correctly. It also make writing the tests more complicated
because tests need to know if they should select the body cell or the
button nested inside of it. This approach also avoids interferring with
the styles.

Fixes #23476

@zarend zarend added Accessibility This issue is related to accessibility (a11y) area: material/datepicker labels Dec 14, 2021
@zarend zarend requested review from jelbourn and crisbeto December 14, 2021 01:39
@zarend
Copy link
Contributor Author

zarend commented Dec 14, 2021

Here's the first approach I tried, which is not working: #24099

Makes changes to the DOM structure of calendar cells. Previous, the DOM
structure looksed like this

Existing DOM structure of each calendar body cell

```
<td
 role="gridcell"
 aria-disabled="false"
 aria-current="date"
 aria-selected="true"
 <!-- ... -->
>
 <!-- additional details ommited -->
</>
```

Using the `gridcell` role allows screenreaders to use table specific
navigation and some screenreaders would announce that the cells are
interactible because of the presence of `aria-selected`. However, some
screenreaders did not announce the cells as interactable and treated it
the same as a cell in a static table (e.g. VoiceOver announces element
type incorrectly angular#23476).

This changes the DOM structure to use buttons
inside of a gridcell to make it more explicit that the table cells can
be interacted with and are not static content. The gridcell role is
still present to table navigation will continue to work, but the `td`
elements are now buttons. The gridcell wrapper is only for adding
information to the a11y tree and not used for visual purposes.

Updated DOM structure:

```
<div
  role="gridcell"
  style="display: contents;"
>
  <td
   role="button"
   aria-disabled="false"
   aria-current="date"
   aria-selected="true"
   <!-- ... -->
  >
   <!-- additional details ommited -->
  </>
<button>
```

I also tried another approach of keeping the `<td/>` as a
`gridcell`, and rendering a button inside of it. This turned out to be
much more complicated with getting the keyboard navigation and focusing
logic to work correctly. It also make writing the tests more complicated
because tests need to know if they should select the body cell or the
button nested inside of it. This approach also avoids interferring with
the styles.

Fixes angular#23476
@zarend zarend force-pushed the calendar-cell-buttons2 branch from ea919a8 to 6f86bae Compare December 14, 2021 01:40
@@ -26,8 +26,10 @@
[style.paddingBottom]="_cellPadding">
{{_firstRowOffset >= labelMinRequiredCells ? label : ''}}
</td>
<td *ngFor="let item of row; let colIndex = index"
role="gridcell"
<div role="gridcell" *ngFor="let item of row; let colIndex = index" style="display: contents;">
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 unfortunately invalid HTML- a <tr> element cannot directly contain a <div>.

We should be able to get something working with the <button> inside the gridcell- what did you run into with that approach?

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 start, it broke many tests because they needed to select the button inside the .mat-calendar-body-cell instead of the .mat-calendar-body-cell itself. Rather than running into specific problems, it seemed more like there could be an easier way to do this.

Wrapping the button with a display: contents; makes things easier because then the rest of the controller code and the styles only need to target the button, and do not need to be aware of the gridcell.

I can also try doing something like this, which should be valid html

<td role="gridcell" style="display: contents;">
  <div role="button" tabindex="0" class="mat-calendar-body-cell" ...>
    ...
  </div>
</td>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did run into styling problems by using the button tag instead of div role="button". But I would expect the styling to be fixable by adding more CSS rules.

@zarend
Copy link
Contributor Author

zarend commented Dec 15, 2021

Although this seems to work when I tested with VoiceOver screenreader, closing this because it's unfortunately invalid HTML. I'll try another way and open a new PR.

@zarend zarend closed this Dec 15, 2021
@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 15, 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) area: material/datepicker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(datepicker): It is announcing the date buttons type incorrectly as text elements
2 participants