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

Calendar / Datepicker: Fixes focus and tab index handling #1658

Closed
wants to merge 136 commits into from

Conversation

fnagel
Copy link
Member

@fnagel fnagel commented Dec 3, 2015

  • Merge with master
  • Adjust file doc headers and meta data
  • 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"

tjvantoll and others added 30 commits January 29, 2015 13:42
Use the `eachDay` option in the other-months demo.
Fix handling of `extraClasses` property, split on space.
This is specifically for multi month pickers. This makes the assumption that
the keyboard is always interacting with the first month in a multi month
calendar. The next step is to store which grid currently has focus and to base
the focus logic off of that.
* Re-add `ui-datepicker-week-col` class name currently used.
* Add test suite.
* Support changing option after initialization.
Make `select()` a private method as it's not part of the specification.
Revert to old behavior with two instead of three chars.
Fixes layout issue with wrong day table cell width.
Change status caching, fix existing value related methods, introduce
$.date construction with date object, selected property is null by
default, add selected getter
@mention-bot
Copy link

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

@fnagel
Copy link
Member Author

fnagel commented Dec 3, 2015

@scottgonzalez @jzaefferer

Still not perfect but much improved a11y in this branch. I think we'll need to discuss some details of the proposed a11y specs in the wiki. The spec lacks information about how to handle multiple calendars, has some focus related difference and differs from the current implementation in general.

Should I just merge all master / merge commits to datepicker branch? Somewhat ugly to review the actual changes at the moment...

@jzaefferer FYI: most of the above mentioned wiki issues (see PR description) have been reported by you in former PRs.

@scottgonzalez
Copy link
Member

Should I just merge all master / merge commits to datepicker branch? Somewhat ugly to review the actual changes at the moment...

I just review individual commits rather than using the full diff when the changes are this big. Next time you can do this as two branches. One with just the merge and then another with your changes on top of that. Then you can send the PR against the branch with just the merge so the PR diff only shows the new code.

@fnagel
Copy link
Member Author

fnagel commented Dec 9, 2015

@scottgonzalez Reminder for another review (as asked for).

Not sure which commits you have already seen but I guess this should be new for you: Fix tab index for prev / next buttons, Fix focus after date selection and after leaving the widget by using tab, Do not rebuilt whole calendar when a date is selected

@jzaefferer
Copy link
Member

Is there any reason to have the merge commit in this PR? Seems like it makes things unnecessarily complicated to review.

PS:

Should I just merge all master / merge commits to datepicker branch? Somewhat ugly to review the actual changes at the moment...

If its a straightforward merge (no conflicts), yes please.

PPS: I should read everything first

@fnagel
Copy link
Member Author

fnagel commented Dec 10, 2015

If its a straightforward merge (no conflicts), yes please.

Datepicker will have always merge conflicts with master.

I will create two PRs next time (see Scotts response).

@jzaefferer
Copy link
Member

Travis build failed with on date unit tests:

Testing tests/unit/date/date.html ..........F..
>> date: core - Months
>> Message: null
>> Actual: 11
>> Expected: -1

Is that the same issue we were looking at before, but with improved output? Or something else?

@fnagel
Copy link
Member Author

fnagel commented Dec 10, 2015

Travis build failed with on date unit tests

That's the issue we talked about in the last PR: #1651 (comment) and following
Not sure why this happens, seems like a Globalize internal issue as this popped up from nowhere when December started.

@fnagel
Copy link
Member Author

fnagel commented Dec 10, 2015

Closed in favor of #1660. Merge and some clean up commits have been merged into datepicker branch.

@scottgonzalez @jzaefferer

@fnagel fnagel closed this Dec 10, 2015
@fnagel fnagel deleted the datepicker-focus-issues branch October 28, 2016 10:20
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.

7 participants