1567377 - Accessibility broken due to aria-owns on address bar input

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[James Teh [:Jamie]]

After bug 1513337, the address bar suggestions are pretty broken for screen readers (and probably other accessibility clients).

STR (with the NVDA screen reader):

1. Focus the address bar.
2. Type test
3. Press backspace.
   • Expected: NVDA should say "t"
   • Actual: It says nothing.
4. press right arrow.
5. Press NVDA+upArrow to report the current line.
   • Expected: NVDA should say "tes"
   • Actual: It says nothing.
6. Press NVDA+tab to report the focus.
   • Expected: NVDA should say just "Search with Google or enter address combo box focused expanded has auto complete editable, tes"
   • Actual: NVDA says this, followed by "list graphic" and all of the search results.
7. Press down arrow to move to a suggestion.
   • Expected: NVDA should report the suggestion.
   • Actual: NVDA says "unknown".

This is due to aria-owns="urlbarView-results" on the input. As I noted in bug 1522440 comment 15, this was previously a no-op because of the different id scope. After bug 1513337, it takes effect and causes some undesirable side effects.

There are two problems here:

1. Making the list owned by the input means that the content of the list is treated as part of the input. This is the cause of the broken behaviour outlined in steps 5 and 6 above.

   We can fix this by putting role="combobox" on the moz-input-box parent and moving aria-owns to that.

2. #urlbarView-results is inside a popup panel, which means it has a different OS widget (HWND on Windows). When a focus event is fired for the selected suggestion, on Windows, an event gets fired with the HWND of the popup and the id of the suggestion. The client then retrieves the accessible for that HWND (the panel element) and asks it for the id of the suggestion. Unfortunately, since the suggestion has been re-parented with aria-owns, it's no longer a descendant of the panel, and thus we fail:

   https://searchfox.org/mozilla-central/rev/da855d65d1fbdd714190cab2c46130f7422f3699/accessible/windows/msaa/AccessibleWrap.cpp#1448

   I've been trying to come up with a way to deal with this in the accessibility code, but it's tricky because the whole point of this code is to make sure a client doesn't ask for a descendant id which isn't actually a descendant.

   My current thinking is to have aria-owns="urlbar-results" (the panel element) instead of urlbarView-results. This way, the suggestion is still a descendant of the popup panel. The only problem with this is that the list won't get role COMBOBOX_LIST because now it's a grandchild of the combobox instead of a child. We can probably deal with that in the accessibility code, though.

Comment 1

[James Teh [:Jamie]]

The other option here is to remove aria-owns altogether and revert bug 1535659. That effectively gets us back to the behaviour before bug 1513337 (since aria-owns was a no-op before then). That's a bit ugly (and results in an extra announcement of "combo box" in NVDA at least when the suggestions list gets focus), but probably less work (and lower risk).

Comment 2

[Marco Zehe (:MarcoZ)]

Note that NVDA's alpha version, after pressing ctrl+t, then typing t for "test", produces this in the error log, which basically seems to confirm all tht Jamie said above:

