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

Combobox Examples with listbox popup: Fix Escape behavior, mouse interaction, visual design, and more #1276

Merged
merged 47 commits into from
Feb 14, 2020

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Nov 22, 2019

I have updated the combobox autocomplete examples:

Preview links

Review checklist

@carmacleod
Copy link
Contributor

carmacleod commented Nov 23, 2019

@jongund
I updated the preview links because they were going to old copies of the 3 examples.

I noticed a focus issue in all 3 comboboxes:

  1. tab to the combobox to focus it
  2. type down arrow a few times to open the listbox and move through the items
  3. type enter to choose an item and close the listbox
  4. now I expected to be able to type down arrow again to reopen the list, but nothing happens
    (typing character keys, or delete, or left or right arrow actually refocuses the textbox, but down and up arrow are ignored)

I think focus needs to go back to the textbox after the listbox is closed.

@jongund
Copy link
Contributor Author

jongund commented Nov 29, 2019

@carmacleod Carolyn I fixed the problem of the listbox not opening

@carmacleod
Copy link
Contributor

Thanks, @jongund !

One more weird case, only in the autocomplete=both example:

  • tab to the combo
  • type down arrow a few times
  • type Enter
  • now type down arrow - nothing happens

I was expecting it to behave the same as the autocomplete=list example, and open the popup even though there's only one item in it.

@jongund
Copy link
Contributor Author

jongund commented Dec 2, 2019

@carmacleod I made some updates to fix the down arrow bug you found, please test

examples/combobox/combobox-autocomplete-both.html Outdated Show resolved Hide resolved
examples/combobox/css/combobox-autocomplete.css Outdated Show resolved Hide resolved
examples/combobox/js/combobox-autocomplete.js Outdated Show resolved Hide resolved
examples/combobox/css/combobox-autocomplete.css Outdated Show resolved Hide resolved
@zcorpan
Copy link
Member

zcorpan commented Dec 3, 2019

Fixed other bugs related to previous code:
Issue #1268: Combobox Examples: Popup incorrectly contains only Utah when Wyoming in input
Issue #1265: Combobox examples: Make clicking outside popup close the popup

Very nice, the combobox feels much less buggy to use now!

I've reported a separate issue that I noticed while reviewing this: #1278

@carmacleod
Copy link
Contributor

@carmacleod I made some updates to fix the down arrow bug you found, please test

Fixed - thank-you, Jon! Much better.

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Combobox.

The full IRC log of that discussion <carmacleod> TOPIC: Combobox
<zcorpan> GitHub: https://github.com//pull/1276
<Jemma> carmacleod: there is some info about fall back value for aria value min and max
<carmacleod> carmacleod: Here is Fallback values table in ARIA spec: http://w3c.github.io/aria/#authorErrorDefaultValuesTable
<carmacleod> jon: combobox Esc key behavior: Esc closes popup; 2nd escape clears textbox
<carmacleod> mck: for autocomplete=both, enter and esc are essentially the same: value is chosen
<carmacleod> mck: for autocomplete=list, enter and esc are different; typing "al" then down arrow, then esc, leaves "al" in combobox
<carmacleod> zakim, who is on the call?
<Zakim> Present: Matt_King, zcorpan, Jemma, MarkMccarthy, carmacleod, jamesn, jongund
<carmacleod> zakim, bye
<Zakim> leaving. As of this point the attendees have been Matt_King, zcorpan, Jemma, MarkMccarthy, carmacleod, jamesn, jongund
<carmacleod> rrsagent, make minutes
<RRSAgent> I have made the request to generate https://www.w3.org/2019/12/03-aria-apg-minutes.html carmacleod
<carmacleod> regrets+ CurtBellew
<carmacleod> rrsagent, make minutes
<RRSAgent> I have made the request to generate https://www.w3.org/2019/12/03-aria-apg-minutes.html carmacleod

@zcorpan
Copy link
Member

zcorpan commented Feb 13, 2020

Thank you @smhigley, I believe I have addressed all comments.

@ZoeBijl ZoeBijl self-requested a review February 13, 2020 22:55
@ZoeBijl
Copy link
Contributor

ZoeBijl commented Feb 13, 2020

Accessibility Review

Notes per screen reader and browser combo.

Narrator with Edge 44.1 on Windows 10 (1909)

The comboboxes are announced correctly ("State, combo-edit", "Search, combo-edit, suggestions available"). But I can't select anything past the first suggestion. Not by pressing down arrow only. If there's another way to interact with comboboxes I'd like to learn about that.

Alt+Down Arrow works 👍.

VoiceOver with Safari 13.0.5 on macOS 10.14.6

