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

Date Picker Dialog Example: Improve high contrast support and refactor javascript #1362

Merged
merged 52 commits into from
Jul 24, 2020

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Mar 31, 2020

Resolves #1360 with the following changes:

  • Updates Javascript to use a single object.
  • Fixes space key issues in grid.
  • Improves high contrast support.

Preview link

View changes in the compare branch for this PR

Review checklist

@jongund
Copy link
Contributor Author

jongund commented Apr 1, 2020

@spectranaut @smhigley @mcking65

I updated the regression tests to fix the SPACE key issue in Issue #1358

@zcorpan
Copy link
Member

zcorpan commented Apr 3, 2020

Part of the regression test for the date picker is very slow to run. It's focusing each cell with half a second delay between each step. Do you know why it's slow?

$ npm run regression -- --match "*datepicker*"

> [email protected] regression /Users/zcorpan/git/w3c/aria-practices
> ava --timeout=1m "--match" "*datepicker*"



  43 tests passed
  2 known failures

  dialog-modal_datepicker › dialog-modal/datepicker-dialog.html [data-test-id="month-year-button-space-return"]: SPACE to buttons change calendar and date in focus
  dialog-modal_datepicker › dialog-modal/datepicker-dialog.html [data-test-id="grid-space-return"]: SPACE or RETURN selects date in focus

Are the known failures intended to be failures still?

