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

2390 aria #2943

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

2390 aria #2943

wants to merge 9 commits into from

Conversation

Eonasdan
Copy link
Owner

@Eonasdan Eonasdan commented Dec 18, 2024

Adds ARIA bits

For testing and features, see this stackblitz

@Eonasdan Eonasdan marked this pull request as ready for review December 19, 2024 14:53
Copy link

@esoergel esoergel left a comment

Choose a reason for hiding this comment

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

I did some preliminary testing (Firefox 133.0.3 on Ubuntu). I made a basic HTML page with popper, font awesome, and bootstrap loaded from CDNs, with tempus dominus JS served locally. The body of the page then contained the html from the Simple example in the docs.

Could you make the toggle element keyboard navigable? At present I think you have to use the mouse to open the widget, though setting tabindex="0" on the data-td-toggle="datetimepicker" seems to do the trick.

What would you think about auto-closing the widget once a date or range has been selected, at least via keyboard navigation? The single date widget closes automatically when a date is selected, which seems like it would be nice here too, though that's more a matter of opinion.

if (this.optionsStore.toggle) {
this.optionsStore.toggle.ariaLabel = `${
this.optionsStore.options.localization.toggleAriaLabel
}, ${date.format()}`;
Copy link

Choose a reason for hiding this comment

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

This triggers a date is undefined error when testing with date ranges. Here's the config I used for testing:

new tempusDominus.TempusDominus(document.getElementById('datetimepicker1'), {
    dateRange: true,
    display: {
        components: {
            clock: false,
            year: true,
        },
        buttons: {
            clear: true,
            close: true,
        },
    },
    localization: {
        format: 'yyyy-MM-dd',
    },
});
  • Click on the calendar icon
  • The daterange picker pops up with the startdate already selected as today
  • Select an end date - the input widget is populated as expected
  • Click the trash icon or any other date, and a date is undefined error is thrown

Copy link

Choose a reason for hiding this comment

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

Actually I also see this on the single date version of the widget - select a date, then click the trash icon.

Copy link
Owner Author

Choose a reason for hiding this comment

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

The auto closing should work the same way with either keyboard or mouse nav. I'll have to check on this stuff. Thanks for feedback.

Copy link

Choose a reason for hiding this comment

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

for sure - let me know if there's anything you'd like me to retest or if there are further details I can provide.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think I fixed the label bug, check out this stackblitz.

The picker does auto close with the keyboard, but I think the label issue you pointed out was causing it to stay open.

Copy link
Owner Author

Choose a reason for hiding this comment

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

And yes adding tabindex="0" will be required if you want people to tab from the input to the icon. That's not something I will do automagically. I think there would be to many edge cases.

Copy link

Choose a reason for hiding this comment

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

yes adding tabindex="0" will be required if you want people to tab from the input to the icon

My bad, I wasn't paying attention to where that icon came from. You're absolutely right.

Running this locally I can confirm the label fix. Thanks!

The picker does auto close with the keyboard

Just dug into this a bit more, I see keyboard and mouse behaving the same, as you stated. I think this is a difference between dateRange: true and daterange: false. With dateRange: false, the picker auto closes when keepOpen is false or not specified, and stays open when it's true. However, with dateRange: true, the picker stays open regardless of the value set on keepOpen. Looking at the code, it looks like auto close is disabled explicitly for date ranges and multiple dates:
https://github.com/Eonasdan/tempus-dominus/blob/master/src/js/actions.ts#L341

Changing that line like this makes it close as I'd expect when the second date is selected:

-      !this.optionsStore.options.dateRange
+      !(this.optionsStore.options.dateRange && this.dates.picked.length < 2)

This behavior isn't new or specific to keyboard navigation though, and so arguably doesn't belong in this PR. If you're open to that change or have another suggestion, I'd be happy to take a crack at it in another PR.

Anyways, this PR all looks great to me, I'll work on getting it in the hands of our QA team to bang away at it.

@esoergel
Copy link

Our QA team has been playing around with this. It looks like with a screen reader enabled, the keyboard navigation works differently. They tested on Windows with NVDA screen reader and Chrome, and I did some more testing on Linux with the Orca screen reader and Chrome.

Without the screen reader on, tabbing around moves between different parts of the widget. With it enabled, tab moves outside the element (sometimes - it seems like there's some state dependency), leaving the widget open with no way to dismiss it.

Without the screen reader on, the arrow keys navigate cleanly through the days available. With it enabled, keyboard navigation is broken. I see that clicking up or down cause the selector to move to the start of the previous or next row and read out all numbers on that row. Clicking left or right reads the left or right digit of the current date. Clicking it again moves to the cursor to the next date, reading the first digit of that date and then the full date. With NVDA it looks like the keys navigate around better, but the widget doesn't update the display to show what's currently selected.

Another thing they discovered was that the today, clear, and close buttons here

image

have their labels read twice by screen readers. I believe this happens because of this line (not part of this PR):

content: attr(title);

That creates a pseudoelement ::after with the title duplicated as its contents:
image

Disabling that css rule in the browser console resolves the issue. Know if it'd be possible to remove entirely?

Are you able to replicate this behavior yourself with a screen reader? It seems like part of this is platform dependent. I can probably provide some screen recordings if that's helpful.

@Eonasdan
Copy link
Owner Author

Hi @esoergel. I didn't have either of these issues on the stackblitz example with Brave v1.74.51 one W11. However, I did notice some strange things with Edge on both the stackblitz and the aria example page. I'm not sure if this is something I can fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

2 participants