After you land on the combobox it's announced as "Menu pop-up combobox, type text or, to display a list of choices, press Control-Option-Space." This combination doesn't work in any of the examples. I take it this is down to the screen reader rather than our example. But I don't know that for sure.

The "Both list and inline autocomplete" example seems to work OK. Can navigate into the auto complete and the options are announced. In the "With list autocomplete" example the options are shown, you can navigate into them, but they're not announced. Is this list defined differently?

The search example options are announced as long as you didn't type into the field first. Not typing and then making a selection from the list results in the same thing.

Alt+Down Arrow works 👍.

Note: If anyone has 10.15 I would appreciate a retest there.

JAWS 2020.1912.11 with Edgium on Windows 10 (1909)

Perfect 🌟.

Visual Review

In high contrast mode the drop down arrow disappears when the field is selected:

Selected combobox with missing arrow

The select state within the dropdown is clear tho:

State combobox with three suggestions showing. There's a clear contrast difference between the selected item and the others.

Conclusion

I suggest we add a border-radius: .125em; to the input. We should fix the disappearing drop down arrow. Other than that this is good to go.

@sinabahram
Copy link

sinabahram commented Feb 13, 2020 via email

@ZoeBijl
Copy link
Contributor

ZoeBijl commented Feb 14, 2020

Thanks @sinabahram! This visually works in both Safari and Edge with VoiceOver and Narrator respectively. But the thing is that if I then press Down Arrow Narrator moves to the drop down arrow instead of the first result.

@sinabahram
Copy link

sinabahram commented Feb 14, 2020 via email

@ZoeBijl
Copy link
Contributor

ZoeBijl commented Feb 14, 2020

Thanks. I've updated my two previous comments quite extensively after your comment. Out of interest: how is GitHub about communicating those changes? If at all.

@sinabahram
Copy link

sinabahram commented Feb 14, 2020 via email

}

