Search suggestions prompt breaks accessibility of location bar suggestions

RESOLVED FIXED in Firefox 43

Status

()

defect
P1
normal
Rank:
15
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Jamie, Assigned: adw)

Tracking

({access, regression})

Trunk
Firefox 43
x86
Windows
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [suggestions][fxsearch])

Attachments

(1 attachment)

Reporter

Description

4 years ago
(Spun off bug 1190368 comment 27.)

STR:
1. In about:config, ensure that browser.urlbar.userMadeSearchSuggestionsChoice is false.
2. Focus the location bar.
3. Type anything that will cause search suggestions to appear.
4. Press down arrow to focus a suggestion.
5. Observe that an accessibility focus event is fired.
6. Retrieve the accessible for this event.
Expected: The accessible for the suggestion list item focused in step 4.
Actual: A broken oleacc client accessible.

The list appears in a separate MozillaDropShadowWindowClass HWND. However, when the search suggestions prompt is displayed, WM_GETOBJECT returns the label for the search suggestions prompt and accChild for the search list items fails on this label accessible.

When the search suggestions prompt is *not* displayed, the list works as expected. In that case, WM_GETOBJECT on the MozillaDropShadowWindowClass returns the list (as expected) and therefore accChild works.

:Gijs noted, "The prompt is inside the listbox because that makes visual sense." As far as accessibility is concerned, the label isn't inside the richlistbox; it is a sibling of it. It certainly makes sense that accChild on this label would fail for the list items.

The question is why WM_GETOBJECT is returning the label; i.e. why the label is considered the root accessible for this HWND. I'm guessing there should really be a parent of both the label and the list which is considered the root accessible for the HWND. accChild on this parent should work to retrieve any of its descendants.

Marking as a regression because although this search suggestions stuff is new functionality, it effectively breaks the location bar autocomplete for users who haven't dismissed the search suggestions prompt.
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [suggestions][fxsearch]

Updated

4 years ago
Rank: 15
Marco, can you help us understand how much of a problem this is?
Flags: needinfo?(mzehe)
Reporter

Comment 2

4 years ago
Not meaning to answer for Marco, but just giving my opinion:
This basically makes the location bar autocomplete *completely unusable* for any screen reader user that has not yet dismissed this search suggestions prompt. Of course, a user can't be expected to know that this prompt is causing the brokenness. Furthermore, a screen reader user won't even be aware of that prompt because of bug 1198687.
That's all I needed to know, thanks
Flags: needinfo?(mzehe)
Assignee

Comment 4

4 years ago
Thanks for filing, James.  Making this block the bug where the opt-in was implemented since that's what caused it.
Blocks: 959567
No longer blocks: 958204, UnifiedComplete
Assignee

Comment 5

4 years ago
James, are there aria attributes we could use to describe to screen readers the relationship between the list and the notification?  Changing the DOM is not so easy because we have animations and XUL flex and... ugh.  I played around a little with aria-owns and some others but no dice.
Flags: needinfo?(jamie)
Reporter

Comment 6

4 years ago
The alert probably shouldn't be *inside* the list anyway, at least as far as a11y is concerned. Ideally, the list and the alert would be children of the same parent container. In this case, it doesn't look like there is a parent container you can use.

