1198683 - Search suggestions prompt breaks accessibility of location bar suggestions

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[James Teh [:Jamie]]

(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.

Comment 1

[Dave Townsend [:mossop]]

Marco, can you help us understand how much of a problem this is?

Comment 2

[James Teh [:Jamie]]

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.

Comment 3

[Dave Townsend [:mossop]]

That's all I needed to know, thanks

Comment 4

[Drew Willcoxon :adw]

Thanks for filing, James.  Making this block the bug where the opt-in was implemented since that's what caused it.

Comment 5

[Drew Willcoxon :adw]

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.

Comment 6

[James Teh [:Jamie]]

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?

Comment 7

[Marco Bonardo [:mak]]

(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.

Comment 8

[James Teh [:Jamie]]

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.

Comment 9

[Drew Willcoxon :adw]

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.

Comment 10

[Drew Willcoxon :adw]

(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.

Comment 11

[Drew Willcoxon :adw]

Attachment: patch

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/[e-mail]-131f749d0fce

https://treeherder.mozilla.org/#/jobs?repo=try&revision=131f749d0fce

Comment 12

[Marco Bonardo [:mak]]

(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.

Comment 13

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/e831db739c39

Comment 14

[Phil Ringnalda (:philor)]

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

Comment 15

[James Teh [:Jamie]]

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.

Comment 16

[Drew Willcoxon :adw]

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.

Comment 17

[James Teh [:Jamie]]

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.

Comment 18

[Marco Bonardo [:mak]]

please file a separate bug for any remaining issues, we can't track closed bugs.

Comment 19

[Drew Willcoxon :adw]

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.

Comment 20

[Marco Bonardo [:mak]]

reopening bugs that landed any patch is a nightmare for release engineering and tracking uplifts, please move to a new bug, seriously.