.combobox polygon {
fill: gray;
Copy link
Contributor

Choose a reason for hiding this comment

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

change this to fill: currentColor; to fix High Contrast Mode.


.combobox polygon {
fill: gray;
stroke: gray;
Copy link
Contributor

Choose a reason for hiding this comment

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

change this to stroke: currentColor; to fix High Contrast Mode.


.combobox button.open polygon,
.combobox .group.focus polygon {
fill: black;
Copy link
Contributor

Choose a reason for hiding this comment

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

change this to fill: currentColor; to fix High Contrast Mode.

.combobox button.open polygon,
.combobox .group.focus polygon {
fill: black;
stroke: black;
Copy link
Contributor

Choose a reason for hiding this comment

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

change this to stroke: currentColor; to fix High Contrast Mode.

}

.combobox button.open polygon,
.combobox .group.focus polygon {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add color: black; to fix High Contrast Mode.

width: 10.75rem;
border-right: none;
outline: none;
font-size: 87.5%;
padding: 0.1em 0.3em;
}

.combobox .group button {
.combobox button {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add color: gray; to fix High Contrast Mode.

@sinabahram
Copy link

I finally got a chance to play with this ... Jaws 2020, latest, and Chrome latest.

I tried the first preview link "both", and I came across several issues. but not sure if this should be tested in Chrome+Jaws yet?

1: the aria-expanded does not seem to be surfaced? I hit the open button, but it never says it's collapsed nor does it announce being in an expanded state. It does announce the transition between collapsed/expanded, which is super odd, but I didn't spend time to figure out if that was coming from an aria-live message on the page or from some native announcement of aria-expanded state change.

2: the button opens a listbox, which allows for no interaction. Instead, when one focuses it, it just kicks you out of forms mode ... I tried lots of times.

3: In the combobox, several things jumped out at me.
A: The values wrap. That was unexpected, but I don't think I have a problem with this, especially since it can reduce the time it takes to find something even if all you can do is iterate previous/next, so I'm fine with that, but just noting that it was super-unexpected behavior.

B: Typing a letter seems to be a one-way path. Once I type 'N", I don't see any way of removing that from my filter. The character model is weirder than that, though, because it doesn't replace the value with 'n', or do what I think almost any screen reader user would expect, which is to be placed on a reduced list of items starting with the letter 'n'. Instead it adds 'n' to the end of whatever string I am on, but then if I use my arrow keys, seems to filter appropriately by starting with 'n'. 

C: I get zero automatic announcements of any found results or other type of dynamic information when typing or backspacing.

D: Hitting backspace just erases the last letter typed or the last letter of whatever item is currently focused, but clearly not selected, since if it were selected, it would erase the whole term.

E: It can lose state sometimes, like I backspaced over a word until I got to a subset of that place's name which I know appears in other states e.g. I backspaced over Northern Mariana Islands, until I had "north", but then when I hit up/down, I didn't get North Carolina.

4: The button says "open", but open what? That feels a bit like "read more" to me, where more specificity is required.

Anyways, I'm going to stop here. There are other issues as well. I'm actually somewhat concerned that I'm going down a rabbit hole because this is not meant to be tested in Chrome+Jaws yet?

If not, sorry for wasting time, but if so, I think a detailed screen reader treatment may be in order here.

@mcking65
Copy link
Contributor

While there are still quite a few bugs, the bugs targeted by this PR are resolved. I am merging. Then, I will respond to all the recent feedback and create separate issues as needed.

Thank you everyone for all the work on this!

@mcking65 mcking65 linked an issue Feb 14, 2020 that may be closed by this pull request
@mcking65 mcking65 merged commit 85085b2 into master Feb 14, 2020
@mcking65 mcking65 deleted the issue1066-combobox-fix-escape-key-and-other-bugs branch February 14, 2020 23:31
michael-n-cooper pushed a commit that referenced this pull request Feb 14, 2020
Combobox Examples with listbox popup: Fix Escape behavior, mouse interaction, visual design, and more  (pull #1276)

Fixes #785, #982, #983, #988, #1261, #1265, and #1268 with the following changes:
* updated JavaScript to use single prototype
* updated escape key behavior
* removed unused files
* fixed regression issues for escape key
* updated tests for single and double escape key tests
* fixed focus bug on enter and removed use of keycode property
* fixed bug in opening list and improved property names for visual focus flags
* fixed bug in opening list with alt key pressed
* fixed bug with enter key not updating aria-expanded
* fixed bugs with down arrow
* added documentation and tests for ALT Down Arrow
* fixed some styling bugs
* Use only one SVG image to show listbox state
* updated CSS for styling listbox focus
* fixed scrolling issue in listbox
* adjusted position of svg image button
* fixed onclik bug where not selecting options when autocomplete is list or none
* updated test for autocomplete, list and none
* fixed bug with onclick behavior and documented option filtering as users type
* updated documentation about filtering of options
* Typo: character -> characters
* fixed option filter bug with autocomplete=none
* improved description of when listbox opens
* use lowercase "the" in sentence
* Minor editorial revision to alt+down in textbox keyboard table
* udpated test file to check for aria-selected on option when listbox opens
* fixed aria-selected tests
* add tests for aria-selected to key down tests
* Add aria-hidden=false and focusable=false to svg in button
* Use fewer descendant combinators in selectors
* Call function instead of setting a property on elements
* Don't call setValue() when hitting backspace, to avoid moving the cursor to the end
* Remove superfluous isPrintableCharacter() call
* Remove unused variable textContent
* Declare option outside for loop. Also use null instead of false
* Set scrollTop once
* Declare index variable in the if block
* Move isPrintableCharacter to ComboboxAutocomplete.prototype
* Add information about the id attribute for all combobox examples; fixes #785

Co-authored-by: Carolyn MacLeod <Carolyn_MacLeod@ca.ibm.com>
Co-authored-by: Matt King <a11yThinker@Gmail.com>
Co-authored-by: Valerie Young <spectranaut@gmail.com>
Co-authored-by: Simon Pieters <zcorpan@gmail.com>
@mcking65
Copy link
Contributor

@sinabahram wrote:

I finally got a chance to play with this ... Jaws 2020, latest, and Chrome latest.

I tried the first preview link "both", and I came across several issues. but not sure if this should be tested in Chrome+Jaws yet?

Yes, it is ready for testing. Bugs can fall into 3 buckets: 1) APG example code, 2) browser, and 3) screen reader. If you haven't yet read it, the section of the APG titled Read Me First explains the scope of practices in this regard. In particular, section 2.2 addresses browser and assistive technology support.

From a W3C perspective, browser bugs are the concern of the ARIA Working Group and screen reader bugs are the concern of the ARIA-AT community group.

1: the aria-expanded does not seem to be surfaced? I hit the open button, but it never says it's collapsed nor does it announce being in an expanded state. It does announce the transition between collapsed/expanded, which is super odd, but I didn't spend time to figure out if that was coming from an aria-live message on the page or from some native announcement of aria-expanded state change.

Correct, there is a JAWS bug because JAWS does not announce the state of the combobox when you navigate to it or read it.

When you click open with the virtual cursor, the mouse click event is causing DOM focus to move to the text input. JAWS announces the focus change event, then the expanded state changes, and JAWS announces the state change event.

When the combobox is expanded, and you move the reading cursor back to the open button and click it again, the DOM focus is still on the text input. Sso, JAWS is announcing the state change from expanded to collapsed.

The APG examples do not use live regions to compensate for anything that could be categorized as lack of screen reader support. Information about the state of the combobox, how many suggested values are available in a combobox popup, which suggestion is automatically selected if any, etc. is all available in the accessibility tree for any comboboxes that correctly implement the pattern. What assistive technologies do with that information is an assistive technology design decision. However, the aria-at project is seeking to build consensus around basic expectations for assistive technology behavior.

I am writing combobox tests for the aria-at project right now. Combobox testing is part of the ARIA-AT project production system design phase, so those tests will be among the first we review with screen reader developers. ARIA-AT will have must have, should have, and nice to have expectations for screen readers. Announcements about suggestion availability may fall into one of the latter 2 buckets.

2: the button opens a listbox, which allows for no interaction. Instead, when one focuses it, it just kicks you out of forms mode ... I tried lots of times.

This is an issue that the ARIA-AT project should highlight for Freedom Scientific. To activate forms mode for a listbox, JAWS issues MSAA default action. In this case, that will generate a click event. Clicking an item in the listbox will close the listbox and put the clicked value into the combobox. Ordinarily, JAWS users will navigate the listbox by pressing down arrow from within the combobox. However, ideally, users would be able to operate the combobox in either reading mode or forms mode. I don't think there is anything we can do about this in the example code.

3: In the combobox, several things jumped out at me.
A: The values wrap. That was unexpected, but I don't think I have a problem with this, especially since it can reduce the time it takes to find something even if all you can do is iterate previous/next, so I'm fine with that, but just noting that it was super-unexpected behavior.

We do this because home and end move the caret in the edit field. BTW, do you think the edit field should be in the up and down arrow navigation? Or, should it keep focus in the list?

B: Typing a letter seems to be a one-way path. Once I type 'N", I don't see any way of removing that from my filter. The character model is weirder than that, though, because it doesn't replace the value with 'n', or do what I think almost any screen reader user would expect, which is to be placed on a reduced list of items starting with the letter 'n'. Instead it adds 'n' to the end of whatever string I am on, but then if I use my arrow keys, seems to filter appropriately by starting with 'n'.

It looks like we still have some serious bugs related to typing, especially after deleting characters from the input. The suggestion list is borked. Thank you for pointing this out. I raised #1332.

C: I get zero automatic announcements of any found results or other type of dynamic information when typing or backspacing.

Yap, and this is considered to be a screen reader shortcoming per the above. We will address this in the ARIA-AT project.

D: Hitting backspace just erases the last letter typed or the last letter of whatever item is currently focused, but clearly not selected, since if it were selected, it would erase the whole term.

In the autocomplete both example, the first press of backspace will erase the autocompletion string -- the automatically suggested portion of the text. The second press would delete the last character typed. Compare tests 1 and 2 for the autocomplete both example with tests 3 and 4 for the autocomplete list example in issue #1332.

E: It can lose state sometimes, like I backspaced over a word until I got to a subset of that place's name which I know appears in other states e.g. I backspaced over Northern Mariana Islands, until I had "north", but then when I hit up/down, I didn't get North Carolina.

This is addressed by #1332.

4: The button says "open", but open what? That feels a bit like "read more" to me, where more specificity is required.

You are right on this. It is worse than that because when the combobox is expanded, nothing about the button changes. For compatibility with touch-based screen readers and reading cursors, this button should be marked up more like a menu button with aria-expanded and aria-controls. I'm surprised that we didn't consider this already. Again, thank you for calling attention to it. I have opened #1333.

Excellent feedback. Thank you!!

@sinabahram
Copy link

sinabahram commented Feb 17, 2020 via email

@smhigley
Copy link
Contributor

@sinabahram there are probably diminishing returns in debating combobox behavior in a merged PR thread :). If you feel like it's worth continued discussion, it would probably be better to open an issue.

That said, here are a couple brief notes in response to a couple of your points:

  • I'm generally inclined to agree with some of the usability concerns around inline autocomplete, spacebar to select, and autoselection (I even ran a usability study around those, among other behaviors, with results here: https://www.24a11y.com/2019/select-your-poison-part-2/). That said, inline autocomplete specifically did not present much of a barrier to the keyboard or screen reader users who participated. Both spacebar and autoselect had far greater impacts on the usability of the control.
  • Even if autocomplete with filtering performs worse in a usability study, that does not mean that there are no use cases where that behavior would be the right choice, or that those patterns should be forbidden. Given that they are valid use cases, it makes sense for us to provide examples in the ARIA Authoring Practices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment