Yet more barely accessible pop-ups... :( STR: 1. In about:config, ensure that browser.urlbar.userMadeSearchSuggestionsChoice is false. 2. Focus the location bar. 3. Type anything that will cause suggestions to appear. 4. Observe that the "Would you like to improve your search experience with suggestions?" prompt appears. Expected: An accessible alert event should be fired on an accessible (probably with role alert) with the label and buttons as its children. Ideally, the buttons should have accelerators or be focusable somehow. Actual: No alert event is fired. The label and buttons are siblings, but they are not inside a separate container accessible.

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

Marco Z., could you try this build please? https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/dwillcoxon@mozilla.com-b6b97bcb02f4 It adds role="alert" to the notification and accesskeys to the learn-more link and the Yes and No buttons. Making them tabbable is not easy because there's special logic that makes the tab key cycle through the popup's items. So I hope the accesskeys are enough. If not we'll have to rework that logic. By the way, is this another case where VoiceOver support is broken? I had to use NVDA on Windows again for this. Marco B., making the link's accesskey actually work was a little tricky. It has to have a click handler, not an href, and it has to have a control attribute that has a valid ID. See nsXULElement::PerformAccesskey for why: http://mxr.mozilla.org/mozilla-central/source/dom/xul/nsXULElement.cpp#629 https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6b97bcb02f4

(In reply to Drew Willcoxon :adw from comment #2) > By the way, is this another case where VoiceOver support is broken? I had > to use NVDA on Windows again for this. Not that that's a bad thing, James, it's just that my development machine is a Mac. :-)

Comment on attachment 8655114 [details] [diff] [review] patch MarcoZ asked me to move this review to Jamie.

Comment on attachment 8655114 [details] [diff] [review] patch Happy to provide feedback, but I'm not comfortable with review, as I'm not familiar with some of the XUL stuff happening here. This is a fantastic start. A few things: 1. For some reason, the alert event is only fired the first time this alert appears. That is, if you dismiss the autocomplete and then bring it up again later, the notification appears, but no alert event gets fired. I'm guessing this has something to do with the way it is hidden and then shown again. MarcoZ, any ideas? 2. It'd be good if we could make that Learn more have role="link". Right now, it gets an accessible role of label, which is kinda odd. 3. I think it's okay that the Yes and No buttons aren't tabbable, given that they have access keys. 4. However, if at all possible, the Yes and No buttons need to be focusable, even if not tabbable; e.g. tabindex="-1" in HTML. The reason is that NVDA (and maybe other screen readers) only automatically reads controls inside alerts if they are focusable, the presumption being that normally, something that is focusable is something the user might want to interact with. Arguably, we should consider changing this presumption in NVDA, but it's worked for all alerts up until now.

The accesskeys in the patch may be problematic for an uplift :( > 4. However, if at all possible, the Yes and No buttons need to be focusable, > even if not tabbable; e.g. tabindex="-1" in HTML. The reason is that NVDA > (and maybe other screen readers) only automatically reads controls inside > alerts if they are focusable, the presumption being that normally, something > that is focusable is something the user might want to interact with. Is this a blocker for you? I'm not sure it's a good idea to add more focus variables to the plan, consider here focus is still in the autocomplete textbox, and the first listbox element is hilighted by DOMMenuItemActive. We are not moving focus to the notification, indeed we are basically forcing the user to make a choice with the mouse and the patch is adding accesskeys. Honestly I think if this is unacceptable we should go back to the design part and completely change the notification interaction. This would be expensive.

Comment on attachment 8655114 [details] [diff] [review] patch Review of attachment 8655114 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/urlbarBindings.xml @@ +1506,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"> > + <xul:hbox anonid="search-suggestions-notification" align="center" nit: move align to its own line @@ +1521,5 @@ > class="text-link" > + value="&urlbar.searchSuggestionsNotification.learnMore;" > + accesskey="&urlbar.searchSuggestionsNotification.learnMore.accesskey;" > + onclick="document.getBindingParent(this).openSearchSuggestionsNotificationLearnMoreURL();" > + control="search-suggestions-notification-learn-more" associating the label to itself sounds like a grey area. I'm not an expert on these attributes, I hope someone can bring expertise. why is this needed? would adding the "link" role allow to remove this? @@ +1522,5 @@ > + value="&urlbar.searchSuggestionsNotification.learnMore;" > + accesskey="&urlbar.searchSuggestionsNotification.learnMore.accesskey;" > + onclick="document.getBindingParent(this).openSearchSuggestionsNotificationLearnMoreURL();" > + control="search-suggestions-notification-learn-more" > + id="search-suggestions-notification-learn-more"/> do we need both an id and an anonid? Can we remove the anonid?

(In reply to Marco Bonardo [::mak] from comment #6) > > 4. However, if at all possible, the Yes and No buttons need to be focusable, > > even if not tabbable > Is this a blocker for you? At this stage, yes. The practical impact is that the user won't know about the Yes and No buttons at all if they aren't focusable. Since that's the only way the user can dismiss the notification, that's pretty serious. We can argue about whether NVDA should be doing the focusable test and perhaps that change should be made in the future, but at the end of the day, this notification is the exception, not the rule.

(In reply to James Teh [:Jamie] from comment #5) > 1. For some reason, the alert event is only fired the first time this alert > appears. That is, if you dismiss the autocomplete and then bring it up again > later, the notification appears, but no alert event gets fired. I'm guessing > this has something to do with the way it is hidden and then shown again. RootAccessible already listens for popupshown events. Could we hook into that? I spent most of the day trying to get that to work, with the idea that whatever a11y events need to be fired could be fired again every time the urlbar popup opens. The problem is that the Accessible here is the document: http://mxr.mozilla.org/mozilla-central/source/accessible/generic/RootAccessible.cpp#288 So RootAccessible::HandlePopupShownEvent doesn't realize that the the event is related to a popup. > 2. It'd be good if we could make that Learn more have role="link". Right > now, it gets an accessible role of label, which is kinda odd. That's strange, I see "Learn more... link Alt+I" in the NVDA Speech Viewer.

(In reply to Marco Bonardo [::mak] from comment #6) > The accesskeys in the patch may be problematic for an uplift :( I was thinking we could hardcode something like "1" and "2" for uplift. Haven't tried that yet though, so don't know whether it works.

(In reply to Drew Willcoxon :adw from comment #9) > > 2. It'd be good if we could make that Learn more have role="link". Right > > now, it gets an accessible role of label, which is kinda odd. > That's strange, I see "Learn more... link Alt+I" in the NVDA Speech Viewer. Hmm. That *is* strange. I'm definitely seeing a label and no link. This is with the try build from comment 2.

Also, I'm seeing the buttons even when they aren't focusable. "No button Alt+n". But only the first time the popup opens.

All of this is with the patch applied. The try link doesn't have any of it at all.

Er, sorry, the try build does have the patch applied.

I'm very concerned that we're seeing such different results. Those buttons definitely don't have the focusable state for me, so NVDA doesn't read them. So, for me: 1. I start the try build from comment 2. 2. I hit alt+d to focus the Location bar. 3. I type "z". 4. I hear "alert Would you like to improve your search experience with suggestions?, Learn more label Alt+l" and nothing else.

OK, the difference is that in my development build I've commented out this: -moz-user-focus: ignore; which applies to the buttons. No need for that to be there. And then the speech viewer shows me the buttons like I quoted in comment 12. So I think the only remaining problem is: (In reply to James Teh [:Jamie] from comment #5) > 1. For some reason, the alert event is only fired the first time this alert > appears. That is, if you dismiss the autocomplete and then bring it up again > later, the notification appears, but no alert event gets fired. I'm guessing > this has something to do with the way it is hidden and then shown again. As I mentioned, today I've been trying to figure out why that happens. Do you have any thoughts about the popupshown idea?

(In reply to Drew Willcoxon :adw from comment #10) > I was thinking we could hardcode something like "1" and "2" for uplift. > Haven't tried that yet though, so don't know whether it works. That does work, but then the buttons look like "Yes (1)", "No (2)" on Windows (and I think Linux too). As an even worse hack maybe we could hardcode accesskeys for like our top five languages and then use 1 and 2 for everything else.

(In reply to Drew Willcoxon :adw from comment #16) > So I think the only remaining problem is: What about the Learn more link being reported as a label? Did you make any local change that might make that correctly be treated as a link? > > 1. For some reason, the alert event is only fired the first time this alert > > appears. > As I mentioned, today I've been trying to figure out why that happens. Do > you have any thoughts about the popupshown idea? Sorry; I don't know much of the core a11y code. MarcoZ should hopefully be able to tell you/point you to someone that can.

Yes, I added role="link" to the label as you suggested, and now it shows up as a link. Sorry for the confusion!

One thing I'm quite confused about. Which is the document: the notification or the list? And how do they relate to each other? Are they siblings in the DOM? They look like siblings, but that suggests there has to be a containing element.

The notification (a xul:hbox) and the listbox (a xul:richlistbox) are siblings in the urlbar-rich-result-popup XBL binding. They're both "anonymous content" in the XUL document. So the DOM contains a xul:panel that is the autocomplete popup and is not anonymous, and then within that is the popup's anonymous content, of which the notification and listbox are top-level children and siblings. Are you familiar with XBL and XUL? It's kind of like web components. You can see the popup's XBL binding and its anonymous content here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#1510 That binding (urlbar-rich-result-popup) is bound to the #PopupAutoCompleteRichResult in the DOM here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#476 And then finally the #PopupAutoCompleteRichResult is defined in the browser's XUL markup here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#150 I'm not sure what you mean about which is the document though.

This is the best I've been able to do. It doesn't work perfectly. Sometimes the alert doesn't appear at all, and sometimes it appears twice, once with the buttons and once without the buttons. The patch fires an AlertActive every time the popup opens and the notification is present, which is what XUL notifications do. That event reaches the accessibility code 100% of the time, and the accessibility code fires nsIAccessibleEvent::EVENT_ALERT in response, but NVDA doesn't read the alert 100% of the time. https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/dwillcoxon@mozilla.com-ba81192c5880 https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba81192c5880

(In reply to James Teh [:Jamie] from comment #18) > (In reply to Drew Willcoxon :adw from comment #16) > > > 1. For some reason, the alert event is only fired the first time this alert > > > appears. > > As I mentioned, today I've been trying to figure out why that happens. Do > > you have any thoughts about the popupshown idea? > Sorry; I don't know much of the core a11y code. MarcoZ should hopefully be > able to tell you/point you to someone that can. With comment #22 taken into account here, I am not sure what else to suggest. It appears we're firing the event, but NVDA doesn't react to it always. Jamie, are you guys filtering out Alerts that repeatedly get fired on the same object?

Comment on attachment 8657383 [details] [diff] [review] patch v2 a11y-r=me. I tested the latest try build and am finding that it speaks the alert reliably enough for me. The additional stuff you hear at the end is text from the auto-complete at the top that fills out in the text field. I'm giving this an a11y-r+.

I have reproduced this bug on Firefox nightly 43.0a1 according to (2015-8-26) It's verified on Latest Firefox Beta Build ID 20151123113812 User Agent Mozilla/5.0 (Windows NT 6.3; rv:43.0) Gecko/20100101 Firefox/43.0 Tested OS- 32bit windows8.1