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.

Sign up for GitHub

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

Jump to bottom

a11y: fix wrong role for day button #1708

Closed
wants to merge 6 commits into from
Closed

a11y: fix wrong role for day button #1708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e6e658e
chore(accessibility): update roles
Kosai106 Mar 2, 2023
47ffceb
Merge branch 'master' into chore/accessibility-roles
gpbl Mar 4, 2023
cad99ac
Remove unwanted rule
gpbl Mar 4, 2023
19af7e0
Merge branch 'master' into chore/accessibility-roles
gpbl Mar 4, 2023
1634c42
Merge branch 'chore/accessibility-roles' of github.com:Kosai106/react…
gpbl Mar 4, 2023
8bd5af6
Merge branch 'master' into chore/accessibility-roles
gpbl Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/react-day-picker/src/components/Day/Day.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('when the day to render has an hidden modifier', () => {
customRender(<Day {...props} />, dayPickerProps);
});
test('should render an empty grid cell', () => {
const cell = screen.getByRole('gridcell');
const cell = screen.getByRole('gridcell', { hidden: true });
expect(cell).toBeEmptyDOMElement();
});
});
Expand All @@ -52,7 +52,7 @@ describe('when a selection mode is set', () => {
customRender(<Day {...props} />, dayPickerProps);
});
test('should render a button named "day"', () => {
const cell = screen.getByRole('gridcell');
const cell = screen.getByRole('button');
expect(cell.nodeName).toBe('BUTTON');
expect(cell).toHaveAttribute('name', 'day');
});
Expand All @@ -66,7 +66,7 @@ describe('when "onDayClick" is present', () => {
customRender(<Day {...props} />, dayPickerProps);
});
test('should render a button', () => {
const cell = screen.getByRole('gridcell');
const cell = screen.getByRole('button');
expect(cell.nodeName).toBe('BUTTON');
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/react-day-picker/src/components/Day/Day.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export function Day(props: DayProps): JSX.Element {
const dayRender = useDayRender(props.date, props.displayMonth, buttonRef);

if (dayRender.isHidden) {
return <div role="gridcell"></div>;
return <div role="gridcell" aria-hidden="true"></div>;
}
if (!dayRender.isButton) {
return <div {...dayRender.divProps} />;
Expand Down
12 changes: 12 additions & 0 deletions packages/react-day-picker/src/components/Table/__snapshots__/Table.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ exports[`should render correctly 1`] = `
role="presentation"
>
<div
aria-hidden="true"
role="gridcell"
/>
</td>
Expand All @@ -82,6 +83,7 @@ exports[`should render correctly 1`] = `
role="presentation"
>
<div
aria-hidden="true"
role="gridcell"
/>
</td>
Expand All @@ -90,6 +92,7 @@ exports[`should render correctly 1`] = `
role="presentation"
>
<div
aria-hidden="true"
role="gridcell"
/>
</td>
Expand All @@ -98,6 +101,7 @@ exports[`should render correctly 1`] = `
role="presentation"
>
<div
aria-hidden="true"
role="gridcell"
/>
</td>
Expand All @@ -106,6 +110,7 @@ exports[`should render correctly 1`] = `
role="presentation"
>
<div
aria-hidden="true"
role="gridcell"
/>
</td>
Expand All @@ -114,6 +119,7 @@ exports[`should render correctly 1`] = `
role="presentation"
>
<div
aria-hidden="true"
role="gridcell"
/>
</td>
Expand Down Expand Up @@ -543,6 +549,7 @@ exports[`when showing the week numbers should render correctly 1`] = `
role="presentation"
>
<div
aria-hidden="true"
role="gridcell"
/>
</td>
Expand All @@ -551,6 +558,7 @@ exports[`when showing the week numbers should render correctly 1`] = `
role="presentation"
>
<div
aria-hidden="true"
role="gridcell"
/>
</td>
Expand All @@ -559,6 +567,7 @@ exports[`when showing the week numbers should render correctly 1`] = `
role="presentation"
>
<div
aria-hidden="true"
role="gridcell"
/>
</td>
Expand All @@ -567,6 +576,7 @@ exports[`when showing the week numbers should render correctly 1`] = `
role="presentation"
>
<div
aria-hidden="true"
role="gridcell"
/>
</td>
Expand All @@ -575,6 +585,7 @@ exports[`when showing the week numbers should render correctly 1`] = `
role="presentation"
>
<div
aria-hidden="true"
role="gridcell"
/>
</td>
Expand All @@ -583,6 +594,7 @@ exports[`when showing the week numbers should render correctly 1`] = `
role="presentation"
>
<div
aria-hidden="true"
role="gridcell"
/>
</td>
Expand Down
2 changes: 1 addition & 1 deletion packages/react-day-picker/src/hooks/useDayRender/useDayRender.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ export function useDayRender(
const buttonProps = {
...divProps,
disabled: activeModifiers.disabled,
role: 'gridcell',
role: 'button',
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this will break accessibility in some ways because button elements are not supposed to be children of grid > rowgroup. I would recommend testing any changes with a screen reader.

I've found that the structure that makes the most sense is to add a div with the role of gridcell and the button inside the gridcell. Additionally, using the aria-pressed attribute for the button when it is selected (since aria-selected is not valid attribute for button elements, and simply having aria-selected in the gridcell did not work in VoiceOver. [as in, it didn't announce that the day was selected]). Adding aria-pressed changes the buttons to toggle buttons, which seems appropriate since a date is either selected or not selected.

So

<table role="grid">
<!-- OMITTED HEAD -->
    <tbody role="rowgroup">
      <tr>
        <td role="presentation">
          <div role="gridcell">
            <button name="day" aria-label="April 1st, 2022" aria-pressed="false"  tabindex="0" type="button">1</button>
           </div>
        </td>
        <!-- !! SELECTED STATE -->
        <td role="presentation">
          <div role="gridcell" aria-selected="true">
            <button name="day" aria-label="April 1st, 2022" aria-pressed="true"  tabindex="0" type="button">1</button>
           </div>
        </td>
      </tr>
   </tbody>
</table>

Copy link
Contributor

Choose a reason for hiding this comment

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

You can possibly consider removing the div and changing role="presentation" to role="gridcell" in the td element. But please test in a screen reader.

Copy link
Owner

Choose a reason for hiding this comment

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

@GeorgeTaveras1231 thanks for your feedback here. Screen readers do read better the calendar when overriding roles, but the aXe test that doesn't like that.

I believe the correct HTML is to render a grid with grid cells being clickable, focusable, and "aria-selected". However, for some old design choices, we have now a button instead of a grid cell. Not easy to change the underlying HTML without introducing breaking changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gpbl 🤔 I have doubts about this approach. From my research it seems gridcell elements are grouping elements (except when the content is editable [think spreadsheet, for example]). In which case, clicking on the cells is just used to focus on the cell to allow the user to type. The way in which the DayPicker works, each Day behaves as a toggle button. Clicking on it, toggles the selected/unselected state. A gridcell by itself wont be able to communicate the sort of interactions each Day supports, as well as a toggle button can.

I also looked at the ARIA role hierarchy to support this point, where you can see interactive elements generally stem from the abstract widget role. There you'll see the button role, whereas the gridcell role inherits from section, which is a structure role, and is mainly for grouping content.

Resources:

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally you can consider making each day a checkbox or radio depending on the mode (single -> radio; multiple -> checkbox) 💡

['aria-selected']: activeModifiers.selected,
tabIndex: isFocused || isFocusTarget ? 0 : -1,
...eventHandlers
Expand Down
4 changes: 2 additions & 2 deletions packages/react-day-picker/test/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { screen } from '@testing-library/react';
import { format } from 'date-fns';

export function getDayButton(day: Date, index = 0) {
return screen.getAllByRole('gridcell', {
return screen.getAllByRole('button', {
name: day.getDate().toString()
})[index];
}
Expand Down Expand Up @@ -81,7 +81,7 @@ export function getMonthGrid(index = 0) {
}

export function queryMonthGrids() {
return screen.queryAllByRole('grid');
return screen.queryAllByRole('grid', { hidden: true });
}

export function getYearDropdown() {
Expand Down
6 changes: 1 addition & 5 deletions website/test/axe.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
import { configureAxe } from 'jest-axe';

export const axe = configureAxe({
rules: {
'aria-allowed-role': { enabled: false }
}
});
export const axe = configureAxe();