Closed
Bug 1207375
Opened 9 years ago
Closed 9 years ago
urlbar a11y still broken by search suggestions opt-in prompt/notification
Categories
(Firefox :: Address Bar, defect, P1)
Firefox
Address Bar
Tracking
()
RESOLVED
FIXED
Firefox 44
Tracking | Status | |
---|---|---|
firefox43 | --- | unaffected |
firefox44 | + | fixed |
People
(Reporter: adw, Assigned: adw)
References
Details
(Whiteboard: [suggestions][fxsearch])
Attachments
(1 file)
1.21 KB,
patch
|
mak
:
review+
Jamie
:
feedback+
|
Details | Diff | Splinter Review |
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+
Assignee | ||
Comment 1•9 years ago
|
||
> 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)
Comment 2•9 years ago
|
||
Yup. Much better; works very nicely. Thanks!
Updated•9 years ago
|
Attachment #8664526 -
Flags: feedback?(jamie) → feedback+
Comment 3•9 years ago
|
||
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. :)
Comment 4•9 years ago
|
||
[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.
tracking-firefox43:
--- → ?
Depends on: 1133213
Comment 5•9 years ago
|
||
[Tracking Requested - why for this release]: comment 4 points out we will need this fix on Aurora.
tracking-firefox42:
--- → ?
Comment 6•9 years ago
|
||
ehr, my fault, Aurora IS 43.
status-firefox44:
affected → ---
tracking-firefox42:
? → ---
Updated•9 years ago
|
status-firefox42:
--- → unaffected
status-firefox43:
--- → affected
status-firefox44:
--- → affected
Comment 7•9 years ago
|
||
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?
Updated•9 years ago
|
Rank: 5
Assignee | ||
Comment 8•9 years ago
|
||
(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.
Assignee | ||
Comment 9•9 years ago
|
||
(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 10•9 years ago
|
||
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+
Updated•9 years ago
|
tracking-firefox44:
--- → +
Comment 11•9 years ago
|
||
adw: When are you planning to land this?
Assignee | ||
Comment 12•9 years ago
|
||
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.
Comment 13•9 years ago
|
||
(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)
Assignee | ||
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/f5f5b6214fb4
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f5f5b6214fb4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Comment 16•9 years ago
|
||
Do you want to uplift this to aurora? Or are you looking to verify the fix on nightly first?
Flags: needinfo?(adw)
Comment 17•9 years ago
|
||
we cannot uplift this until bug 1133213 has been uplifted.
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(adw)
Assignee | ||
Comment 18•9 years ago
|
||
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.
status-firefox42:
unaffected → ---
tracking-firefox43:
+ → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•