IO - inputCore.InputManager.executeGesture (09:23:55.558):
Input: kb(laptop):t
DEBUGWARNING - NVDAObjects.IAccessible.IAccessible._get_IAccessibleRole (09:23:55.657):
accRole failed: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))
ERROR - core.CorePump.run (09:23:56.078):
errors in this core pump cycle
Traceback (most recent call last):
File "core.pyc", line 492, in run
File "braille.pyc", line 2139, in pumpAll
File "braille.pyc", line 1887, in handlePendingCaretUpdate
File "braille.pyc", line 1893, in _doCursorMove
File "braille.pyc", line 956, in update
File "braille.pyc", line 811, in _addTextWithFields
File "NVDAObjects\IAccessible\ia2TextMozilla.pyc", line 342, in getTextWithFields
File "NVDAObjects\IAccessible\ia2TextMozilla.pyc", line 290, in _getText
AttributeError: 'NoneType' object has no attribute 'obj'
DEBUGWARNING - NVDAObjects.IAccessible.IAccessible._get_IAccessibleRole (09:23:56.109):
accRole failed: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))
DEBUGWARNING - NVDAObjects.IAccessible.IAccessible._get_IAccessibleRole (09:23:56.117):
accRole failed: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))
DEBUGWARNING - NVDAObjects.IAccessible.IAccessible._get_IAccessibleRole (09:23:56.121):
accRole failed: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))
DEBUGWARNING - NVDAObjects.IAccessible.IAccessible._get_IAccessibleRole (09:23:56.127):
accRole failed: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))
DEBUGWARNING - NVDAObjects.IAccessible.IAccessible._get_IAccessibleRole (09:23:56.132):
accRole failed: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))
DEBUGWARNING - NVDAObjects.IAccessible.IAccessible._get_IAccessibleRole (09:23:56.138):
accRole failed: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))
DEBUGWARNING - NVDAObjects.IAccessible.IAccessible._get_IAccessibleRole (09:23:56.142):
accRole failed: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))
DEBUGWARNING - NVDAObjects.IAccessible.IAccessible._get_IAccessibleRole (09:23:56.148):
accRole failed: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))
DEBUGWARNING - NVDAObjects.IAccessible.IAccessible._get_IAccessibleRole (09:23:56.154):
accRole failed: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))
DEBUGWARNING - NVDAObjects.IAccessible.IAccessible._get_IAccessibleRole (09:23:56.160):
accRole failed: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))
DEBUGWARNING - NVDAObjects.IAccessible.IAccessible._get_IAccessibleRole (09:23:56.164):
accRole failed: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))

Comment 3

[Dão Gottwald [:dao]]

(In reply to James Teh [:Jamie] from comment #0)

1. Making the list owned by the input means that the content of the list is treated as part of the input. This is the cause of the broken behaviour outlined in steps 5 and 6 above.

   We can fix this by putting role="combobox" on the moz-input-box parent and moving aria-owns to that.

Sigh... moz-input-box is supposed to go away at some point so we probably don't want to use that.

I was wondering why the input is wrapped in a seemingly useless div here: https://www.w3.org/TR/wai-aria-practices/examples/combobox/aria1.1pattern/listbox-combo.html#sc1_label
Now I begin to understand. This seems ugly and I don't understand how authors are supposed to know this in the first place. The whole combobox accessibility story seems brittle and not at all intuitive.

2. #urlbarView-results is inside a popup panel, which means it has a different OS widget (HWND on Windows). When a focus event is fired for the selected suggestion, on Windows, an event gets fired with the HWND of the popup and the id of the suggestion. The client then retrieves the accessible for that HWND (the panel element) and asks it for the id of the suggestion. Unfortunately, since the suggestion has been re-parented with aria-owns, it's no longer a descendant of the panel, and thus we fail:

Bug 1551598 is moving us away from using a popup.

Comment 4

[James Teh [:Jamie]]

(In reply to Dão Gottwald [::dao] from comment #3)

1. Making the list owned by the input means that the content of the list is treated as part of the input. This is the cause of the broken behaviour outlined in steps 5 and 6 above.
   We can fix this by putting role="combobox" on the moz-input-box parent and moving aria-owns to that.

Sigh... moz-input-box is supposed to go away at some point so we probably don't want to use that.

A wrapping span would work just as well.

I was wondering why the input is wrapped in a seemingly useless div here: https://www.w3.org/TR/wai-aria-practices/examples/combobox/aria1.1pattern/listbox-combo.html#sc1_label
Now I begin to understand. This seems ugly and I don't understand how authors are supposed to know this in the first place. The whole combobox accessibility story seems brittle and not at all intuitive.

I agree to a large extent, though the ARIA 1.1 spec is pretty clear at least about hierarchy:

A composite widget containing a single-line textbox and another element, such as a listbox or grid, that can dynamically pop up to help the user set the value of the textbox.
...
Authors MUST ensure an element with role combobox contains or owns a text input element with role textbox
...
When a combobox is expanded, authors MUST ensure it contains or owns an element that has a role of listbox, tree, grid, or dialog. This element is the combobox popup.

The idea is that a combobox is semantically a container for the other two things; it groups them together. The problem with the combobox role being applied to the input and then the input "owning" (re-parenting) the list is that the list then becomes part of the input's text, which is precisely the problem we're seeing here.

Bug 1551598 is moving us away from using a popup.

It looks like bug 1551598 might be a few weeks away at best. Is that correct? If so, I'd prefer to implement a workaround here in the short term, as otherwise, the address bar is effectively broken for screen reader users of Nightly in the meantime.

Comment 5

[James Teh [:Jamie]]

Attachment: Bug 1567377: Remove aria-owns from URL bar to fix broken accessibility.

1. Remove aria-owns from the URL bar.
   Before bug 1513337, this was a no-op because it crossed anonymous scope, which accessibility doesn't allow.
   After bug 1513337 when this started taking effect, it caused some undesirable side effects.
   Bug 1551598 should mitigate some of these issues, so we can re-evaluate then.

2. Revert bug 1535659, which removed role="combobox" from the URL bar results parent.
   While the intention was to make a cleaner accessibility hierarchy, without aria-owns, we need this to ensure the correct role for the results list.

Comment 6

[Marco Bonardo [:mak]]

[Tracking Requested - why for this release]: Accessibility regression

Comment 7

[Dão Gottwald [:dao]]

(In reply to James Teh [:Jamie] from comment #4)

Bug 1551598 is moving us away from using a popup.

It looks like bug 1551598 might be a few weeks away at best. Is that correct?

No, it's supposed to get done this week.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f4a78cd1973
Remove aria-owns from URL bar to fix broken accessibility. r=dao

Comment 9

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/8f4a78cd1973

Comment 10

[Marco Zehe (:MarcoZ)]

Jamie, now that both bug 1551598 and bug 1566324 have landed, what else is needed here?

Comment 11

[James Teh [:Jamie]]

With these fixes, we could now put role="combobox" aria-owns="urlbarView-results" on the moz-input-box parent. However:

(In reply to Dão Gottwald [::dao] from comment #3)

Sigh... moz-input-box is supposed to go away at some point so we probably don't want to use that.

I guess this is bug 1556566? As I see it, we have three options:

1. Put the attributes on the moz-input-box for now and move them to a wrapping span after bug 1556566 is fixed.
2. Wrap the moz-input-box or the input in a span with the attributes above. After bug 1556566, the moz-input-box will be gone, leaving the wrapping span.
3. Leave things as they are right now until bug 1556566 is fixed and add the wrapping span after that.

Dao, what would you prefer?

Comment 12

[Dão Gottwald [:dao]]

Let's go with option 1. Thanks!

Comment 13

[James Teh [:Jamie]]

I'm working on this. When I apply aria-owns to the moz-input-box, aria-activedescendant sometimes fails to fire a11y focus events and thus the a11y test times out. I'm not sure why yet; I'll investigate further.

Comment 14

[James Teh [:Jamie]]

This is annoying. Distilled test case:
data:text/html,<div tabindex="0" role="combobox" aria-owns="listbox" onkeydown="listbox.hidden = false; this.setAttribute('aria-activedescendant', 'option');"></div><div id="listbox" hidden role="listbox"><div id="option" role="option">option</div></div>
Tab to the combo box and press down arrow. You should get a focus event for the option, but you don't.
I think what's happening is this:

1. The autocomplete results list is collapsed and thus not in the a11y tree. If you press down arrow at this point, it will simultaneously create the list and focus the first item.
2. We see the aria-activedescendant change and schedule DocAccessible::ARIAActiveDescendantChanged.
3. WillRefresh is called.
4. We process insertions. Because aria-owns is present, we don't create the subtree for the list. DocAccessible::RelocateARIAOwnedIfNeeded schedules a relocation and returns false when called here: https://searchfox.org/mozilla-central/rev/29cce9a2684ef64c4f1f996087da8b7545d31f65/accessible/base/TreeWalker.cpp#340
5. We process generic notifications, including ARIAActiveDescendantChanged. The subtree for the results list still doesn't exist, so we fail to get an accessible for the active descendant.
6. Finally, we process relocations. The subtree for the list gets created.. too late!

I'm guessing there's a good reason we process all other notifications before invalidations and relocations. I understand what relocations are, but not invalidations yet. Moving invalidations/relocations before generic notifications fixes this, but I'm guessing it might break other things.

Eitan, any thoughts?

Comment 15

[James Teh [:Jamie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51c6e706e5d598e1b529b08378cfd8d2e03cc520

Comment 16

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #14)

Moving invalidations/relocations before generic notifications fixes this, but I'm guessing it might break other things.

FWIW, all a11y mochitests and browser-chrome tests pass with this change. See try run in comment 15.

Comment 17

[James Teh [:Jamie]]

Filed bug 1571327 for comment 14.

Comment 18

[James Teh [:Jamie]]

Attachment: Bug 1567377: Correct a11y semantics for address bar.

For accessibility, the address bar is now exposed as a parent combo box which contains the input and the results list.
The combo box role on urlbarView-body-inner is no longer needed and has therefore been removed.
This means screen readers no longer report an extraneous combo box whenever the results list is opened.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4cbaa7f6db9
Correct a11y semantics for address bar. r=dao

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cce9199bd9f6
Backed out changeset f4cbaa7f6db9 for causing several browser chrome failures. CLOSED TREE

Comment 21

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&revision=f4cbaa7f6db9df40de9722a780f8c34fd938bcc9&searchStr=browser%2Cchrome&group_state=expanded&selectedJob=260319167

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=260319167&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=260318609&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/cce9199bd9f6f1948b1b8dcefc3dead6b77ed5b9

[task 2019-08-07T10:07:41.681Z] 10:07:41 INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_autocomplete_a11y_label.js | Should still have expected searches remaining - 3 >= 0 -
[task 2019-08-07T10:07:41.683Z] 10:07:41 INFO - Buffered messages finished
[task 2019-08-07T10:07:41.683Z] 10:07:41 INFO - TEST-UNEXPECTED-FAIL | browser/components/urlbar/tests/browser/browser_autocomplete_a11y_label.js | Uncaught exception - undefined - timed out after 50 tries.
[task 2019-08-07T10:07:41.684Z] 10:07:41 INFO - Leaving test bound searchSuggestions
[task 2019-08-07T10:07:41.685Z] 10:07:41 INFO - GECKO(3123) | MEMORY STAT | vsize 3112MB | residentFast 402MB | heapAllocated 136MB
[task 2019-08-07T10:07:41.686Z] 10:07:41 INFO - TEST-OK | browser/components/urlbar/tests/browser/browser_autocomplete_a11y_label.js | took 6487ms
[task 2019-08-07T10:07:41.687Z] 10:07:41 INFO - GECKO(3123) | ++DOCSHELL 0x7f6af87ce000 == 1 [pid = 3192] [id = {360f3bbb-606c-4576-91a1-371fc6adda9c}]
[task 2019-08-07T10:07:41.697Z] 10:07:41 INFO - GECKO(3123) | ++DOMWINDOW == 2 (0x7f6af8c71d40) [pid = 3192] [serial = 35] [outer = (nil)]
[task 2019-08-07T10:07:41.698Z] 10:07:41 INFO - GECKO(3123) | [Child 3192, Main Thread] WARNING: No active window: file /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSContext.cpp, line 664
[task 2019-08-07T10:07:41.698Z] 10:07:41 INFO - GECKO(3123) | ++DOMWINDOW == 3 (0x7f6af876ac00) [pid = 3192] [serial = 36] [outer = 0x7f6af8c71d40]
[task 2019-08-07T10:07:41.699Z] 10:07:41 INFO - GECKO(3123) | [Parent 3123, Main Thread] WARNING: Constructing RangeBoundary with invalid value: 'mRef || aOffset == 0', file /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RangeBoundary.h, line 79
[task 2019-08-07T10:07:41.699Z] 10:07:41 INFO - GECKO(3123) | [Parent 3123, Main Thread] WARNING: Constructing RangeBoundary with invalid value: 'mRef || aOffset == 0', file /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RangeBoundary.h, line 79
[task 2019-08-07T10:07:41.700Z] 10:07:41 INFO - GECKO(3123) | ++DOMWINDOW == 4 (0x7f6af8771c00) [pid = 3192] [serial = 37] [outer = 0x7f6af8c71d40]
[task 2019-08-07T10:07:41.702Z] 10:07:41 INFO - checking window state
[task 2019-08-07T10:07:41.703Z] 10:07:41 INFO - GECKO(3123) | [Child 3192, Main Thread] WARNING: NS_ENSURE_SUCCESS(mStatus, *this) failed with result 0x80004005: file /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIURIMutator.h, line 489
[task 2019-08-07T10:07:41.703Z] 10:07:41 INFO - GECKO(3123) | JavaScript error: resource://testing-common/PromiseTestUtils.jsm, line 112: uncaught exception: Object
[task 2019-08-07T10:07:41.704Z] 10:07:41 INFO - TEST-START | browser/components/urlbar/tests/browser/browser_autocomplete_autoselect.js

[task 2019-08-07T10:04:52.812Z] 10:04:52 INFO - TEST-PASS | accessible/tests/browser/events/browser_test_focus_urlbar.js | Expanded [DOM node id: urlbar-input, role: entry, name: 'Search with Google or enter address', address: 0x7fa2bb616c40] cannot be collapsed! -
[task 2019-08-07T10:04:52.813Z] 10:04:52 INFO - Ensuring autocomplete focus on down arrow (2)
[task 2019-08-07T10:04:52.813Z] 10:04:52 INFO - Buffered messages finished
[task 2019-08-07T10:04:52.817Z] 10:04:52 INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/events/browser_test_focus_urlbar.js | Test timed out -
[task 2019-08-07T10:04:52.820Z] 10:04:52 INFO - GECKO(1074) | [Parent 1074, Main Thread] WARNING: Constructing RangeBoundary with invalid value: 'mRef || aOffset == 0', file /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RangeBoundary.h, line 79
[task 2019-08-07T10:04:52.820Z] 10:04:52 INFO - GECKO(1074) | [Parent 1074, Main Thread] WARNING: Constructing RangeBoundary with invalid value: 'mRef || aOffset == 0', file /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RangeBoundary.h, line 79
[task 2019-08-07T10:04:52.821Z] 10:04:52 INFO - GECKO(1074) | [Child 1393, Main Thread] WARNING: No active window: file /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSContext.cpp, line 664
[task 2019-08-07T10:04:52.821Z] 10:04:52 INFO - GECKO(1074) | [Parent 1074, Main Thread] WARNING: Suboptimal indexes for the SQL statement 0x7fa2b4d708d0 (http://mzl.la/1FuID0j).: file /builds/worker/workspace/build/src/storage/mozStoragePrivateHelpers.cpp, line 108
[task 2019-08-07T10:04:52.822Z] 10:04:52 INFO - GECKO(1074) | MEMORY STAT | vsize 2989MB | residentFast 324MB | heapAllocated 91MB
[task 2019-08-07T10:04:52.824Z] 10:04:52 INFO - TEST-OK | accessible/tests/browser/events/browser_test_focus_urlbar.js | took 90307ms

Comment 22

[James Teh [:Jamie]]

These failures are caused by a regression introduced with the recent landing of bug 686400 (which I didn't have in my tree when I was testing locally). Filed bug 1572317 to deal with this.

Comment 23

[:Gijs (he/him)]

With bug 1572317 landed, is this ready to reland?

Comment 24

[James Teh [:Jamie]]

Triggered reland.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aca32125501c
Correct a11y semantics for address bar. r=dao

Comment 26

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/aca32125501c