If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in Firefox 51

Status

()

Firefox
Search
P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: roxana.leitan@softvision.ro, Assigned: adw)

Tracking

51 Branch
Firefox 51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified, firefox52 verified)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
[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
Priority: -- → P2
Whiteboard: [fxsearch]
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.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Priority: P2 → P1
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).
(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).
See Also: → bug 1300053
Depends on: 1300053
Comment hidden (mozreview-request)
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

a year ago
mozreview-review
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?
Attachment #8789958 - Flags: review?(florian) → review+
Comment hidden (mozreview-request)
Attachment #8789958 - Flags: review+
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.
Attachment #8789958 - Flags: review?(florian)
Comment hidden (mozreview-request)
Whoops, missed a stopEvent setter.  Boy this code is complex.

Comment 11

a year ago
mozreview-review
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
Attachment #8789958 - Flags: review?(florian) → review+

Comment 12

a year ago
Pushed by dwillcoxon@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/24cefbae2be9
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Blocks: 1303259

Comment 14

a year ago
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
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
status-firefox52: --- → verified
You need to log in before you can comment on or make changes to this bug.