If we were just worried about the hierarchy, having it as a sibling of the list the way it is now is probably okay (if not ideal) once bug 1198687 is fixed. However, the real problem is that a11y can't seem to handle both the notification and the list being at the root of this pop-up HWND. When the alert is displayed, a11y tries to return the alert as the root, which breaks the list. I don't know if there's some way to fix/work around this or whether the only way is to have a parent container. Marco, any thoughts?
Flags: needinfo?(jamie)
(In reply to James Teh [:Jamie] from comment #6)
> If we were just worried about the hierarchy, having it as a sibling of the
> list the way it is now is probably okay (if not ideal) once bug 1198687 is
> fixed. However, the real problem is that a11y can't seem to handle both the
> notification and the list being at the root of this pop-up HWND. When the
> alert is displayed, a11y tries to return the alert as the root, which breaks
> the list.

Actually, we would like the user to make a choice when the notification is shown. as I understand it, this bug only exists until the user made a choice, then everything works properly.
So, if we fix bug 1198687, would the status quo be acceptable and understandable? it would basically require to make a choice before accessing the list.

The positioning here is quite fragile and I'm not sure we can find a better solution in a short time.
Reporter

Comment 8

4 years ago
I'm afraid this isn't workable. The problem is that the list items fire focus even when the notification is shown, but because the notification is shown, this focus is broken. Due to the nature of this brokenness, the broken focus appears as a child of the alert, and when NVDA (and perhaps other screen readers) gets an alert event on an ancestor of the focus, it ignores it (because the alert is already focused and thus doesn't need to be read like an unfocused notification). Also, it's pretty ugly because the user hears unknown every time one of those broken list items gets focus. That will confuse many users. They won't understand that dismissing the notification will magically make these "unknown" things go away.
Assignee

Updated

4 years ago
Assignee: nobody → adw
Status: NEW → ASSIGNED
Assignee

Comment 9

4 years ago
A hack fix would be to suppress events for the items as long as the notification is present, effectively forcing the user to answer it before using the urlbar.  Bug 1190368 already added a way to do that.
Assignee

Comment 10

4 years ago
(In reply to James Teh [:Jamie] from comment #6)
> The alert probably shouldn't be *inside* the list anyway, at least as far as
> a11y is concerned. Ideally, the list and the alert would be children of the
> same parent container.

As I explained in the other bug, this is actually the case -- sort of.  The alert and the richlistbox are siblings in the same parent, it's just that they're anonymous content in the same parent binding.
Assignee

Comment 11

4 years ago
Posted patch patchSplinter Review
Oh hey, this works.  This is based off the patch in bug 1198687.  The experience is not great.  Along with the problems with the patch described in that bug, NVDA reads the first item first, and then it reads the alert.  Ideally it would read the alert first I would think.

https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/dwillcoxon@mozilla.com-131f749d0fce

https://treeherder.mozilla.org/#/jobs?repo=try&revision=131f749d0fce
Attachment #8657391 - Flags: review?(mak77)
Attachment #8657391 - Flags: feedback?(jamie)
(In reply to Drew Willcoxon :adw from comment #11)
> Oh hey, this works.  This is based off the patch in bug 1198687.  The
> experience is not great.  Along with the problems with the patch described
> in that bug, NVDA reads the first item first, and then it reads the alert. 
> Ideally it would read the alert first I would think.

I tried building locally and tested with nvda, for me (on win10) the alert is read first.
Also nothing is read from the list until i key down.
The only downside I see is that as soon as the second letter is entered, it stops annoucing the alert, so some users may skip it inadvertently, but it would fire again at the next use...

I confirm that sometimes the alert doesn't fire, but it does most of the times.
Attachment #8657391 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/e831db739c39
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Assignee

Updated

4 years ago
Attachment #8657391 - Flags: feedback?(jamie)
Reporter

Comment 15

4 years ago
I'm still usually seeing this brokenness even once this prompt is dismissed. It seems to work briefly when I restart Firefox, but after using the list once or twice, it stops working and NVDA just reports "unknown" for every list item that is focused.

When it works, in the "PopupAutoCompleteRichResult" panel, I see two accessibles: a "Change search settings" button and the results list. When it doesn't, I see only the button. It's like the list accessible just disappears from the hierarchy and can't be accessed, but Gecko obviously still knows about it since it's firing (invalid) focus events.
Assignee

Comment 16

4 years ago
It's working reliably for me with the NVDA speech viewer.  I have speech turned off so I don't know whether it's speaking what the viewer shows.  I'm not using NVDA all the time though, only to test this bug, so my experience is certainly limited.
Reporter

Comment 17

4 years ago
I can repro this with NVDA as follows:
1. Open a new browser window.
2. Focus the address bar.
3. Type "asdf".
4. Press down arrow.
Observe: The search suggestion is read as expected.
5. Press escape to dismiss the suggestion.
6. Press down arrow twice. (I'm not sure why you have to press it twice, but nothing happens the first time.)
Expected: A search suggestion should be read.
Actual: NVDA reports "unknown".
From this point on, I only ever get "unknown" when I navigate search suggestions in this window. If I open a new browser window, it works the first time as above.
please file a separate bug for any remaining issues, we can't track closed bugs.
Assignee

Comment 19

4 years ago
Sigh, I can reproduce on Nightly but not on my local build, which is what comment 16 is based on.  I'll just reopen this bug instead of filing a new one.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
reopening bugs that landed any patch is a nightmare for release engineering and tracking uplifts, please move to a new bug, seriously.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Assignee

Updated

4 years ago
Depends on: 1207375
You need to log in before you can comment on or make changes to this bug.