Skip to content

feat(material/datepicker): Add aria-current="date" to current date #23714

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
Merged
merged 1 commit into from
Nov 15, 2021
Merged

feat(material/datepicker): Add aria-current="date" to current date #23714

merged 1 commit into from
Nov 15, 2021

Conversation

ByzantineFailure
Copy link
Contributor

Closes #23713

@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 8, 2021
@ByzantineFailure
Copy link
Contributor Author

I am reasonably certain that the unit test added here for this functionality could be better but am not quite certain of the best way forward.

Right now it checks that the td which contains the current date has a child with the today class and gets the value of the date to ensure it's today. This may be better tested in the "today" unit test or via some other method. Just feels brittle.

If there are thoughts around how to do this better please let me know!

@ByzantineFailure ByzantineFailure force-pushed the datepicker-aria-current branch 2 times, most recently from 521de2a to 887012c Compare October 8, 2021 17:46
@jelbourn jelbourn added Accessibility This issue is related to accessibility (a11y) G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround labels Oct 8, 2021
jelbourn
jelbourn reviewed Oct 8, 2021
View reviewed changes
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.

Thanks for the PR!

src/material/datepicker/calendar-body.spec.ts Outdated
@@ -55,6 +55,13 @@ describe('MatCalendarBody', () => {
expect(todayCell.innerHTML.trim()).toBe('3');
});

it('sets aria-current="date" on today', () => {
const todayCell = calendarBodyNativeElement.querySelector(
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 also test:

  • When today is in the active month, that there's exactly one cell with aria-current
  • When today is not in the active month, that there's exactly zero cells with aria-current

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly enough:

todayValue in MatCalendarBody is a number. Its value is passed in from MatMonthView with ! non-null assertion. In MatMonthView, today not being within the month is represented by this value being null.

I can replicate this setup in testing by simply setting the testing component's today value to be number|null. I'm going to do that here, but if it makes sense would it be fine by you to retype MatCalendarBody's todayValue @Input from number to number|null? Right now we're explicitly passing null in via non-null assertion which feels a bit unfortunate.

Copy link
Contributor Author

@ByzantineFailure ByzantineFailure Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Mea culpa:

todayValue is taken via _dateAdapter.today() converted to a number via _getCellCompareValue. This function generates a new Date object via the year, month, and day from the passed in DateAdapter. DateAdapter does not allow null to be returned from this method, so I was incorrect.

I'll update the tests to work utilize a timestamp outside the specified month (which I probably should have done to begin with).

@ByzantineFailure ByzantineFailure force-pushed the datepicker-aria-current branch 2 times, most recently from 0e3af66 to 173d480 Compare October 11, 2021 22:24
@jelbourn jelbourn added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed labels Oct 18, 2021
@jelbourn
Copy link
Member

Caretaker note: this should be a fix

@jelbourn jelbourn added the target: patch This PR is targeted for the next patch release label Oct 18, 2021
jelbourn
jelbourn reviewed Oct 18, 2021
View reviewed changes
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.

Looks good, just one last nit

src/material/datepicker/calendar-body.spec.ts Outdated
const todayCell = todayCells[0];

expect(todayCell).not.toBeNull();
expect(todayCell.innerHTML.trim()).toBe('3');
Copy link
Member

Choose a reason for hiding this comment

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

Prefer textContent here to innerHTML
(similar for other tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about the delay getting back to this! This issue should be resolved now.

@ByzantineFailure ByzantineFailure force-pushed the datepicker-aria-current branch 2 times, most recently from 3605274 to 60cffce Compare October 20, 2021 19:05
@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Oct 21, 2021
@mmalerba mmalerba mentioned this pull request Nov 15, 2021
Closed
@mmalerba mmalerba force-pushed the datepicker-aria-current branch from 60cffce to 9889151 Compare November 15, 2021 19:11
@mmalerba mmalerba force-pushed the datepicker-aria-current branch from 9889151 to 1e815ac Compare November 15, 2021 19:13
@mmalerba mmalerba merged commit 6f1a1b7 into angular:master Nov 15, 2021
mmalerba pushed a commit that referenced this pull request Nov 15, 2021
@ByzantineFailure @mmalerba
fix(material/datepicker): Add aria-current="date" to current date (#2…
ba5d36a 
…3714)

(cherry picked from commit 6f1a1b7)
This was referenced Dec 1, 2021
Closed
Closed
@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 Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn left review comments

@mmalerba mmalerba Awaiting requested review from mmalerba

Assignees
No one assigned
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 G This is is related to a Google internal issue merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat(material/datepicker): Apply aria-current="date" to the current date's cell
3 participants
@ByzantineFailure @jelbourn @mmalerba