Skip to content

Conversation

fnagel
Copy link
Member

@fnagel fnagel commented Dec 10, 2015

  • Calendar: Fix tab index for prev / next buttons
  • Calendar: Fix ARIA / id attributes for multiple grids
  • Datepicker: Fix focus after date selection and after leaving the widget by using tab
  • Calendar: Do not rebuilt whole calendar when a date is selected (improves performance, fixes datepicker issues)

This should fix all "Focus related" issues in the wiki: http://wiki.jqueryui.com/w/page/12137778/Datepicker
See section "6 - Open issues being discussed" -> "Next Steps / ToDo"

* Change next / prev button structure for better tabindex
* Improve focus and active handling
Remove unneeded tabindex attribute change. Set focus to input after tab,
let default behavior decide which elements gains focus next.
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @arschmitz, @scottgonzalez and @mikesherov to be potential reviewers

@jzaefferer
Copy link
Member

Both datepicker and calendar don't make any use of _addClass() and its siblings. Should probably add that to the list on the wiki.

@fnagel
Copy link
Member Author

fnagel commented Dec 14, 2015

Both datepicker and calendar don't make any use of _addClass() and its siblings. Should probably add that to the list on the wiki.

Already on the wiki list.

@jzaefferer
Copy link
Member

"Make use of classes option", gotcha.

@jzaefferer
Copy link
Member

Testing in Chrome with some of the demos, I can't reproduce any of the issues listed on the wiki under "Focus related" anymore. Great work! I noticed a few new/other issues:

Testing with demos/datepicker/default.html, hitting tab when the grid is focused moves the focus to the prev button, and immediately hides the datepicker, with nothing focused. This works fine for the standalone (inline) calendar.

Testing demos/calendar/multiple-months.html, the focus outline stays on one of the three grids while using cursor keys to move focus around, including changing months. The focused day cell can't be insider the focused grid, but also outside. Clicking inside the calendar, but outside of any day cells, moves the focus outline to that grid, while clicking any of the day cells leaves it where it was.

@fnagel
Copy link
Member Author

fnagel commented Dec 16, 2015

hitting tab when the grid is focused moves the focus to the prev button, and immediately hides the datepicker, with nothing focused

Right, that's how it works at the very moment. Not sure why this "hit down arrow" was introduced in the first place.

Same for multiple-month: this is how this was supposed to work until now. I just tried to make it functional to have something to discuss. Pretty sure we do not want multiple focusable grids.
The wiki specs miss any advice for multiple datepicker.

Wiki needs some love anyway. It seems Gunderson is just copying ARIA specs in the wiki page. Not that helpful at the moment.

@jzaefferer
Copy link
Member

Right, that's how it works at the very moment. Not sure why this "hit down arrow" was introduced in the first place.

I'm not sure what you're talking about, since I didn't even mention down arrow key in my comment, and I didn't notice any issues related to that.

@fnagel
Copy link
Member Author

fnagel commented Dec 28, 2015

Right, that's how it works at the very moment. Not sure why this "hit down arrow" was introduced in the first place.

I'm not sure what you're talking about, since I didn't even mention down arrow key in my comment, and I didn't notice any issues related to that.

Ok, than I did not understand what you were talking about. The current behaviour "hitting tab when the grid is focused moves the focus to the prev button, and immediately hides the datepicker, with nothing focused" is expected. What woul you expect? Did you notice that using shift + tab focuses next, than prev and than closing the datepicker

@jzaefferer
Copy link
Member

Did you notice that using shift + tab focuses next, than prev and than closing the datepicker

That one works well. Here's what doesn't: Grid is focused, hit tab (just once). I expect the next focusable element to get focused. Instead the prev button is focused, but only briefly, since the datepicker is closed and nothing focused. That's very different from the shift+tab behaviour.

@fnagel
Copy link
Member Author

fnagel commented Jan 20, 2016

Sorry for the late response.

I expect the next focusable element to get focused.

Ok, now I understand. This behavior (having tab index: prev, next, grid, tab away) was the behavior we had before (but a little broken) this PR. I just wanted to fix the current state before before starting implementing changes.

That's very different from the shift+tab behaviour.