@@ -696,7 +706,7 @@ ariaTest('SPACE on cancel button does not select date', exampleFile, 'okay-cance

await setDateToJanFirst2019(t);
await t.context.session.findElement(By.css(ex.buttonSelector)).click();
await t.context.session.findElement(By.css(ex.currentlyFocusedButton)).sendKeys(Key.ARROW_RIGHT);
await t.context.session.findElement(By.css(ex.currentlyFocusedDay)).sendKeys(Key.ARROW_RIGHT);
await t.context.session.findElement(By.css(ex.cancelButton)).sendKeys(Key.SPACE);
Copy link
Member

Choose a reason for hiding this comment

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

Should all Key.SPACEs be replaced with ' '? Maybe we can lint it also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zcorpan

I just did some testing and we can only do this when an keyboard event handler is associated with a button. So I think we should only use " " technique when needed and use Key.SPACE when it works correctly,

@zcorpan
Copy link
Member

zcorpan commented Apr 3, 2020

Improved high contrast support

The focus indicator is a 2px white border instead of a 1px white border. Could it be a different color, or more visible in some other way?

Screenshot of the date picker with focus on a date's cell.

@jongund
Copy link
Contributor Author

jongund commented Apr 3, 2020

@zcorpan Your right there is not much difference, I am going to change the border to be dashed to see if it stands out more.

@jongund
Copy link
Contributor Author

jongund commented Apr 3, 2020

@zcorpan I made some changes to focus styling and aria-selected styling that helps in high contrast, please review.

@jongund jongund requested a review from zcorpan April 3, 2020 18:21
@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Update menu button datepicker dialog example.

The full IRC log of that discussion <carmacleod> Topic: Update menu button datepicker dialog example
<carmacleod> github: https://github.com//issues/1362

@jongund
Copy link
Contributor Author

jongund commented May 5, 2020

@zcorpan

I made some updates for high contrast support for dates and buttons in the dialog, can you review and comment on the changes.

@mcking65
Copy link
Contributor

@jongund, I just wrapped up my editorial review and pushed some revisions. While reviewing, I found 2 more things we need to change before requesting final reviews.

  1. Remove aria-autocomplete from the input. It is a normal textbox, not a combobox, so we shouldn't specify autocomplete none.
  2. Remove support for down arrow opening the dialog when focus is on the button; modify script, test, and documentation accordingly. The button is a normal button, not a menu button, so it should not support activation with down arrow.

@jongund
Copy link
Contributor Author

jongund commented Jul 17, 2020

@mcking65
Is the reason it is not to use the menu button pattern is because it opens a dialog and not a menu?

@jongund
Copy link
Contributor Author

jongund commented Jul 17, 2020

@mcking65
The date picker dialog has been updated by removing the autocomplete attribute and the down arrow now does not open the dialog.

@mcking65
Copy link
Contributor

@jongund wrote:

Is the reason it is not to use the menu button pattern is because it opens a dialog and not a menu?

Yes, exactly.

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

Final editorial review complete: The documentation of the example is consistent with editorial guidelines, the relevant design patterns, and the ARIA spec, and it pmatches what is implemented in the code.

I also tested thoroughly with JAWS and NVDA in Chrome. As expected, based on our testing of the date picker combobox example, there are improvements needed by screen reader developers. There are deficiencies in screen reader support for abbr on TH when in a grid. And, the way NVDA reads a grid makes understanding a grid of numbers difficult.

@carmacleod
Copy link
Contributor

The calendar button takes focus on page load. Do we want that? I found it a bit surprising.

@carmacleod
Copy link
Contributor

carmacleod commented Jul 21, 2020

In the dialog, shortcut keys are assigned to the additional buttons for changing the month and year displayed in the calendar.

How can the user discover the shortcut keys? Should we have tooltips (@smhigley 😄 ?) on the change month/year buttons?

Keyboard help is displayed at the bottom of the dialog.

Or maybe the change month/year shortcuts can be added to the keyboard help text?
i.e. the keyboard help text currently says:

Cursor keys can navigate dates

maybe change it to:

Cursor keys can navigate dates. PageUp/PageDown changes month. Shift+PageUp/PageDown changes year.

Come to think of it, should we change the word "Cursor" to "Arrow"? i.e. maybe some folks don't know what "Cursor" keys are?
So maybe:

Arrow keys navigate dates. PageUp/PageDown changes month. Shift+PageUp/PageDown changes year.

Can anyone think of a more concise, but still understandable, way to write that sentence?

@carmacleod
Copy link
Contributor

If the user only types a 2-digit year, what do we want to happen?
Currently, the calendar opens in the 1900's.
For example, 5/7/09 opens to May 7th, 1909.
I think we should at least change that to open May 7th, 2009, because now that we are in the 2000's, people think of '09 meaning 2009.

@smhigley
Copy link
Contributor

@carmacleod could we open a separate issue for those bugs? I'm in favor of keeping this PR to the changes Jon already outlined & merging it (and those changes look good to me, fwiw). Then we can have additional smaller PRs for each other bug :).

@carmacleod
Copy link
Contributor

@smhigley I'm happy to open new issues for the shortcuts and the 2-digit year. But the calendar button taking focus on page load is being introduced by this PR (here's the released example), so I think we should fix that one before it goes in?
It seems to be because the DatePickerDialog is created on page load and then "this.close()" is called, but that puts focus on the button. So just needs a tiny refactor to call the "hide the dialog" code separately from the "focus the button" code.

@carmacleod carmacleod removed the request for review from curtbellew July 21, 2020 18:19
@jongund
Copy link
Contributor Author

jongund commented Jul 21, 2020

@carmacleod
@smhigley
I fixed the focus on button when page loads and also added a feature for a 2 digit date to default to 20xx.
Please test when you have a chance.
Thank you for the feedback.

Copy link
Contributor

@carmacleod carmacleod left a comment

Choose a reason for hiding this comment

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

Thanks, @jongund.
Looks good!

@jongund
Copy link
Contributor Author

jongund commented Jul 21, 2020

@carmacleod
Thank you for all your testing and feedback.

@mcking65 mcking65 merged commit 733ceab into master Jul 24, 2020
@mcking65 mcking65 deleted the issue1360-update-datepicker-dialog-example branch July 24, 2020 22:54
michael-n-cooper pushed a commit that referenced this pull request Jul 24, 2020
Date Picker Dialog Example: Improve high contrast support and refactor javascript (pull #1362)

Resolves #1360 with the following changes:
* Updates Javascript to use a single object.
* Fixes space key issues in grid.
* Improves high contrast support.

Co-authored-by: Simon Pieters <[email protected]>
Co-authored-by: Matt King <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update date picker dialog example to use single object and remove buttons from date grid
8 participants