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 Pattern and Examples: Convert to support draft ARIA 1.2 specification #1255

Merged
merged 54 commits into from
Nov 15, 2019

Conversation

@mcking65 mcking65 changed the title Combobox Pattern and Examples: Convert to support draft ARIA 1.2 specification WIP: Combobox Pattern and Examples: Convert to support draft ARIA 1.2 specification Nov 8, 2019
@jongund
Copy link
Contributor

jongund commented Nov 10, 2019

@mcking65 Added the grid combobox example to the branch

@jongund
Copy link
Contributor

jongund commented Nov 11, 2019

@mcking65 I updated the regression tests for the modified combobox examples for both, inline and none

@mcking65
Copy link
Contributor Author

@jongund, thank you, thank you for the updates!!! I finished the editorial updates to the grid combobox example page. So, now, except for the tests, all 4 examples are ready for review.

Do you have time to finish work on the tests?

Tomorrow, Simon will work on the pattern revisions, and then the entire PR will be ready for review.

examples/combobox/js/listbox.js Outdated Show resolved Hide resolved
examples/combobox/js/listboxOption.js Outdated Show resolved Hide resolved
aria-practices.html Outdated Show resolved Hide resolved
aria-practices.html Outdated Show resolved Hide resolved
aria-practices.html Outdated Show resolved Hide resolved
aria-practices.html Outdated Show resolved Hide resolved
@zcorpan
Copy link
Member

zcorpan commented Nov 11, 2019

@mcking65 and @jongund I would appreciate your review on my changes. Thanks!

Also see #1250 (comment)

@zcorpan
Copy link
Member

zcorpan commented Nov 12, 2019

We should update the tests as well. Edit: npm test currently fails a bunch of combobox tests.

@jongund
Copy link
Contributor

jongund commented Nov 12, 2019

@mcking65 Why did you change the test ids grid-combo.html? Everything was working before the changes. I work on fixing the test cases to use the new test ids.

@mcking65
Copy link
Contributor Author

@jongund asked:

@mcking65 Why did you change the test ids grid-combo.html? Everything was working before the changes. I work on fixing the test cases to use the new test ids.

Jon, I'm sorry ... I didn't mean to screw that up. I thought I made only changes where the test ID didn't match what needed to be tested. Technically, since we are using a text input, the ones that include textbox in the ID are fine, and I could have left that part of the ID alone.

@jongund
Copy link
Contributor

jongund commented Nov 12, 2019

@mcking65 I updated the grid combobox regression test files

@mcking65
Copy link
Contributor Author

Carolyn,thank you for the awesome reviews!!

Except for the 2 where I commented otherwise, I merged all your suggestions.

@carmacleod wrote:

Functional issues:

  • I agree with @smhigley that Escape should not clear the text input if the popup is open. The user just wants to close the popup at that point - they do not want their previous value to be erased without their consent. Typing Escape with the popup closed can clear the text input. Fixing this includes rewriting the keyboard interaction prose for Escape in the pattern and example.