Not really when accepting that the grid is already the last element in our tabindex list. One can go back with SHIFT+TAB like: next, prev and focus the element before the triggering input.

I second that this behavior is somewhat unexpected and should probably be changed. Perhaps the idea was to have quick access to the grid after focusing the input and prevent tabbing trough the controls before using the actual calendar.

@fnagel
Copy link
Member Author

fnagel commented Jan 20, 2016

Regarding that Globalize / December Bug:

https://github.com/fnagel/jquery-ui/blob/datepicker-globalize/tests/unit/date/core.js#L170

I've analyzed this and came to the concolusion that the test fails for a good reason.
We're using .months(1) here, which returns the current month + next month. The actual test:

equal( firstMonth.month(), lastMonth.month() - 1 );

which is basically a compare of the month index. This is ok when tests run in November (10 === 11 - 1) but will fail in December (11 === 0 - 1).

I 've added a fixed date as proposed by @jzaffaerer here: #1651 (comment)

@fnagel
Copy link
Member Author

fnagel commented Jan 20, 2016

Removed some unwanted debugging markup from the default datepicker demo via rebase.

ok( !nextMonth.first, "Next month not marked as first" );
ok( nextMonth.last, "Next month marked as last" );

equal(
Copy link
Member

Choose a reason for hiding this comment

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

Now that the values no longer depend on the current date, replace this with two "static" assertions:

equal( currentMonth.month(), 11 );
equal( nextMonth.month(), 12 );

(or whatever the expected values are)

Copy link
Member

Choose a reason for hiding this comment

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

Let's address this, then the PR should be good to land.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the late response. Changed the assertions.

Merge?

@jzaefferer
Copy link
Member

I second that this behavior is somewhat unexpected and should probably be changed. Perhaps the idea was to have quick access to the grid after focusing the input and prevent tabbing trough the controls before using the actual calendar.

It still sounds like you're not seeing the behaviour I'm seeing - what I tried to describe should very obviously look like a bug, but just unexpected behaviour.

Anyway, let's finish this and we can deal with that issue in the next PR.

@jzaefferer
Copy link
Member

This behavior (having tab index: prev, next, grid, tab away) was the behavior we had before (but a little broken) this PR.

If hitting tab while the grid is focused would immediately close the datepicker, it would be fine. Instead of briefly focuses the prev button, then closes it, and the focus is lost (I guess defaults to body). Instead of focusing the prev button and (accidentally?) closing the datepicker, it should focus the next element in tab order AND close the datepicker immediately.

Hopefully that makes a little more sense.

@fnagel
Copy link
Member Author

fnagel commented Jan 28, 2016

Ok. Now (using the default demo without modifications) I see the issue. Please try adding a link tag before and after the input:

<a href="#">test</a>
<p>Date: <input type="text" id="datepicker"></p>
<a href="#">test</a>

Try again. Now that issue is gone, right?

Seems the "search for next / prev element in the tabindex" method needs some improvement. Next PR should be fine as that will change a lot a11y things anyway.

@fnagel
Copy link
Member Author

fnagel commented Feb 1, 2016

@jzaefferer @scottgonzalez I talked to Jon Gunderson a few days ago. Very helpful talk and promising outcome! He'll talk to his working group colleagues and provide us with some info how to handle multiple month grids and some specific info how to handle calendar only (without an input).

In addition he's willing to help us improving other widgets, too. This includes testing and feedback as well as the possibility to get our widgets tested by people using AT on a daily basis.

Next steps: Merge this PR when ready, implement the a11y wiki specs as they are, improve a11y with Jon's upcoming feedback, implement everything else (classes options, tests, missing options and callbacks, etc.).

Make sure tests are not depending on current month. Fixes tests run in December.
Removed duplicate test. Improve test descriptions.
@fnagel
Copy link
Member Author

fnagel commented Feb 26, 2016

@jzaefferer Changed the date tests as proposed. Should ready to merge then, right?

@jzaefferer
Copy link
Member

Yeah, let's move on.

@fnagel fnagel merged commit 6baa74b into jquery:datepicker Feb 27, 2016
@fnagel fnagel deleted the datepicker-a11y branch October 28, 2016 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants