1297976 - [One-off searches] Screen reader does not read the one-offs searches buttons

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[roxleitan]

[Affected versions]:
Nightly 51 Build ID 20160822064441
 
[Affected platforms]:
Windows 10 x64

[Steps to reproduce]:
1. Install and open NVDA screen reader
2. Open Firefox with a new profile
3. Type something in the Awesomebar
4. Hover over one-offs searches buttons

[Expected result]:
Screen reader should read the one-offs searches buttons

[Actual result]:
Screen reader does not read the one-offs searches buttons

[Additional notes]:
Reproducible also on Ubuntu 15.04 x64 with Orca Screen Reader
Screen reader also does not read the one-offs searches buttons from Search bar

Comment 1

[Drew Willcoxon :adw]

I'm going to bump this up to a P1 since it makes the one-offs unusable for some people and we can't ship without fixing it.

The only argument against that might be that the existing one-offs in the search bar aren't accessible either and we've been shipping them for a while now -- if that's even true, maybe it's not.  The urlbar and search bar use the same one-offs implementation but maybe the urlbar missed something that the searchbar is doing specifically for accessibility.  But even if it is true that's no excuse not to prioritize fixing it.

Comment 2

[Marco Bonardo [:mak]]

Just to be clear, I didn't set it as a P1 since based on latest definitions that's for "should be fixed in current version", vs P2 that is "should be fixed in next version". Since one-off are nightly-only there's no such hurry. (I'm not a big fan of those definitions, but that's what we got, that said, we clearly can manage priorities more freely).

Comment 3

[Florian Quèze [:florian]]

(In reply to Drew Willcoxon :adw from comment #1)

> The only argument against that might be that the existing one-offs in the
> search bar aren't accessible either and we've been shipping them for a while
> now -- if that's even true, maybe it's not.

See bug 1107695 for how we made the one-off buttons accessible for the searchbar. We fixed it when it regressed recently (bug 1279665).

Comment 4

[Drew Willcoxon :adw]

Attachment: Bug 1297976 - Fix accessibility for the unified one-off search buttons in the urlbar and searchbar.

Review commit: https://reviewboard.mozilla.org/r/77980/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77980/

Comment 5

[Drew Willcoxon :adw]

Bug 1180944 broke accessibility for the searchbar one-offs too.  In addition to the duplicate IDs (bug 1300053), aria-activedescendant is being set on the new one-offs binding instead of correctly on the textbox as it was before.

I tested this patch with NVDA on Windows 10, and the NVDA Speech Viewer tool shows the same output for the searchbar that it does for a Nightly before the urlbar one-offs landed.  The patch also fixes the urlbar one-offs.

To generate unique IDs, the patch (ab)uses telemetryOrigin.  I think that's OK, but let me know if you want to rename that property or if you want to do something else like use the ID of the binding parent element.

There's one hack in the patch.  For some reason, in the allowEmptySelection=false case (i.e., urlbar) in handleKeyPress, NVDA doesn't read the first one-off when you arrow-down to it from the last item in the results list, and similarly it doesn't read the last one-off when you arrow-up to it from the first item in the results list.  It looks like the this.popup.selectedIndex = -1 setter is preventing it because when I comment that out, there's no problem.  I'm hoping that the changes we end up making in bug 1295458 will let us remove it, but maybe not.

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8789958 [details]
Bug 1297976 - Fix accessibility for the unified one-off search buttons in the urlbar and searchbar.

https://reviewboard.mozilla.org/r/77980/#review76944

I'm fine with abusing the telemetryOrigin field for now (and yes, using the binding parent's id would be cleaner).

::: browser/components/search/content/search.xml:1787
(Diff revision 1)
>                    // Wrap around the selection to the last one-off.
>                    this.selectedButton = null;
>                    stopEvent = this.advanceSelection(false, true, true);
>                    if (stopEvent) {
>                      this.popup.selectedIndex = -1;
> +                    // NVDA doesn't read the newly selected one-off for the

Do you know if this workaround is needed because of a bug from our accessibility code or from NVDA? Is there a bug on file to cover this?

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 8789958 [details]
Bug 1297976 - Fix accessibility for the unified one-off search buttons in the urlbar and searchbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77980/diff/1-2/

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8789958 [details]
Bug 1297976 - Fix accessibility for the unified one-off search buttons in the urlbar and searchbar.

This moves the advanceSelection call after the popup.selectedIndex setter.  Fortunately that's possible because advanceSelection always returns true in these two cases, so there's no need to guard the selectedIndex setter with a stopEvent conditional, so there's no need to call advanceSelection before setting selectedIndex.  So that works around the problem.

One thing I noticed while doing more testing with this latest patch is that even before the refactoring, the first one-off isn't always read by NVDA.  I could be wrong but it seems like if I arrow down to it quickly, it's not read.  If instead I pause on the last list item for a moment and then arrow down, it is read.  This new patch has the same behavior, and since it's the same, I didn't look into why that happens.

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 8789958 [details]
Bug 1297976 - Fix accessibility for the unified one-off search buttons in the urlbar and searchbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77980/diff/2-3/

Comment 10

[Drew Willcoxon :adw]

Whoops, missed a stopEvent setter.  Boy this code is complex.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8789958 [details]
Bug 1297976 - Fix accessibility for the unified one-off search buttons in the urlbar and searchbar.

https://reviewboard.mozilla.org/r/77980/#review77220

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24cefbae2be9
Fix accessibility for the unified one-off search buttons in the urlbar and searchbar. r=florian

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/24cefbae2be9

Comment 14

[Liviu Cirdei]

This is verified on Aurora 51.0a2 (2016-09-21) and Nightly 52(2016-09-20). The screen reader now reads the one-off buttons and "Change search settings" button for both Awesomebar and Searchbar.
I tested on:
 * Windows 10 + NVDA
 * Ubuntu 15.04 + Orca Screen Reader
 * Mac 10.10 + Voice Over