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

Add combobox date picker example for issue 34 #1413

Merged
merged 59 commits into from
Jul 7, 2020

Conversation

mcking65
Copy link
Contributor

@mcking65 mcking65 commented May 26, 2020

Adds the combobox datepicker example described in issue #34. Incorporates work from #1017.

Preview Link

Preview of date picker combobox example

Review checklist


Preview | Diff

jongund and others added 2 commits May 26, 2020 12:19
* Add Initial Cut of Date Picker Example (pull #839)

For issue #34, add a directory for datepicker examples and a first cut of code.

* Date Picker Example: Add link to development issue

* updated datepicker example and documentation

* fixed broken reference

* made updates to the documentation

* made updates to the documentation

* made updates to the documentation

* made updated code

* updated example

* updated code

* fixed problem with document clicks when date picker is open

* fixed problem with document clicks when date picker is open

* updated month year title

* improved source code documentation

* focus returns to textbox and added Home and End key commands for first and last day of the week

* edited description of example

* updated documentation

* updated documentation

* updated documentation

* fixed some bugs

* fixed some bugs

* removed some debugging code

* validated js and css code

* fixed bug

* added two regression tests

* updated keyboard behavior for opeing the datepicker

* updated date picker example to support keyboard announcements

* updated date picker example to support keyboard announcements

* updated date picker example to support keyboard announcements

* added down arrow key

* fixe bug with onclick

* updated click event on date to put date in textbox and close dialog

* down arrow appears on input focus

* updated click behaviors for input and calendar button

* updated datepicker document click event to handle clicking in textbox

* updated datepicker document click event to handle clicking in textbox

* updated code for down arrow in textbox

* updated example CSS

* updated code for down arrow in textbox

* fixed bug with document click event for dialog

* fixed interation bugs and added accessibilty feature description

* updated documentation, fied bugs and added a menu button date picker

* fixing mouse interaction issues

* fixed mouse bugs with date picker example

* cleaning up some of the code

* added feature to move to the next or previosu month when clicking on a disabled date

* changed header cells from using aria-label to abbr attribute

* fixed bugs with buttons for changing the next/previous year and month buttons

* fixed bug with next/previous buttons when focus is in the textbox

* created intial example of menu button for opening date picker

* added a file to support spin buttons

* initial implementation of a datepicker using spin buttons

* added documenation about values wrapping for day and month

* added documenation about values wrapping for day and month

* button label is updated with current date information

* button label is updated with current date information for combobox too now

* Move spinbutton and combobox examples

Moved the combobox datepicker to the aria 1.1 combobox directory.
Move the spinbutton date picker to new branch issue125-spinbutton-datepicker-example.

* Remove files copied to new issue1046-datepicker-dialog branch

* updating example to be compatible with date gird used in dialog date picker example

* fixed css bug

* added message live region to dialog

* fixed some bugs related to messages

* updated documentation on live regions

* updated regression test file for noew location of example and added a few tests for combobox

* removed aria-controls from buttons in the dialog to change the month/year and updated the file names to be compatible with other combox file names

* test commit

* test commit 2

* updated date picker code to use the Date object like the datepicker dialog example

* improved code by adding helper functions and removing a redundant function

* updated file name to be consistent wih other examples

* restored previous file name for compatibility with ava testing

* improved code readibility by using helper function to test if same month

* removed some unused variables

* removed unused properties and used helper function to compare days

* updated docuemntation and added ids for developing regression tests

* started integration of dialog box regression tests

* button label now updates with the selected date information

* updated link to provide feedback to point to th epull request

* removed aria-expanded from button

* updated method names to be more consistent and descriptive

* updated documentation

* updated documentation

* updated documentation

* updated documentation

* updated properties to move role and arai-expanded to input box, changed aria-owns to aria-controls and removed disabled days form the date grid

* updated some of the tests

* updated tests and fixe a tabindex bug

* removed some unused code

* removed unused functions

* updated CSS for hding disabled dates

* fixed data-date attribute computation

* fixed html validation bugs

* fixed some bugs

* restored

* restored

* restored

* updated test cases

* Change version in title and footer from 1.1 to 1.2

* changes file name

* updated code

* fixed bug with last row being shown

* added updating button label based on selected date

* This test is no longer needed

* changed design to remove buttons from gridcells

* Added role gridcell to support validator bug for not recognizig `td` in contect of a `role=grid` as a `gridcell`

* improved high contrast of combobox buton svg image

* added role=groidcell to pass html validator

* fixed bug with aria-selected

* fixed bug with aria-select following focus

* separated space and enter behavior and fixed bug with focus on combobox when page loads

* Updated links to source files

* Fixed broken link

* updated tests and added keyboard event for button

* updated test using the SPACE key

* updated focus styling

* Apply editorial suggestions from code review by @carmacleod

Co-Authored-By: Carolyn MacLeod <[email protected]>

* updated focus styling to use solid borders

* updated styling of date with tabindex=0

* updated regression tests coverage to inlcude missing tests

* removed reference to undefined object

* Add missing data-test-id for tabindex=-1

* Add test for month-year-button-space-return

* removed code to update button with the current date

* Fix an assertion in the month-year-button-space-return test

* Use await where appropriate

Tests were sometimes passing and sometimes failing in CI.
I think the reason may be that the code wasn't using await consistently.

* fixed regression test to use Key.SPACE instead of space character

* made improvements to high contrast support

* updating regression tests

* updated regression test to use queryElements and remove t.plan, but getting rejected promise errors

* updated documentation on high contrast support

* updated documentation on high contrast support

* updated documentation on high contrast support

* updated documentation on high contrast support

* Fix tests to use queryElements correctly

* Fix spelling errors caught by npm run lint:spelling

* Add stubs for missing tests

* removed code for an unused live region for a combobox message

Co-authored-by: Matt King <[email protected]>
Co-authored-by: Carolyn MacLeod <[email protected]>
Co-authored-by: Simon Pieters <[email protected]>
@curtbellew
Copy link

I ran through some testing tools, screen reader, keyboard and high contrast tests. Everything looks fantastic but I had one question. The use of the abbr attribute in the column header TH's. For instance the demo uses abbr="Sunday" when the TH text is "Su". I was thinking that abbr in the TH had been deprecated in HTML5. Also, I have always thought of abbr as the abbreviation of the long content so abbr="Su" for the TH text "Sunday" instead. I usually recommend using aria-label for the long versions of the column header in cases where we want longer text to be read out by screen readers.

@jscholes
Copy link
Contributor

I'm glad to see a functional datepicker example be included for the combo box pattern. There are a few usability concerns that I'm perceiving based on testing the preview link.

Date Input

  1. The date format uses a placeholder attribute. Most screen reader users are going to find it difficult (or impossible) to review it character by character, for example to determine whether the collection of lower case Y's represents a 2, 3 or 4 digit year entry. Plus the other accessibility concerns with placeholder. This is a common problem with date fields and can be fixed either by:
    • including the date format inline on the page and referencing it via aria-describedby; or
    • including it in the label ("Date ("mm/dd/yyyy").
  2. The example page states: "A live region provides information on how to open the date picker dialog when textbox receives focus (e.g. using down arrow key)". However, this doesn't work for me with the latest stable builds of both NVDA and Chrome, and seems like a suboptimal way of providing hint text. Again, aria-describedby would be the ideal candidate here, allowing JAWS users to turn off hints for example.

Modal

  1. A live region is used to communicate the new month when crossing a boundary. However, as the grid and modal containers are also aria-labelledby the heading, NVDA speaks the information twice and JAWS speaks it three times (latest stable builds of each):
    • JAWS: "April 2020 �Modal� �dialog. April 2020 �Grid. Th 30. April 2020"
    • NVDA: "April 2020. 30 Th. April 2020"
  2. Because this follows the grid pattern rather than using an application role with a table of buttons, NVDA users must press Escape twice to dismiss the modal. Once to exit focus mode, and once more to actually pass the keystroke through to the page.
  3. While navigating between the cells using any of the arrow keys, it consistently takes an entire second for JAWS to announce the new destination cell.
  4. The use of abbreviated day names is a horrible UX for screen reader users. As @curtbellew pointed out, having the full day name inside the abbr attribute on each <th> seems backwards, but screen readers seem to ignore it regardless. From a design perspective, I predict that devs and designers will want abbreviated column headers, but we should probably find a reliable way to present the full text to a screen reader so that the pattern can do the right thing out of the box.
  5. The current date isn't marked with aria-current="date". Granted, it doesn't seem to be made visually apparent either, but again this is something designers will probably patch in without adding the appropriate programmatic indication.
  6. The "Cancel" button is positioned before "OK", which users will only expect if they're accustomed to working on MacOS or iOS.
  7. The help text, "Cursor keys can navigate dates", doesn't communicate any of the other supported keystrokes such as Page Up/Down and Home/End. As with the help message for the date input, I also wonder if there's a reason not to apply this to the grid via aria-describedby?

@JAWS-test
Copy link

JAWS-test commented May 31, 2020

I was thinking that abbr in the TH had been deprecated in HTML5.
Also, I have always thought of abbr as the abbreviation of the long content so abbr="Su" for the TH text "Sunday" instead

Both is wrong, see https://html.spec.whatwg.org/multipage/tables.html#attr-th-abbr:

It is typically an abbreviated form of the full header cell, but can also be an expansion, or merely a different phrasing

@jscholes
Copy link
Contributor

Some further observations:

  1. The example page states: "The date picker dialog is opened by ... moving keyboard focus to the combobox and pressing Enter". Enter is not listed as having this function as part of the Combo Box design pattern, not even as an optional affordance. Encouraging web authors to implement it in this way will prevent default, quick form submission for users who prefer to type a date manually and then hit Enter.
  2. When swiping through dates in the table with iOS, the column headers aren't announced at all. This makes it seem like just a grid of numbers, which has long been an accessibility antipattern for calendars. When combined with the apparent lack of support for the abbr attribute on <th> by screen readers, I think the cells must have more descriptive accessible names.

@curtbellew
Copy link

I was thinking that abbr in the TH had been deprecated in HTML5.
Also, I have always thought of abbr as the abbreviation of the long content so abbr="Su" for the TH text "Sunday" instead

Both is wrong, see https://html.spec.whatwg.org/multipage/tables.html#attr-th-abbr:

It is typically an abbreviated form of the full header cell, but can also be an expansion, or merely a different phrasing

I think I've looked at an older version of the standard when I've researched this in the past -- https://www.w3.org/TR/2010/WD-html-markup-20100624/th.html . It looks like updated versions indicate that it's not been deprecated. Sorry about that. The second part of your quote is pretty much what I said above so I'm not following. In any case, I don't see this as any sort of violation since abbr hasn't been deprecated. I'd still recommend using aria-label. Thanks!

@carmacleod
Copy link
Contributor

If you click on the drop-down arrow with the mouse, focus stays on the input.
Should it instead go to the grid so that arrow keys can be used to navigate?

…out live region for opeing the dialog box from the textbox
@jongund
Copy link
Contributor

jongund commented Jun 9, 2020

Made the following changes:

  • Removed placeholder attribute from combobox
  • Added description to combobox for date format
  • Removed information from the documentation about a live region describing how to open the dialog from the combobox
  • Added a test case for the aria-describedby attribute on the combobox

examples/combobox/js/combobox-datepicker.js Outdated Show resolved Hide resolved
examples/combobox/js/combobox-datepicker.js Outdated Show resolved Hide resolved
examples/combobox/css/datepicker-combobox.css Outdated Show resolved Hide resolved
examples/combobox/css/combobox-datepicker.css Show resolved Hide resolved
examples/combobox/css/combobox-datepicker.css Outdated Show resolved Hide resolved
Copy link
Contributor

@spectranaut spectranaut left a comment

Choose a reason for hiding this comment

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

This is an approving "code review". Read through all the javascript for the combobox datepicker, the logic was so clean and easy to read! Looks so good, @jongund ! I just left comments about changes in the datepicker-dialog.html file.

examples/dialog-modal/datepicker-dialog.html Outdated Show resolved Hide resolved
examples/dialog-modal/datepicker-dialog.html Outdated Show resolved Hide resolved
@jongund
Copy link
Contributor

jongund commented Jun 26, 2020

@spectranaut
@mcking65

I have fixed the spacing issue with combobox-datepicker.html and removed datepicker-dialog.html from being updated in this pull request, it is updated in a different pull request.

jongund and others added 6 commits June 30, 2020 18:02
* Add Initial Cut of Date Picker Example (pull #839)

For issue #34, add a directory for datepicker examples and a first cut of code.

* Date Picker Example: Add link to development issue

* updated datepicker example and documentation

* fixed broken reference

* made updates to the documentation

* made updates to the documentation

* made updates to the documentation

* made updated code

* updated example

* updated code

* fixed problem with document clicks when date picker is open

* fixed problem with document clicks when date picker is open

* updated month year title

* improved source code documentation

* focus returns to textbox and added Home and End key commands for first and last day of the week

* edited description of example

* updated documentation

* updated documentation

* updated documentation

* fixed some bugs

* fixed some bugs

* removed some debugging code

* validated js and css code

* fixed bug

* added two regression tests

* updated keyboard behavior for opeing the datepicker

* updated date picker example to support keyboard announcements

* updated date picker example to support keyboard announcements

* updated date picker example to support keyboard announcements

* added down arrow key

* fixe bug with onclick

* updated click event on date to put date in textbox and close dialog

* down arrow appears on input focus

* updated click behaviors for input and calendar button

* updated datepicker document click event to handle clicking in textbox

* updated datepicker document click event to handle clicking in textbox

* updated code for down arrow in textbox

* updated example CSS

* updated code for down arrow in textbox

* fixed bug with document click event for dialog

* fixed interation bugs and added accessibilty feature description

* updated documentation, fied bugs and added a menu button date picker

* fixing mouse interaction issues

* fixed mouse bugs with date picker example

* cleaning up some of the code

* added feature to move to the next or previosu month when clicking on a disabled date

* changed header cells from using aria-label to abbr attribute

* fixed bugs with buttons for changing the next/previous year and month buttons

* fixed bug with next/previous buttons when focus is in the textbox

* created intial example of menu button for opening date picker

* added a file to support spin buttons

* initial implementation of a datepicker using spin buttons

* added documenation about values wrapping for day and month

* added documenation about values wrapping for day and month

* button label is updated with current date information

* button label is updated with current date information for combobox too now

* Move spinbutton and combobox examples

Moved the combobox datepicker to the aria 1.1 combobox directory.
Move the spinbutton date picker to new branch issue125-spinbutton-datepicker-example.

* Remove files copied to new issue1046-datepicker-dialog branch

* updating example to be compatible with date gird used in dialog date picker example

* fixed css bug

* added message live region to dialog

* fixed some bugs related to messages

* updated documentation on live regions

* updated regression test file for noew location of example and added a few tests for combobox

* removed aria-controls from buttons in the dialog to change the month/year and updated the file names to be compatible with other combox file names

* test commit

* test commit 2

* updated date picker code to use the Date object like the datepicker dialog example

* improved code by adding helper functions and removing a redundant function

* updated file name to be consistent wih other examples

* restored previous file name for compatibility with ava testing

* improved code readibility by using helper function to test if same month

* removed some unused variables

* removed unused properties and used helper function to compare days

* updated docuemntation and added ids for developing regression tests

* started integration of dialog box regression tests

* button label now updates with the selected date information

* updated link to provide feedback to point to th epull request

* removed aria-expanded from button

* updated method names to be more consistent and descriptive

* updated documentation

* updated documentation

* updated documentation

* updated documentation

* updated properties to move role and arai-expanded to input box, changed aria-owns to aria-controls and removed disabled days form the date grid

* updated some of the tests

* updated tests and fixe a tabindex bug

* removed some unused code

* removed unused functions

* updated CSS for hding disabled dates

* fixed data-date attribute computation

* fixed html validation bugs

* fixed some bugs

* restored

* restored

* restored

* updated test cases

* Change version in title and footer from 1.1 to 1.2

* changes file name

* updated code

* fixed bug with last row being shown

* added updating button label based on selected date

* This test is no longer needed

* changed design to remove buttons from gridcells

* Added role gridcell to support validator bug for not recognizig `td` in contect of a `role=grid` as a `gridcell`

* improved high contrast of combobox buton svg image

* added role=groidcell to pass html validator

* fixed bug with aria-selected

* fixed bug with aria-select following focus

* separated space and enter behavior and fixed bug with focus on combobox when page loads

* Updated links to source files

* Fixed broken link

* updated tests and added keyboard event for button

* updated test using the SPACE key

* updated focus styling

* Apply editorial suggestions from code review by @carmacleod

Co-Authored-By: Carolyn MacLeod <[email protected]>

* updated focus styling to use solid borders

* updated styling of date with tabindex=0

* updated regression tests coverage to inlcude missing tests

* removed reference to undefined object

* Add missing data-test-id for tabindex=-1

* Add test for month-year-button-space-return

* removed code to update button with the current date

* Fix an assertion in the month-year-button-space-return test

* Use await where appropriate

Tests were sometimes passing and sometimes failing in CI.
I think the reason may be that the code wasn't using await consistently.

* fixed regression test to use Key.SPACE instead of space character

* made improvements to high contrast support

* updating regression tests

* updated regression test to use queryElements and remove t.plan, but getting rejected promise errors

* updated documentation on high contrast support

* updated documentation on high contrast support

* updated documentation on high contrast support

* updated documentation on high contrast support

* Fix tests to use queryElements correctly

* Fix spelling errors caught by npm run lint:spelling

* Add stubs for missing tests

* removed code for an unused live region for a combobox message

Co-authored-by: Matt King <[email protected]>
Co-authored-by: Carolyn MacLeod <[email protected]>
Co-authored-by: Simon Pieters <[email protected]>
…out live region for opeing the dialog box from the textbox
@mcking65 mcking65 force-pushed the issue34-add-combobox-datepicker branch from 892df5f to c4dd45a Compare July 1, 2020 01:03
@mcking65
Copy link
Contributor Author

mcking65 commented Jul 1, 2020

@jongund, this is close. There are some changes I think we need to make before merging, and some that can wait and be addressed as separate issues after merge.

I think we can merge by July 10. We will use the July 7 meeting to resolve anything that we have not resolved asynchronously this week.

After reviewing the experience and the feedback above, here are some changes I think we need to make in this branch. I explain the reasons below.

  1. Change the down arrow button (line 61 in HTML) as follows:
    • Change aria-label from "Date" to "Choose Date"
    • Remove aria-expanded, aria-controls, and aria-haspopup and adjust tests and documentation of states and props accordingly.
    • Remove outdated documentation about dynamic changes to the button label.
  2. Move the div with id cb-description to be between the input and the button in the reading order.
  3. Remove the Enter key behavior from the combobox and adjust documentation accordingly.
  4. Make clicking the down arrow icon place focus inside the dialog in the same way that pressing down arrow does.
  5. Instead of having the dialog labeled by the current month, put aria-label="Choose Date" on the dialog.

Why these changes?

First, the button that opens the dialog, other than the fact that it is not keyboard focusable, should be consistent with other buttons that open dialogs. We do not use aria-haspopup because that changes the role of the button to menubutton, even if the value of haspopup is dialog. That is a limitation of certain accessibility APIs. The value of dialog for haspopup is intended only for the combobox element itself.

This button is different from the down arrow buttons for comboboxes that popup a listbox or grid because those popup elements are not modal. When the popup is not modal, or when a popup can be open without containing visual focus, a touch user can collapse the combobox by activating the down arrow icon. Because of that, we use aria-expanded on the button so that touch screen reader users can readily know the state of the popup from the button and get feedback from activating the button. In the case of a dialog, screen readers inhibit access to the button when the dialog is open, and the dialog itself includes a cancel button for closing via touch.

The label change on the button is to give screen reader users better understanding of the purpose of the button, and it matches the name I proposed for the dialog.

The accessible name change on the dialog is for three reasons. First, changing the title of the dialog dynamically is one of the reasons for the excess speech described in earlier feedback. The second reason is that a label of "Choose Date" on the dialog gives better feedback to the user about where they are, keeping in mind that the grid is labeled by the month and year. Finally, it makes it possible for the button that opens the dialog and the dialog to have the same accessible name, which is consistent with what we recommend for dialogs.

I hope the change to reading order for the combobox description div does not complicate visual positioning. In general, I find that it is a much better screen reader experience, especially with touch, when such instructions are immediately adjacent to the input. This is especially true if we later add some error checking and error messages could be displayed there.

We should remove the enter key behavior, because as noted by others, it is not consistent with the combobox pattern. While I have seen this in the wild, it is potentially problematic and not a practice we should promote.

Finally, moving the focus into the dialog when the down arrow icon is clicked as Carolyn suggested is important. That makes it consistent with other buttons that open dialogs. This is particularly important for screen reader users who activate the button with a reading cursor or via touch on a mobile device.

From an editorial copy perspective, I plan to make changes to substantially trim the accessibility features section so it is consistent with how we draft that section in other examples. It will cover only accessibility features with explainations of the ways in which they improve accessibility. However, I WILL NOT commit those changes in this branch. I'll make a separate branch with a PR requesting merge into this branch so we can iron out the content of the accessibility features section without cluttering this PR. So, as you make the above changes, please don't touch the accessibility features section; only change documentation in the tables after it.

There is one other topic that I'd like to discuss in the July 7 meeting -- revealing keyboard functionality in the UI. Please do not make any changes in this branch related to this topic. If the group arrives at some consensus, we will land those changes separately. Basically, I think that we should reveal the keyboard assignments for page up/down and shift+pageUp/Down on their respective buttons. Perhaps we do it in a manner similar to how we reveal the labels on the icons in the editor toolbar example. Or, possibly it is in a help field that appears at the bottom of the dialog when one of those buttons has focus. Related to this, I'm still not a fan of the live region that announces the cursor key instruction. If we keep that, we need to improve the wording. However, I think those types of instructions are the types of instructions that screen readers could choose to provide for beginner users, just like they tell users what keys to press in a menu or listbox. We could have aria-at assertions related to the provision of such instructions.

@jongund
Copy link
Contributor

jongund commented Jul 2, 2020

@mcking65

I have checked in the changes base on your last comments.
Did you want to use aria-current="date" instead of aria-selected="true" to indicate the currently selected date in the grid?

@a11ydoer
Copy link
Contributor

a11ydoer commented Jul 7, 2020

Other than a space key issue in “date picker dialog: date grid” – it comes with the behavior of scrolling and jumping down, the example looks like a well-thought-out implementaiton.

Copy link
Contributor

@a11ydoer a11ydoer left a comment

Choose a reason for hiding this comment

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

All the good changes although I do not see what part was changed in js code.
In my recollection based on comments, major changes were removal of unused functions. Great job!

@jongund
Copy link
Contributor

jongund commented Jul 7, 2020

@mcking65
Removed test for keyboard support regression tests for the SPACE and ENTER key for the "Choose Date" Button, since the reference in the example has been removed.

@jongund jongund self-requested a review July 7, 2020 14:28
Copy link
Contributor

@jongund jongund left a comment

Choose a reason for hiding this comment

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

I am concerned the "accessible features" section does not sufficiently highlight some of the accessibility details of this example, especially support of keyboard focus styling in high contrast modes of operating systems. But this example needs to be merged so we can get to the other pull requests that need review and merging.

@mcking65 mcking65 merged commit 33cb0ba into master Jul 7, 2020
@mcking65 mcking65 deleted the issue34-add-combobox-datepicker branch July 7, 2020 19:31
michael-n-cooper pushed a commit that referenced this pull request Jul 7, 2020
Add combobox date picker example(pull #1413)

Resolves #34 by adding a combobox example that opens a dialog containing a calendar grid.

Co-authored-by: Jon Gunderson <[email protected]>
Co-authored-by: Carolyn MacLeod <[email protected]>
Co-authored-by: Simon Pieters <[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.

9 participants