1567384 - One-off search buttons no longer get accessibility focus

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[James Teh [:Jamie]]

STR (with the NVDA screen reader):

1. Focus the address bar.
2. Type test
3. Press up arrow.
   • Expected: NVDA should say "Search for "test" with: grouping, Change search settings button"
   • Actual: NVDA says "test" (or nothing without a fix for bug 1567377)

The one-off search code sets aria-activedescendant to fire a11y focus:
https://searchfox.org/mozilla-central/rev/da855d65d1fbdd714190cab2c46130f7422f3699/browser/components/search/content/search-one-offs.js#768

      this.textbox.setAttribute("aria-activedescendant", button.id);

Normally, aria-activedescendant only works on the element with DOM focus. Before bug 1513337, this.textbox referred to the textbox element, which (as a special exception) is considered to have focus when its child input has focus. Now, this.textbox refers to an ancestor hbox element, which does nothing because the hbox doesn't have DOM focus.

This should be fixed by having the one-off search code set aria-activedescendant on the focused input. It's probably also worth adding a test for these one-off buttons to accessible/tests/browser/events/browser_test_focus_urlbar.js.

Comment 1

[Dão Gottwald [:dao]]

I'll see how easily I can extend browser_test_focus_urlbar.js for this, but for now I'll just submit the one-line fix.

Comment 2

[Dão Gottwald [:dao]]

Attachment: Bug 1567384 - Set oneOffSearchButtons.textbox to UrlbarInput::inputField since there's no real textbox anymore. r=adw

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b1686f9d4c3
Set oneOffSearchButtons.textbox to UrlbarInput::inputField since there's no real textbox anymore. r=adw

Comment 4

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 9b1686f9d4c3 (bug 1567384) for browser-chrome failures at browser/base/content/test/performance/browser_urlbar_keyed_search.js

Backout: https://hg.mozilla.org/integration/autoland/rev/9723393bae6b61798bad3977567994af94781e02

Failure push: https://hg.mozilla.org/integration/autoland/rev/4db620732a3c1978e0d08148de5341d6a2398fef

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=257435667&repo=autoland&lineNumber=2663

task 2019-07-19T18:55:36.055Z] 18:55:36 INFO - TEST-START | browser/base/content/test/performance/browser_urlbar_keyed_search.js
[task 2019-07-19T18:55:45.039Z] 18:55:45 INFO - TEST-INFO | started process screentopng
[task 2019-07-19T18:55:45.521Z] 18:55:45 INFO - TEST-INFO | screentopng: exit 0
[task 2019-07-19T18:55:45.521Z] 18:55:45 INFO - Buffered messages logged at 18:55:36
[task 2019-07-19T18:55:45.521Z] 18:55:45 INFO - Entering test bound quantumbar
[task 2019-07-19T18:55:45.521Z] 18:55:45 INFO - Buffered messages logged at 18:55:37
[task 2019-07-19T18:55:45.521Z] 18:55:45 INFO - First opening, useAwesomebar=false
[task 2019-07-19T18:55:45.521Z] 18:55:45 INFO - Console message: [JavaScript Warning: "Key event not available on some keyboard layouts: key=“r” modifiers=“accel,alt” id=“key_toggleReaderMode”" {file: "chrome://mochikit/content/tests/SimpleTest/EventUtils.js" line: 870}]
[task 2019-07-19T18:55:45.522Z] 18:55:45 INFO - Console message: [JavaScript Warning: "Key event not available on some keyboard layouts: key=“r” modifiers=“accel,alt” id=“key_quickRestart”" {file: "chrome://mochikit/content/tests/SimpleTest/EventUtils.js" line: 870}]
[task 2019-07-19T18:55:45.522Z] 18:55:45 INFO - Console message: [JavaScript Warning: "Key event not available on some keyboard layouts: key=“i” modifiers=“accel,alt,shift” id=“key_browserToolbox”" {file: "chrome://mochikit/content/tests/SimpleTest/EventUtils.js" line: 870}]
[task 2019-07-19T18:55:45.522Z] 18:55:45 INFO - Buffered messages finished
[task 2019-07-19T18:55:45.523Z] 18:55:45 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_urlbar_keyed_search.js | reflow at __rebuild@chrome://browser/content/search/search-one-offs.js was encountered 2 times,
[task 2019-07-19T18:55:45.525Z] 18:55:45 INFO - it was expected to happen up to 1 times. - false == true - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/performance/head.js :: reportUnexpectedReflows :: line 200
[task 2019-07-19T18:55:45.526Z] 18:55:45 INFO - Stack trace:
[task 2019-07-19T18:55:45.528Z] 18:55:45 INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:reportUnexpectedReflows:200
[task 2019-07-19T18:55:45.529Z] 18:55:45 INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:withPerfObserver:701
[task 2019-07-19T18:55:45.531Z] 18:55:45 INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:runUrlbarTest:846
[task 2019-07-19T18:55:45.532Z] 18:55:45 INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/browser_urlbar_keyed_search.js:quantumbar:47
[task 2019-07-19T18:55:45.535Z] 18:55:45 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1346
[task 2019-07-19T18:55:45.536Z] 18:55:45 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1381
[task 2019-07-19T18:55:45.538Z] 18:55:45 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1209
[task 2019-07-19T18:55:45.539Z] 18:55:45 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
[task 2019-07-19T18:55:45.544Z] 18:55:45 INFO - Full stack:
[task 2019-07-19T18:55:45.545Z] 18:55:45 INFO - __rebuild@chrome://browser/content/search/search-one-offs.js:503:22
[task 2019-07-19T18:55:45.547Z] 18:55:45 INFO - async*_rebuild@chrome://browser/content/search/search-one-offs.js:441:18
[task 2019-07-19T18:55:45.548Z] 18:55:45 INFO - _on_popupshowing@chrome://browser/content/search/search-one-offs.js:1377:10
[task 2019-07-19T18:55:45.550Z] 18:55:45 INFO - handleEvent@chrome://browser/content/search/search-one-offs.js:175:23
[task 2019-07-19T18:55:45.551Z] 18:55:45 INFO - _openPanel@resource:///modules/UrlbarView.jsm:404:16
[task 2019-07-19T18:55:45.553Z] 18:55:45 INFO - onQueryResults@resource:///modules/UrlbarView.jsm:262:10
[task 2019-07-19T18:55:45.554Z] 18:55:45 INFO - runUrlbarTest/testFn/popup.onQueryResults@chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:776:9
[task 2019-07-19T18:55:45.555Z] 18:55:45 INFO - _notify@resource:///modules/UrlbarController.jsm:576:25
[task 2019-07-19T18:55:45.557Z] 18:55:45 INFO - receiveResults@resource:///modules/UrlbarController.jsm:176:10
[task 2019-07-19T18:55:45.558Z] 18:55:45 INFO - notifyResults@resource:///modules/UrlbarProvidersManager.jsm:388:23
[task 2019-07-19T18:55:45.560Z] 18:55:45 INFO - add@resource:///modules/UrlbarProvidersManager.jsm:394:7
[task 2019-07-19T18:55:45.562Z] 18:55:45 INFO - onSearchResult@resource:///modules/UrlbarProviderUnifiedComplete.jsm:141:13
[task 2019-07-19T18:55:45.563Z] 18:55:45 INFO - notify@resource://gre/modules/UnifiedComplete.jsm:2739:22
[task 2019-07-19T18:55:45.565Z] 18:55:45 INFO - notifyResult@resource://gre/modules/UnifiedComplete.jsm:2754:7
[task 2019-07-19T18:55:45.566Z] 18:55:45 INFO - _addMatch@resource://gre/modules/UnifiedComplete.jsm:2111:10
[task 2019-07-19T18:55:45.567Z] 18:55:45 INFO - _addSearchEngineMatch@resource://gre/modules/UnifiedComplete.jsm:1820:10
[task 2019-07-19T18:55:45.569Z] 18:55:45 INFO - _matchCurrentSearchEngine@resource://gre/modules/UnifiedComplete.jsm:1737:10
[task 2019-07-19T18:55:45.570Z] 18:55:45 INFO - async*_matchFirstHeuristicResult@resource://gre/modules/UnifiedComplete.jsm:1433:32
[task 2019-07-19T18:55:45.572Z] 18:55:45 INFO - asyncexecute@resource://gre/modules/UnifiedComplete.jsm:948:35
[task 2019-07-19T18:55:45.573Z] 18:55:45 INFO - asyncstartSearch/<@resource://gre/modules/UnifiedComplete.jsm:2896:28
[task 2019-07-19T18:55:45.575Z] 18:55:45 INFO - promise callback*startSearch@resource://gre/modules/UnifiedComplete.jsm:2896:8
[task 2019-07-19T18:55:45.576Z] 18:55:45 INFO - startQuery/<@resource:///modules/UrlbarProviderUnifiedComplete.jsm:150:23
[task 2019-07-19T18:55:45.577Z] 18:55:45 INFO - startQuery@resource:///modules/UrlbarProviderUnifiedComplete.jsm:132:11
[task 2019-07-19T18:55:45.579Z] 18:55:45 INFO - start@resource:///modules/UrlbarProvidersManager.jsm:300:30

Comment 5

[Marco Bonardo [:mak]]

please file a follow-up bug for test coverage, if not addressing that here.

Comment 6

[Marco Bonardo [:mak]]

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

Comment 7

[James Teh [:Jamie]]

I notice this now depends on bug 1567377. Just to clarify, aria-activedescendant still needs to go on the input, not the parent role="combobox", as documented in the ARIA spec. You may have had another reason for depending on that bug; please disregard if so.

Comment 9

[Dão Gottwald [:dao]]

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

I notice this now depends on bug 1567377. Just to clarify, aria-activedescendant still needs to go on the input, not the parent role="combobox", as documented in the ARIA spec. You may have had another reason for depending on that bug; please disregard if so.

This patch appears to break browser_test_focus_urlbar.js and I suspect that's related to bug 1567377, hence the dependency.

Comment 10

[Dão Gottwald [:dao]]

browser_test_focus_urlbar.js still times out on "Ensuring autocomplete focus on down arrow (2)" after the second patch from bug 1567377. This failure did not occur when I first attempted to land this: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9b1686f9d4c3608d6fb5be34fadba2460d919f6f

Jamie, any clue what's going on here?

Comment 11

[James Teh [:Jamie]]

This is a bug in the one-offs code:

1. The user presses down arrow.
2. The results list sets aria-activedescendant.
3. SearchOneOffs._updateStateForButton gets called with null.
4. _updateStateForButton removes aria-activedescendant, thus overriding 2).

This patch fixes the problem, but you may want to implement this differently:

diff --git a/browser/components/search/content/search-one-offs.js b/browser/components/search/content/search-one-offs.js
index 11cefed6a4efb..9484dd4ceae22 100644
--- a/browser/components/search/content/search-one-offs.js
+++ b/browser/components/search/content/search-one-offs.js
@@ -661,7 +661,10 @@ class SearchOneOffs {

     if (this.textbox) {
       if (!button) {
-        this.textbox.removeAttribute("aria-activedescendant");
+        let active = this.textbox.getAttribute("aria-activedescendant");
+        if (active && active.includes("-engine-one-off-item-")) {
+          this.textbox.removeAttribute("aria-activedescendant");
+        }
       } else {
         this.textbox.setAttribute("aria-activedescendant", button.id);
       }

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e41dc7906592
Set oneOffSearchButtons.textbox to UrlbarInput::inputField since there's no real textbox anymore. r=adw

Comment 13

[Cristian Brindusan [:cbrindusan]]

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

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you request beta uplift? Thanks!

Comment 15

[Dão Gottwald [:dao]]

Jamie, can you verify that this is fixed in Nightly? Thanks!

Comment 16

[James Teh [:Jamie]]

This mostly works, except that the first time a one-off search gets selected, it doesn't get a11y focus. Subsequent selections do get a11y focus. STR:

1. Press alt+d to focus the address bar.
2. Type: test
3. Press down arrow repeatedly until you hit the last suggestion.
4. Press down arrow again.
   • Expected: The first one-off search should get a11y focus.
   • Actual: it doesn't; aria-activedescendant on #urlbar-input is null.
5. Press down arrow again.
   • Observe: The second one-off search gets a11y focus.
6. Press escape twice to reset.
7. Type: test
8. Press up arrow.
   • Expected: The "Change search settings" button should get a11y focus.
   • Actual: it doesn't; aria-activedescendant on #urlbar-input is null.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, Dão, can you file a new bug for the leftover issues?

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 9079371 [details]
Bug 1567384 - Set oneOffSearchButtons.textbox to UrlbarInput::inputField since there's no real textbox anymore. r=adw

Beta/Release Uplift Approval Request

• User impact if declined: accessibility is broken for one off search buttons
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: Jamie has verified this, I'm fixing the remaining issue in bug 1580822 where I'm also adding a test.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): trivial fix that can't break accessibility more than it already is
• String changes made/needed:

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9079371 [details]
Bug 1567384 - Set oneOffSearchButtons.textbox to UrlbarInput::inputField since there's no real textbox anymore. r=adw

Accessibility fix, let's uplift for beta 7.

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d9c4ede99bc1