Added this to #1066.

  • the black hover outline appears when hovering over what looks like empty space off to the right of the button (but is actually the combo's label element). This feels unexpected.

Is this something that could be addressed in #1263? Or added to one of the other open issues in the combobox project?

  • I somehow managed to cause the popup to show only "Utah" while the combobox value was "Wyoming" (the last value in the list - which I had not selected). This occurred on 2 separate occasions when returning to the open browser tab. I don't think this should be possible. :)
    proof of Wyoming-Utah bug

Opened #1268 and scheduled in next milestone.

@JAWS-test
Copy link

I believe this is solveable if Freedom Scientific wishes to solve it.

I'm afraid that won't happen because the problem has been there for many years and not a single improvement has happened since.

I think it is related to aria-activedescendant.

The problem has nothing to do with aria-activedescendant (this works fine now, it was just a problem in the past). The problem is that role=combobox is not recognized by JAWS. So it's not the list entries that are affected, but the element itself.

@mcking65
Copy link
Contributor Author

@jongund wrote:

In the ARIA practices section for combobox "WAI-ARIA Roles, States, and Properties"

Item 1

If the combobox element is editable, it has a value for aria-multiline of false. Note that the default value of aria-multiline is false.

  1. The design pattern mentions the use of aria-multiline=false if the combobox is editable, but none of our editable comnbobox examples defines the aria-multiline property. What is even more confusing is that the practices say the default value is false, so does it need to be defined or not? If not why do we say anything.

Removed it in commit dd6f6c0.

  1. We also do not say what to do if the values in the combobox can only be from the options in the popup.

The very first bullet says that the element that displays the value has role combobox. That's all that is needed. However, I do think that we might eventually benefit from a property something like "aria-valuefrom" that would take an ID and explicitly define the source of the value for the combobox.

Item 2

When a combobox receives focus, DOM focus is placed on the combobox element

Does this statement make any sense now that the textbox has the combobox role?

Certainly ... we need focus on that element. Obviously, it gets it by default if you use html input type text, but you would need to explicitly make the element focusable if it were not an element that is focusable by default.

@mcking65
Copy link
Contributor Author

@@charmarkk, thank you very much for the thorough review!! I believe I have merged all your suggestions.

@mcking65
Copy link
Contributor Author

@carmacleod wrote:

See example bug reported by @LaurenceRLewis in #1261 (comment) ... every instance of aria-haspopup="true" in the example code and in the Role, Property, State section of the examples should either be changed to aria-haspopup="listbox" or deleted completely.

Removed with commit e51ad04.

@spectranaut, This will need to be changed in the regression tests.

@carmacleod
Copy link
Contributor

  • the black hover outline appears when hovering over what looks like empty space off to the right of the button (but is actually the combo's label element). This feels unexpected.

Is this something that could be addressed in #1263? Or added to one of the other open issues in the combobox project?

Added to #1263 by @zcorpan.

@carmacleod
Copy link
Contributor

VoiceOver+Chrome on macOS Catalina 10.15.1

  • focusing the combobox and then typing down arrow: all 3 listbox examples say the list item twice, although the first time is cut short. This is most apparent in the autocomplete=none example for "w-weather, ch-cheap flights to NY".

  • the autocomplete=both example has some very weird behavior where sometimes only a part of the previously-selected list item is spoken as you arrow through the list. For example: focus the combo and type down arrow, and Alaska is selected and spoken (even though the first item is Alabama); type down arrow again, and VO says "lask", down arrow again to hear "merican Samoa Arizona". If you wait long enough before typing down arrow again, it may synchronize so that the selection matches what is spoken. But then down arrow again results in "District of Columbi". It's pretty unusable.

  • ctrl+alt+space (i.e. VO+space) does open the popup in the listbox (not grid) examples in Chrome (but does not focus the first item)

VoiceOver+Safari on macOS Catalina 10.15.1

  • VO does not read the list items in the autocomplete=list or autocomplete=none or grid examples in Safari. These 3 examples are completely unusable in VO+Safari on Catalina.

  • However, VO does read list items in the autocomplete=both example

  • VO+space does not open the list in Safari (for any example)

@mcking65
Copy link
Contributor Author

@carmacleod wrote:

VoiceOver+Chrome on macOS Catalina 10.15.1

  • focusing the combobox and then typing down arrow: all 3 listbox examples say the list item twice, although the first time is cut short. This is most apparent in the autocomplete=none example for "w-weather, ch-cheap flights to NY".
  • the autocomplete=both example has some very weird behavior where sometimes only a part of the previously-selected list item is spoken as you arrow through the list. For example: focus the combo and type down arrow, and Alaska is selected and spoken (even though the first item is Alabama); type down arrow again, and VO says "lask", down arrow again to hear "merican Samoa Arizona". If you wait long enough before typing down arrow again, it may synchronize so that the selection matches what is spoken. But then down arrow again results in "District of Columbi". It's pretty unusable.

Both these behaviors smell like bugs in our code and are also probably related to #1264. @spectranaut is working on #1264 today.

  • ctrl+alt+space (i.e. VO+space) does open the popup in the listbox (not grid) examples in Chrome (but does not focus the first item)

Interesting, confusing.

VoiceOver+Safari on macOS Catalina 10.15.1

The fact that they do work in 10.14.6 does not necessarily implicate Safari, but kind of points in that direction. @accdc has some different implementations of the same pattern we could test with 10.15. Apple may have implemented overly restrictive processing of aria-activedescendant, overlooking the fact that the ARIA spec supports activedescendant on a text input or combobox that controls an element that also supports activedescendant.

This is almost certainly because of the inline selection.

  • VO+space does not open the list in Safari (for any example)

VO-space is supposed to be equivalent to a mouse click on the element in the VO cursor. Given that, does VO announcing that VO-space should open the popup make sense?

@jongund
Copy link
Contributor

jongund commented Nov 14, 2019

@mcking65 do we distinguish the term "focus" from "DOM focus" in the practices?

Item 2

When a combobox receives focus, DOM focus is placed on the combobox element

Does this statement make any sense now that the textbox has the combobox role?

Certainly ... we need focus on that element. Obviously, it gets it by default if you use html input type text, but you would need to explicitly make the element focusable if it were not an element that is focusable by default.

@charmarkk
Copy link
Contributor

charmarkk commented Nov 14, 2019

@mcking65 wrote:

  • VO+space does not open the list in Safari (for any example)

VO-space is supposed to be equivalent to a mouse click on the element in the VO cursor. Given that, does VO announcing that VO-space should open the popup make sense?

I think that does make sense, Matt.

@zcorpan zcorpan dismissed their stale review November 14, 2019 19:44

My prior comments have been fixed

Copy link
Contributor

@charmarkk charmarkk left a comment

Choose a reason for hiding this comment

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

Approving all of my editorial changes that Matt committed. Feedback provided/solved and other work done also looks good to me - great work everyone!

@carmacleod carmacleod self-requested a review November 14, 2019 22:07
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.

+1 for working draft!

@mcking65 mcking65 merged commit 632bc8a into master Nov 15, 2019
@mcking65 mcking65 deleted the issue1244-aria1.2-combobox-conversion branch November 15, 2019 04:02
michael-n-cooper pushed a commit that referenced this pull request Nov 15, 2019
Combobox Pattern and Examples: Convert to support draft ARIA 1.2 specification (pull #1255)

To resolve #1250 and resolve #1244:
1. Revise the combobox pattern to remove the ARIA 1.0 and ARIA 1.1 guidance and replace with ARIA 1.2 guidance. Keeps a note about ARIA 1.0.
2. In examples, remove ARIA 1.0 and ARIA 1.1 pattern subdirectories and examples.
3. Convert the 3 ARIA 1.0 listbox popup examples into a 1.2 example.
4. Convert the ARIA 1.1 grid popup example into a 1.2 example.
5. Revise regression tests for combobox examples to test the 1.2 versions.
@carmacleod carmacleod mentioned this pull request May 13, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants