urlbar a11y still broken by search suggestions opt-in prompt/notification

RESOLVED FIXED in Firefox 44

Status

()

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

People

(Reporter: adw, Assigned: adw)

Tracking

Trunk
Firefox 44
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox43 unaffected, firefox44+ fixed)

Details

(Whiteboard: [suggestions][fxsearch])

Attachments

(1 attachment)

The patch to bug 1198683 fixed the case where the opt-in prompt is shown, but it actually broke the case where the prompt is not shown.

I don't know why but when I remove search-suggestions-notification from aria-owns, both cases work.

James, could you please verify that this build works for you, once it becomes available?  Thanks.

https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/dwillcoxon@mozilla.com-8ca67bed7b07
Flags: firefox-backlog+
> The patch to bug 1198683 fixed the case where the opt-in prompt is shown,
> but it actually broke the case where the prompt is not shown.
> 
> I don't know why but when I remove search-suggestions-notification from
> aria-owns, both cases work.
> 
> James, could you please verify that this build works for you, once it
> becomes available?  Thanks.
> 
> https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/
> dwillcoxon@mozilla.com-8ca67bed7b07
Attachment #8664526 - Flags: review?(mak77)
Attachment #8664526 - Flags: feedback?(jamie)
Yup. Much better; works very nicely. Thanks!
Attachment #8664526 - Flags: feedback?(jamie) → feedback+
This probably only surfaced after bug 1133213 landed, which actually introduces aria-owns support in that it rearranges the accessible tree. So before bug 1133213, this was actually not broken I think. :)
[Tracking Requested - why for this release]:
The behavior introduced in bug 1198683 depends on bug 1133213.
Bug 1133213 was backed out of Aurora (43) because of crash fall-outs. However, the current plan is to re-land that bug on Aurora as soon as these crashes are fixed. When that happens, we will need to up-lift this to Aurora to avoid breaking users.
Depends on: 1133213
[Tracking Requested - why for this release]: comment 4 points out we will need this fix on Aurora.
ehr, my fault, Aurora IS 43.
Comment on attachment 8664526 [details] [diff] [review]
Remove search-suggestions-notification from aria-owns

Review of attachment 8664526 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/urlbarBindings.xml
@@ +1133,5 @@
>  
>    <binding id="urlbar-rich-result-popup" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-rich-result-popup">
>  
>      <content ignorekeys="true" level="top" consumeoutsideclicks="never"
> +             aria-owns="richlistbox">

I need some more info, just to be sure and for future reference:

1. is aria-owns="richlistbox" still needed? if aria-owns defines a parent-child relationsip, I'm not sure why it is needed. Here "richlistbox" is a direct child, as well as "search-suggestions-notification", the DOM should be enough. Could be bug 1133213 fixed more than what indicates its title? I'd like some confirmation about the behavior with and without aria-owns.

2. Is the opt-in notification still accessible when shown? Cause we originally added both ids to aria-owns to fix accessibility and now we remove it. Did Jamie test all the cases: notification shown, notification dismissed with true, dismissed with false?
Rank: 5
(In reply to Marco Bonardo [::mak] from comment #7)
> 1. is aria-owns="richlistbox" still needed?

Yes, without it items are read as "unknown" when the opt-in is visible.

> 2. Is the opt-in notification still accessible when shown?

I tested all the cases, but I'll needinfo James about it.
(In reply to Marco Bonardo [::mak] from comment #7)
> 2. Is the opt-in notification still accessible when shown? Cause we
> originally added both ids to aria-owns to fix accessibility and now we
> remove it. Did Jamie test all the cases: notification shown, notification
> dismissed with true, dismissed with false?
Flags: needinfo?(jamie)
Comment on attachment 8664526 [details] [diff] [review]
Remove search-suggestions-notification from aria-owns

Review of attachment 8664526 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, provided Jamie confirms we are still good when the notification is shown and dismissed (with either true or false).
Attachment #8664526 - Flags: review?(mak77) → review+
adw: When are you planning to land this?
This is what Marco said:

(In reply to Marco Bonardo [::mak] from comment #10)
> r=me, provided Jamie confirms we are still good when the notification is
> shown and dismissed (with either true or false).

So I've been waiting on Jamie to confirm, but I was planning on waiting only a week or so before just landing.  I don't agree with having Jamie double-confirm, but I'm doing so because Marco asked.
(In reply to Marco Bonardo [::mak] from comment #10)
> r=me, provided Jamie confirms we are still good when the notification is
> shown and dismissed (with either true or false).
Confirmed; all works as expected. Thanks.

I have a feeling the alert event sometimes isn't fired, but it's fairly rare and that'd be a different bug anyway even if I'm correct.
Flags: needinfo?(jamie)
https://hg.mozilla.org/mozilla-central/rev/f5f5b6214fb4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Do you want to uplift this to aurora? Or are you looking to verify the fix on nightly first?
Flags: needinfo?(adw)
we cannot uplift this until bug 1133213 has been uplifted.
Flags: needinfo?(adw)
Bug 1133213 has been wontfixed for 43, so this bug no longer needs to be uplifted.  Since this bug was caused by bug 1133213, this bug is not actually present on 43, which I verified by manual testing.
You need to log in before you can comment on or make changes to this bug.