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

DatePicker: Add option to not auto-open on focus #1451

Merged
merged 4 commits into from Apr 18, 2017
Merged

DatePicker: Add option to not auto-open on focus #1451

merged 4 commits into from Apr 18, 2017

Conversation

c-w
Copy link
Contributor

@c-w c-w commented Apr 10, 2017

Pull request checklist

  • Addresses an existing issue: #1266
  • Include a change request file if publishing
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Description of changes

When focus lands on the date picker, it currently immediately opens the date picker. This is confusing from an accessibility point of view. This pull request adds an option to disable the auto-open behavior so that the user can open the date picker manually, e.g. by hitting enter.

Focus areas to test

Test steps (verified manually on DatePicker.Input.Example: screencast):

  1. Create a date-picker with the disableAutoFocus property set to true.
  2. Tab through the page and onto the picker, it should not automatically open.
  3. Hit enter, the picker should open.

@mikewheaton
Copy link
Contributor

@mikewheaton mikewheaton commented Apr 10, 2017

Does it ever make sense to open it immediately on keyboard focus? Maybe instead of this being an option we should just remove the functionality entirely.

@c-w c-w changed the title Bugfix/date picker focus DatePicker: Improve focus handling for accessibility Apr 10, 2017
@c-w c-w changed the title DatePicker: Improve focus handling for accessibility DatePicker: Improve focus accessibility Apr 10, 2017
@c-w
Copy link
Contributor Author

@c-w c-w commented Apr 10, 2017

@mikewheaton If you're happy to make that API change, we can go for it. @scriby: do you see a scenario where auto-opening the date-picker is the correct behavior?

@scriby
Copy link
Contributor

@scriby scriby commented Apr 10, 2017

From the standpoint of supporting screenreaders, I think it's much more understandable to require the user to take an explicit action to open the popup than to just launch them into it without context. So, I think it's a reasonable stance to take that we don't allow the date picker to open automatically on focus.

Whether or not a third party might want that behavior regardless some day is another story...

@mdahamiwal
Copy link
Contributor

@mdahamiwal mdahamiwal commented Apr 11, 2017

@c-w , @mikewheaton I think we should go through the accessible date pickers listed at http://www.webaxe.org/accessible-date-pickers/ and decide on the design. There are both types 1. ones that open the picker popup on focus and 2. others that provide a focusable picker button next to date text box.

@c-w
Copy link
Contributor Author

@c-w c-w commented Apr 11, 2017

While we have the conversation about the focus-open behavior, I split out #1477 for the tab-order changes as the fix is valuable on its own.

@c-w c-w changed the title DatePicker: Improve focus accessibility DatePicker: Add option to not auto-open on focus Apr 11, 2017
@c-w
Copy link
Contributor Author

@c-w c-w commented Apr 13, 2017

@mdahamiwal @mikewheaton: Did you take a decision on whether you want the always/never auto-open behavior or whether you'd prefer to make it configurable?

@mikewheaton
Copy link
Contributor

@mikewheaton mikewheaton commented Apr 17, 2017

Let's just make it an option for now, as both look like valid options :)

@c-w
Copy link
Contributor Author

@c-w c-w commented Apr 17, 2017

@mikewheaton: Thanks for the decision. This pull request adds the new option to disable auto-open on focus of the date picker. Could you please give it a review?

Copy link
Contributor

@mikewheaton mikewheaton left a comment

Looks good, thanks!

@c-w
Copy link
Contributor Author

@c-w c-w commented Apr 17, 2017

@mikewheaton: Thanks for the sign-off. Could you please merge this in before master advances again? I don't have merge-access.

@mdahamiwal mdahamiwal merged commit b4c3c69 into microsoft:master Apr 18, 2017
1 check passed
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants