ARIA combobox should map to AXComboBox with role description of 'combo box'

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jdiggs, Assigned: jdiggs)

Tracking

unspecified
mozilla56
Unspecified
macOS
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

According to the Core AAM [1], the ARIA combobox role should map to 

AXRole: AXComboBox
AXSubrole: <nil>
AXRoleDescription: 'combo box'

Gecko is currently exposing this role as an AXPopUpButton with roledescription 'pop up button'.

[1] https://rawgit.com/w3c/aria/master/core-aam/core-aam.html#role-map-combobox
Posted patch proposed patch (obsolete) — Splinter Review
Assignee: nobody → jdiggs
Status: NEW → ASSIGNED
Attachment #8884236 - Flags: review?(mzehe)
Comment on attachment 8884236 [details] [diff] [review]
proposed patch

This will also affect the mapping for the native <select> element with size=1, which is indeed designed to behave like a popup button menu like in Safari. We're mimicking the native widget behavior as closely as possible with native HTML widgets. So if we change this mapping, we'll break every <select> element with size=1, or rather, its accessibility.

So if ARIA comboboxes indeed require a different handling on MacOS than <select size="1">, we'll have to have a specially cased ARIA combobox for this. r- for now.
Attachment #8884236 - Flags: review?(mzehe) → review-
Posted patch proposed patch (obsolete) — Splinter Review
(In reply to Marco Zehe (:MarcoZ) from comment #2)

> So if ARIA comboboxes indeed require a different handling on MacOS than
> <select size="1">, we'll have to have a specially cased ARIA combobox for
> this. r- for now.

Thanks for the review! And sorry about not catching the select-related mapping. This patch creates a new internal role. I went with EDITCOMBOBOX because the ARIA role combobox assumes there will be a text input, whereas the HTML select with size of 1 does not. And I verified (via accessibility inspector) that HTML select is not turning into a AXComboBox. Please let me know if I missed anything else.
Attachment #8884236 - Attachment is obsolete: true
Attachment #8884797 - Flags: review?(mzehe)
Comment on attachment 8884797 [details] [diff] [review]
proposed patch

Thanks, this looks correct now! And don't worry, this is not the first time Apple's popup button menus have caused a bit of confusion for people who normally use Windows or Linux. ;) r=me.
Attachment #8884797 - Flags: review?(mzehe) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed18b5a361f9
ARIA combobox should map to AXComboBox. r=marcoz
Keywords: checkin-needed
Backed out for timing out in a11y's accessible/tests/mochitest/events/test_valuechange.html and browser-chrome's accessible/tests/browser/e10s/browser_caching_value.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/582c6d6f8b04fc9774f2ac8cbdc673282bdd2e19

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ed18b5a361f9ee5f0656eb98e2fce54adbd93373&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable

Failure log a11y: https://treeherder.mozilla.org/logviewer.html#?job_id=113019935&repo=mozilla-inbound
[task 2017-07-10T13:47:30.863163Z] 13:47:30     INFO - TEST-PASS | accessible/tests/mochitest/events/test_valuechange.html | Wrong value of  'splitter'  
[task 2017-07-10T13:47:30.863897Z] 13:47:30     INFO - TEST-PASS | accessible/tests/mochitest/events/test_valuechange.html | Test with ID = ' 'splitter'  value changed' succeed.  Event 'value change' was handled. 
[task 2017-07-10T13:47:30.864437Z] 13:47:30     INFO - Invoke the ' 'combobox'  value changed' test { scenario #0: expected 'text value change' event;  }
[task 2017-07-10T13:47:30.865107Z] 13:47:30     INFO - Buffered messages finished
[task 2017-07-10T13:47:30.865791Z] 13:47:30     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/events/test_valuechange.html | Test timed out. 
[task 2017-07-10T13:47:30.866285Z] 13:47:30     INFO - reportError@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:121:7

Failure log browser-chrome: https://treeherder.mozilla.org/logviewer.html#?job_id=113019946&repo=mozilla-inbound
[task 2017-07-10T13:33:19.106117Z] 13:33:19     INFO - Value should change to @aria-valuetext when @aria-valuenow is removed
[task 2017-07-10T13:33:19.114514Z] 13:33:19     INFO - TEST-PASS | accessible/tests/browser/e10s/browser_caching_value.js | Wrong value of ['div@id="slider" node', address: [object HTMLDivElement], role: slider, address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessibleValue)]] - 
[task 2017-07-10T13:33:19.116470Z] 13:33:19     INFO - TEST-PASS | accessible/tests/browser/e10s/browser_caching_value.js | Wrong current value of ['div@id="slider" node', address: [object HTMLDivElement], role: slider, address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessibleValue)]] - 
[task 2017-07-10T13:33:19.118357Z] 13:33:19     INFO - TEST-PASS | accessible/tests/browser/e10s/browser_caching_value.js | Wrong minimum value of ['div@id="slider" node', address: [object HTMLDivElement], role: slider, address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessibleValue)]] - 
[task 2017-07-10T13:33:19.129275Z] 13:33:19     INFO - TEST-PASS | accessible/tests/browser/e10s/browser_caching_value.js | Wrong maximum value of ['div@id="slider" node', address: [object HTMLDivElement], role: slider, address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessibleValue)]] - 
[task 2017-07-10T13:33:19.134153Z] 13:33:19     INFO - TEST-PASS | accessible/tests/browser/e10s/browser_caching_value.js | Wrong minimum increment value of ['div@id="slider" node', address: [object HTMLDivElement], role: slider, address: [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessibleValue)]] - 
[task 2017-07-10T13:33:19.143291Z] 13:33:19     INFO - Initially value is not set for combobox
[task 2017-07-10T13:33:19.145112Z] 13:33:19     INFO - TEST-PASS | accessible/tests/browser/e10s/browser_caching_value.js | Correct value for ['input@id="combobox" node', address: [object HTMLInputElement], role: editcombobox, address: [xpconnect wrapped nsIAccessible]] - 
[task 2017-07-10T13:33:19.148178Z] 13:33:19     INFO - Value should change when @value attribute is updated
[task 2017-07-10T13:33:19.150094Z] 13:33:19     INFO - Buffered messages finished
[task 2017-07-10T13:33:19.159117Z] 13:33:19     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/e10s/browser_caching_value.js | Test timed out -
The reason for the failure(s) appears to be my not changing a couple of role-based comparisons to IsCombobox() calls in the code related to accessible-event emission. This patch fixes that, and makes the main test failure (which I could reproduce) go away (at least locally).

That said, I'm seeing other failures in both Linux and macOS that are combobox/select related but appear to be independent of my patch. Given the environment differences, before I ask for re-review or commit, I'd like to throw this patch at the official test servers to see if I've addressed all failures.
Attachment #8884797 - Attachment is obsolete: true
Flags: needinfo?(jdiggs)
The previous failure in test_expandable.xul was the result of a stupid mistake I made. This patch fixes that.
Attachment #8885225 - Attachment is obsolete: true
Comment on attachment 8885390 [details] [diff] [review]
Patch for try server prior to re-review

Giving this another r+ since there were some more code changes, and the try run looks OK, with a11y tests now passing onn all desktop platforms that I could see.
Attachment #8885390 - Flags: review+
Thanks Marco. I reviewed the failures shown in the try build / treeherder and they all have bug numbers and seem to be known failures, including those marked "unexpected." So I'll set the checkin-needed keyword and hope the commit sticks this time. :)

Thanks again to Mark for pushing my patches to try!
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53823f1fb9a1
ARIA combobox should map to AXComboBox. r=marcoz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/53823f1fb9a1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Whiteboard: [stockwell fixed:product]
Whiteboard: [stockwell fixed:product]
You need to log in before you can comment on or make changes to this bug.