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

[EuiDatePicker] Add screenreader-only calendar week day labels #7748

Merged
merged 17 commits into from
May 20, 2024
Merged

[EuiDatePicker] Add screenreader-only calendar week day labels #7748

merged 17 commits into from
May 20, 2024

Conversation

mgadewoll
Copy link
Contributor

@mgadewoll mgadewoll commented May 13, 2024 •
edited

Summary

closes #5937

This PR adds screenreader-only full-length, i18n week day labels for the EuiDatePicker calendar.

QA

On the Storybook or EUI docs for EuiDatePicker review:

  • check the EuiDatePicker DOM output to see that the header cells now have two elements (visible short name with aria-hidden and an additional full-text name with euiScreenReaderOnly)
  • using a screen-reader navigate the EuiDatePicker and verify that the full-length week names are read and not the short form
  • change the locale to e.g. it and verify that the first day of the week is Monday (lu => lunedi) and not Sunday (In Storybook, if the calendar popover was opened, close it first to apply the control changes)

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA - N/A
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
    • If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist~ - N/A

@mgadewoll mgadewoll marked this pull request as ready for review May 13, 2024 14:44
@mgadewoll mgadewoll requested a review from a team as a code owner May 13, 2024 14:44
@mgadewoll mgadewoll requested a review from 1Copenut May 13, 2024 14:44
@mgadewoll
Copy link
Contributor Author

mgadewoll commented May 13, 2024 •
edited

@1Copenut Could you check the changes in NVDA and JAWS and verify if everything works as expected?

1Copenut
1Copenut approved these changes May 13, 2024
View reviewed changes
Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 LGTM!

I tested these new calendar labels with several pairings. All announced the full names correctly with no issue.

  • MacOS Safari + VO
  • Win10 Firefox + NVDA
  • Win10 Chrome + NVDA
  • Win10 Chrome + JAWS

@mgadewoll
Copy link
Contributor Author

👍 LGTM!

I tested these new calendar labels with several pairings. All announced the full names correctly with no issue.

  • MacOS Safari + VO
  • Win10 Firefox + NVDA
  • Win10 Chrome + NVDA
  • Win10 Chrome + JAWS

@1Copenut Awesome, thanks for checking! ❤️

cee-chen
cee-chen requested changes May 13, 2024
View reviewed changes
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Raising some blocking localization concerns here (see above comment)

@cee-chen
Copy link
Member

Changes look great, I'm so happy this ended up being super clean and locale-aware! Updating the unit tests is my only blocking change request left. If you don't get to it before the end of your work week I can help grab it and get this merged before next week's release, just let me know!

cee-chen
cee-chen approved these changes May 15, 2024
View reviewed changes
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

🚢 🎉

Not sure if we want to have Trevor test one more time in screen readers since logic changed very slightly (but not output?) - either way this is good to go from my perspective!

@mgadewoll
Copy link
Contributor Author

To be safe it might be good to have one last check, but I'd let @1Copenut decide if he would like to run it another time? 🏃

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@1Copenut
Copy link
Contributor

To be safe it might be good to have one last check, but I'd let @1Copenut decide if he would like to run it another time? 🏃

I ran it through the same set of screen readers. Logic feels good, and the full days announced themselves as they should.

@cee-chen
Copy link
Member

Thanks a million! Merging now so this gets into the release tomorrow.

@cee-chen cee-chen merged commit 4d032e0 into elastic:main May 20, 2024
5 checks passed
@mgadewoll mgadewoll mentioned this pull request May 21, 2024
Merged
mgadewoll added a commit to elastic/kibana that referenced this pull request May 22, 2024
@mgadewoll
Upgrade EUI to v94.5.1 (#183944)
8189e1a 
`v94.5.0-backport.1` ⏩ `v94.5.1`

[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)

---

## [`v94.5.1`](https://github.com/elastic/eui/releases/v94.5.1)

**Bug fixes**

- Fixed an `EuiDualRange`s with `showInput` bug, where `min`/`max`
values and invalid states were not being correctly set if values were
empty strings ([#7767](elastic/eui#7767))

**Accessibility**

- Improved `EuiDatePicker` and `EuiSuperDatePicker`'s time selection
screen reader UX ([#7726](elastic/eui#7726))
- Improved the accessibility of `EuiDatePicker` by providing full
screen-reader-only week day names to the calendar header
([#7748](elastic/eui#7748))
- Improved `EuiBadge`'s ability to tell when text within the badge is
selected/highlighted and selection color contrast
([#7752](elastic/eui#7752))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cee-chen cee-chen cee-chen approved these changes

@1Copenut 1Copenut 1Copenut approved these changes

Assignees
No one assigned
Labels
accessibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[EuiDatePicker][COGNITION]: Consider adding screen reader only text to the calendar day labels
5 participants
@mgadewoll @cee-chen @kibanamachine @elasticmachine @1Copenut