A screen reader does not receive any focus events when tabbing through the available one-off search buttons in the new Search UI. STR: 1. With NVDA running, go to the search field with Ctrl+K. 2. Enter any search term. 3. Press tab to select one of the available one-off buttons to do a search with. Expected: NVDA should speak the search engine name. Actual: Nothing gets spoken, and NVDA thinks focus is still on the Search field.

Talking to Marco and Florian, this should be fixable by adding/updating/removing the aria-activedescendant attribute on the focused element (the textbox) with the ID of the button which is "selected". See https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-activedescendant_attribute for more details.

I implemented the patch as described in comment 1 (and also following documentation from http://msdn.microsoft.com/en-us/library/ie/dd347027%28v=vs.85%29.aspx), and it seems to have worked well on VoiceOver on Mac. I sent it to tryserver to get Windows builds. Marco, could you try these builds to check if it also works properly for NVDA? Builds should appear at: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/felipc@gmail.com-777a526fb3e4

Hi Felipe, can you provide a point value.

Comment on attachment 8538941 [details] [diff] [review] Set aria-activedescendant on search engine buttons This appears to be correct, however when I tab through the items with NVDA, focus always lands on some "unknown" element instead of the button itself. Also, the buttons all appear to be the children of a label. That has also been the case before this patch, and it is kind of weird, since a label is not a container element. Not yet sure what is going on, why this isn't working as expected.

(In reply to Marco Zehe (:MarcoZ) from comment #4) > Also, the buttons all appear to be the children of a label. That has also > been the case before this patch, and it is kind of weird, since a label is > not a container element. Not yet sure what is going on, why this isn't > working as expected. The buttons are children of a <description> XUL element, used for the way it wraps its content automatically. Is there an aria attribute we can add on it so that it's seen as a container by accessibility tools?

(In reply to Florian Quèze [:florian] [:flo] from comment #5) > (In reply to Marco Zehe (:MarcoZ) from comment #4) > > > Also, the buttons all appear to be the children of a label. That has also > > been the case before this patch, and it is kind of weird, since a label is > > not a container element. Not yet sure what is going on, why this isn't > > working as expected. > > The buttons are children of a <description> XUL element, used for the way it > wraps its content automatically. Is there an aria attribute we can add on it > so that it's seen as a container by accessibility tools? Try setting role="presentation".

Yes, I would also go with role "presentation", since this seems to be a presentational element only. Also funny you can use xul:description at all in this fashion. My other question, as I thought about this some more, is: Are these buttons part of anonymous content? If so, the id reference may not be valid for the reference, and thus NVDA says"unknown". But try the role "presentation" thing first and we'll see if the buttons now start working.

(In reply to Marco Zehe (:MarcoZ) from comment #7) > Yes, I would also go with role "presentation", since this seems to be a > presentational element only. Also funny you can use xul:description at all > in this fashion. > > My other question, as I thought about this some more, is: Are these buttons > part of anonymous content? If so, the id reference may not be valid for the > reference, and thus NVDA says"unknown". They're added inside anon content, but the buttons themselves aren't anon content, I think. So I would have thought it should work. :-\

Just sent to try a version with role="presentation" on the description container. Builds will appear in: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/felipc@gmail.com-ff7e37102435

This new try build also doesn't improve the situation for me. I hear "List", followed by "unknown" when I enter something in the search field and hit tab. It looks like this also causes the list of autocompletes to be expanded, because when I check where my focus is, it is, or claims to be, a child of the list that is the auto complete tree. Siblings of that tree are the buttons, and according to my viewer, they have the correct IDs. But focus events are fired on some unknown accessible that claims to be a child of the list of autocompletes. But when I then look in the hierarchy, there is no unknown child. There is only another list that contains the column headers, and the items of autocomplete itself.

(In reply to Marco Zehe (:MarcoZ) from comment #11) > This new try build also doesn't improve the situation for me. I hear "List", > followed by "unknown" when I enter something in the search field and hit > tab. It looks like this also causes the list of autocompletes to be > expanded, because when I check where my focus is, it is, or claims to be, a > child of the list that is the auto complete tree. Siblings of that tree are > the buttons, and according to my viewer, they have the correct IDs. But > focus events are fired on some unknown accessible that claims to be a child > of the list of autocompletes. But when I then look in the hierarchy, there > is no unknown child. There is only another list that contains the column > headers, and the items of autocomplete itself. This sounds like the autocomplete a11y messes with our ability to indicate the "focused" button. I wonder if we could get away with changing the tooltiptext to use the one-off search button's label, so that the accessible name of the search input box changes. Don't know if that would always be updated in the screenreader, though, and it feels like a hack. :-( Marco, do you know how hard it would be to make activedescendant work correctly here? Or is that a question for surkov?

Unassigning myself for the moment because I don't know what is the next step here, and don't want to block anyone who could pick it up. But I'm happy to take the bug again to continue explore other options once we know what to try. I'm wondering if we should take the patch as posted here anyway as it made a positive difference on Mac, and seems to have had the intended effect (tabbing/moving through the engines list will say "Button, <engine name>" as if real focus had changed). I wonder if this is a bug in NVDA or in our a11y code that is not properly respecting activedescendant on Windows.

(In reply to Gijs Kruitbosch (Gone until Jan 5) from comment #12) > Marco, do you know how hard it would be to make activedescendant work > correctly here? Or is that a question for surkov? That is definitely a question for Surkov.

(In reply to Gijs Kruitbosch from comment #12) > Marco, do you know how hard it would be to make activedescendant work > correctly here? Or is that a question for surkov? to skip me on reading whole conversation, what's wrong with activedescendant? (current list of problems is https://bugzilla.mozilla.org/buglist.cgi?f1=OP&list_id=11809149&short_desc=activedescendant&f0=OP&resolution=---&f4=CP&query_format=advanced&j1=OR&f3=CP&short_desc_type=allwordssubstr&component=Disability%20Access%20APIs&product=Core)

(In reply to alexander :surkov from comment #15) > (In reply to Gijs Kruitbosch from comment #12) > > > Marco, do you know how hard it would be to make activedescendant work > > correctly here? Or is that a question for surkov? > > to skip me on reading whole conversation, what's wrong with > activedescendant? (current list of problems is > https://bugzilla.mozilla.org/buglist. > cgi?f1=OP&list_id=11809149&short_desc=activedescendant&f0=OP&resolution=--- > &f4=CP&query_format=advanced&j1=OR&f3=CP&short_desc_type=allwordssubstr&compo > nent=Disability%20Access%20APIs&product=Core) We're trying to use it to point to nodes in the popup containing the autocomplete results for the search field (while text focus stays in the search field). These items are not part of the autocomplete results (ie they're not text, they're additional buttons). It looks like whatever magic is making the autocomplete results (the text bits) accessible is interfering with activedescendant making the extra buttons accessible. Details of the observed behaviour on Windows (it seems to work on Mac!) are in comment #11 .

do you have code snippet/test case to demo the problem?

(In reply to alexander :surkov from comment #17) > do you have code snippet/test case to demo the problem? The best thing I can suggest is building Firefox with the patch on this bug applied, or using the try build from comment #9

ok, I'll put it into my queue if somebody won't forestall me explaining what's wrong with aria-activedescendant implementation

you know I don't see at all that aria-activedescendant attribute is changed, it'd be good to have some help from somebody who knows this code.

The buttons are anonymous content, right? You can't set an id on anonymous nodes and expect that to work as a way to reference these nodes from the outside. If you want to use aria-activedescendant here, you'll probably need to make the buttons non-anonymous.

(In reply to Dão Gottwald [:dao] from comment #21) > The buttons are anonymous content, right? No, http://hg.mozilla.org/mozilla-central/annotate/3d846527576f/browser/base/content/urlbarBindings.xml#l1111

(In reply to Florian Quèze [:florian] [:flo] from comment #22) > (In reply to Dão Gottwald [:dao] from comment #21) > > The buttons are anonymous content, right? > > No, > http://hg.mozilla.org/mozilla-central/annotate/3d846527576f/browser/base/ > content/urlbarBindings.xml#l1111 That line of code doesn't prove anything as far as I can tell.

(In reply to Dão Gottwald [:dao] from comment #21) > The buttons are anonymous content, right? You can't set an id on anonymous > nodes and expect that to work as a way to reference these nodes from the > outside. If you want to use aria-activedescendant here, you'll probably need > to make the buttons non-anonymous. The buttons are "real" content inserted into anon content. That does not make them anon content.

(In reply to :Gijs Kruitbosch from comment #24) > (In reply to Dão Gottwald [:dao] from comment #21) > > The buttons are anonymous content, right? You can't set an id on anonymous > > nodes and expect that to work as a way to reference these nodes from the > > outside. If you want to use aria-activedescendant here, you'll probably need > > to make the buttons non-anonymous. > > The buttons are "real" content createElementNS doesn't put the node in the document, so there's no notion of anonymous vs. non-anonymous... > inserted into anon content. That does not > make them anon content. It very much does. What makes you think otherwise? I just checked, DOM Inspected confirms these buttons are anonymous.

(In reply to Dão Gottwald [:dao] from comment #25) > (In reply to :Gijs Kruitbosch from comment #24) > > (In reply to Dão Gottwald [:dao] from comment #21) > > > The buttons are anonymous content, right? You can't set an id on anonymous > > > nodes and expect that to work as a way to reference these nodes from the > > > outside. If you want to use aria-activedescendant here, you'll probably need > > > to make the buttons non-anonymous. > > > > The buttons are "real" content > > createElementNS doesn't put the node in the document, so there's no notion > of anonymous vs. non-anonymous... > > > inserted into anon content. That does not > > make them anon content. > > It very much does. What makes you think otherwise? We insert non-anon content into anon containers all the time, e.g. the <tab>s in the tabbrowser's <tabs> are non-anon. It sounds like we need a <children includes="button"> inside the container if we want this to not be anon (assuming that is enough). I assumed that was there already. In any case... while you might be right about the anon content, that doesn't really explain why this works on mac (see comment 2) and not on Windows. I'm not aware of any material differences there in terms of XUL/XBL/anon content.

(In reply to :Gijs Kruitbosch from comment #26) > We insert non-anon content into anon containers all the time, e.g. the > <tab>s in the tabbrowser's <tabs> are non-anon. No, that <tabs> element is isn't anonymous. > It sounds like we need a > <children includes="button"> inside the container if we want this to not be > anon (assuming that is enough). yes

(In reply to Dão Gottwald [:dao] from comment #27) > (In reply to :Gijs Kruitbosch from comment #26) > > We insert non-anon content into anon containers all the time, e.g. the > > <tab>s in the tabbrowser's <tabs> are non-anon. > > No, that <tabs> element is isn't anonymous. ... but the container in which they actually end up (the box.box-inherit) is.

Sure. The point is that for the inserted node to be non-anonymous, the node you call appendChild on needs to be non-anonymous (i.e. the node the binding is attached to), and then you can use <children> to place the non-anonymous nodes where you want them in the anonymous tree.

Leaving a ping here and letting everyone know that I am ranking this as one of the top usability problems for people with assistive technologies in the Firefox front-end at the moment. Not knowing where one tabs to is just not good.

attachment 8539245 [details] [diff] [review] has bitrotted since bug 1124904 and bug 1102038 landed; but it should now be easier to rewrite.

So you also need aria-owns in order to get the aria-activedescendant thing to work. This only fixes the one-off buttons, we'd need similar work to make adding opensearch items and the settings button actually work - they'd need to come out of the binding and then be inserted correctly. Which is interesting because they have separate containers. Rather than the current class-based specific binding, we could probably get away with using a different tag than 'button' for the latter in order to be able to differentiate them in the includes lists for the respective <children> lists. Generally, though, does this look passable, Florian?

Try push for some builds: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bcc0df60bb5 Marco, can you let me know if this fixes the one-off search buttons for you?

Comment on attachment 8564312 [details] [diff] [review] make one-off buttons accessible, Review of attachment 8564312 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to :Gijs Kruitbosch from comment #32) > Generally, though, does this look passable, Florian? Yes, the direction looks reasonable here :). I had similar thoughts about the open search and settings items; now that they are keyboard accessible, they make this bug more difficult. ::: browser/base/content/urlbarBindings.xml @@ +1143,5 @@ > > // Finally, build the list of one-off buttons. > let list = document.getAnonymousElementByAttribute(this, "anonid", > "search-panel-one-offs") > + let existingButtons = this.querySelectorAll('.searchbar-engine-one-off-item'); Is there a reason why you used single quotes for the querySelector calls? @@ +1206,5 @@ > let dummyItems = enginesPerRow - (engines.length % enginesPerRow || enginesPerRow); > for (let i = 0; i < engines.length; ++i) { > let engine = engines[i]; > let button = document.createElementNS(kXULNS, "button"); > + button.id = "engine-one-off-" + engine.name.replace(/[^a-zA-Z]/g, "-"); I wonder how this regexp will work for engines with unicode names that don't contain any a-z characters. ::: browser/themes/linux/browser.css @@ +936,5 @@ > #search-container { > min-width: calc(54px + 11ch); > } > > +/* Search popup (see also: searchbar.css) */ What's the reason for the CSS move?

(In reply to Florian Quèze [:florian] [:flo] from comment #34) > Comment on attachment 8564312 [details] [diff] [review] > make one-off buttons accessible, > > Review of attachment 8564312 [details] [diff] [review]: > ----------------------------------------------------------------- > > (In reply to :Gijs Kruitbosch from comment #32) > > > Generally, though, does this look passable, Florian? > > Yes, the direction looks reasonable here :). > > I had similar thoughts about the open search and settings items; now that > they are keyboard accessible, they make this bug more difficult. Yups. > ::: browser/base/content/urlbarBindings.xml > @@ +1143,5 @@ > > > > // Finally, build the list of one-off buttons. > > let list = document.getAnonymousElementByAttribute(this, "anonid", > > "search-panel-one-offs") > > + let existingButtons = this.querySelectorAll('.searchbar-engine-one-off-item'); > > Is there a reason why you used single quotes for the querySelector calls? No, just my dev setup being all over the place for lack of internet access. > @@ +1206,5 @@ > > let dummyItems = enginesPerRow - (engines.length % enginesPerRow || enginesPerRow); > > for (let i = 0; i < engines.length; ++i) { > > let engine = engines[i]; > > let button = document.createElementNS(kXULNS, "button"); > > + button.id = "engine-one-off-" + engine.name.replace(/[^a-zA-Z]/g, "-"); > > I wonder how this regexp will work for engines with unicode names that don't > contain any a-z characters. Yeah. I copied this from Felipe's patch, I'm not actually entirely sure if we even really need to do this... we're just assigning to a property, are there things that are actually *illegal* ? Maybe we should filter for those instead, if anything. > ::: browser/themes/linux/browser.css > @@ +936,5 @@ > > #search-container { > > min-width: calc(54px + 11ch); > > } > > > > +/* Search popup (see also: searchbar.css) */ > > What's the reason for the CSS move? The elems are non-anon now and therefore don't get the searchbar.css applied because it's included in the XBL only. (this actually also changes precedence things because XBL stylesheets are special... we should doublecheck that it does actually work reliably after the move, though I didn't see anything) The other sad thing is that this is pretty invasive and probably shouldn't be uplifted to 36... 37... well, if I get this done quickly, I guess it has a full beta cycle. I'd defer to your opinion, I guess. But it's a little annoying how much stuff this touches. I wonder if with the aria-owns change we could get away with not moving this to non-anon content... it'd simplify things a lot. :-\

(In reply to :Gijs Kruitbosch from comment #35) > > > +/* Search popup (see also: searchbar.css) */ > > > > What's the reason for the CSS move? > > The elems are non-anon now and therefore don't get the searchbar.css applied > because it's included in the XBL only. Ah, makes sense. Is this a good opportunity to move some of this CSS to a file in the shared folder? > The other sad thing is that this is pretty invasive and probably shouldn't > be uplifted to 36... 37... well, if I get this done quickly, I guess it has > a full beta cycle. I'd defer to your opinion, I guess. It's clearly too late for 36, and you would need a significantly different patch for 36 anyway as the keyboard navigation changes didn't land there. They actually haven't landed on 37 yet, but I totally expect them to be uplifted to 37 next week. If we get this finished in the next 2-3 weeks, I think it would be reasonable to uplift to 37/beta.

Yes, this definitely goes in the right direction! One thing: The parent of the buttons is an "unknown" accessible object. "unknown" means that there is no appropriate role assigned to it. The ID of the element in question is "PopupSearchAutoComplete". I suggest to give this element a role of "group" instead. Having a role of "unknown" in there is just not nice. Also, the labels for "search <search term with@ would be cool if thez were spoken there, but the popup is also the parent of the autocomplete list, so the announcement could get a bit confusing if these just became the label&accessible name for that group. Right now, all one hears when tabbing are the names of the engines. But let|s continue on that path and see how nice we can get it! >(

So it turns out the anon content stuff is a red herring, and marking the popup as aria-owned by the textbox is both necessary and sufficient to make this work on Windows. Tested and refined this a bit with Marco already, and also made the 'add search engine' button and 'change search settings' buttons work. How does this look?

Comment on attachment 8565034 [details] [diff] [review] make one-off buttons accessible, Review of attachment 8565034 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to :Gijs Kruitbosch from comment #38) > So it turns out the anon content stuff is a red herring, and marking the > popup as aria-owned by the textbox is both necessary and sufficient to make > this work on Windows. Excellent news! :-) ::: browser/base/content/urlbarBindings.xml @@ +1132,4 @@ > let addEngines = gBrowser.selectedBrowser.engines; > if (addEngines && addEngines.length > 0) { > const kBundleURI = "chrome://browser/locale/search.properties"; > let bundle = Services.strings.createBundle(kBundleURI); Can this also be cleaned up? @@ +1254,5 @@ > list.appendChild(button); > } > + > + let groupText = this.bundle.formatStringFromName("searchForWith", [textbox.value], 1); > + list.setAttribute("aria-label", groupText); Why is this needed? It seems to me this is duplicated from the inputHandler function (which is called once synchronously). ::: browser/locales/en-US/chrome/browser/search.properties @@ +17,5 @@ > +# for the one-off search buttons. %1$S is the search term, and the implication > +# is that the one-off button labels (ie the search engine names) come at the > +# end of the string. This should match searchFor.label and searchWith.label in > +# browser.dtd > +searchForWith=Search for %1$S with: Should this string have quotes or something to separate the keyword from the wrapping constant message? That's something we do using colors (gray vs black) in the visible UI. Is there a way to make a version of this patch that doesn't require a new string for 37? (or are we giving up on fixing this for 37?)

Comment on attachment 8565034 [details] [diff] [review] make one-off buttons accessible, Went ahead and ran this on a fresh local build I made. Yep, this is what I had in mind! Thank you!

(In reply to Florian Quèze [:florian] [:flo] from comment #39) > Comment on attachment 8565034 [details] [diff] [review] > make one-off buttons accessible, > > Review of attachment 8565034 [details] [diff] [review]: > ----------------------------------------------------------------- > > (In reply to :Gijs Kruitbosch from comment #38) > > > So it turns out the anon content stuff is a red herring, and marking the > > popup as aria-owned by the textbox is both necessary and sufficient to make > > this work on Windows. > > Excellent news! :-) > > ::: browser/base/content/urlbarBindings.xml > @@ +1132,4 @@ > > let addEngines = gBrowser.selectedBrowser.engines; > > if (addEngines && addEngines.length > 0) { > > const kBundleURI = "chrome://browser/locale/search.properties"; > > let bundle = Services.strings.createBundle(kBundleURI); > > Can this also be cleaned up? Probably, will do. > @@ +1254,5 @@ > > list.appendChild(button); > > } > > + > > + let groupText = this.bundle.formatStringFromName("searchForWith", [textbox.value], 1); > > + list.setAttribute("aria-label", groupText); > > Why is this needed? It seems to me this is duplicated from the inputHandler > function (which is called once synchronously). Are you sure I had issues with that for the original tooltiptext solution I used here (I iterated with Marco for a bit)... I'll have another look. Might be later this week because I need to focus on another issue first... > ::: browser/locales/en-US/chrome/browser/search.properties > @@ +17,5 @@ > > +# for the one-off search buttons. %1$S is the search term, and the implication > > +# is that the one-off button labels (ie the search engine names) come at the > > +# end of the string. This should match searchFor.label and searchWith.label in > > +# browser.dtd > > +searchForWith=Search for %1$S with: > > Should this string have quotes or something to separate the keyword from the > wrapping constant message? That's something we do using colors (gray vs > black) in the visible UI. An interesting suggestion. I guess so, yeah. > > Is there a way to make a version of this patch that doesn't require a new > string for 37? (or are we giving up on fixing this for 37?) Hm, well, we could compose the message using the existing elements that have the searchFor and searchWith labels, I guess. Kind of hacky, but it should work.

(In reply to :Gijs Kruitbosch from comment #41) > Are you sure insert question mark here.... :-\ > I had issues with that for the original tooltiptext solution I > used here (I iterated with Marco for a bit)...

(In reply to :Gijs Kruitbosch from comment #42) > (In reply to :Gijs Kruitbosch from comment #41) > > Are you sure > > insert question mark here.... :-\ https://hg.mozilla.org/releases/mozilla-beta/annotate/af24cff80f2d/browser/base/content/urlbarBindings.xml#l1075

This reuses existing strings, adds quotes (which I don't think need to be localizable?) and otherwise should address your nits. We no longer really need the .bundle refactoring, though I left it in for now as it is slightly cleaner, IMO. As you like.

Comment on attachment 8567125 [details] [diff] [review] make one-off buttons accessible, Review of attachment 8567125 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: browser/base/content/urlbarBindings.xml @@ +1110,4 @@ > if (textbox.value) { > self.removeAttribute("showonlysettings"); > + groupText = headerSearchText.previousSibling.value + > + '"' + headerSearchText.value + '"' + If this string was displayed, these quotes would have to be localized. I don't know if localized quotes are of any use for screen readers; I'm tempted to assume this is fine for at least 38/37, and if it turns out screen readers do care about which quotes are used, we can improve this in 39 where we aren't string frozen. @@ +1115,4 @@ > headerPanel.selectedIndex = 1; > } > else { > + groupText = noSearchHeader.value; noSearchHeader is only used here, so I would prefer if you moved the "let noSearchHeader = " line to inside this else block.

remote: https://hg.mozilla.org/integration/fx-team/rev/42269ccaf982

I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/eb6ee2f081b1 for browser-chrome-1 failures: https://treeherder.mozilla.org/logviewer.html#?job_id=2077808&repo=fx-team

Relanded with the dumb copy/paste mistake fixed (s/this/self/). This fixed the issues with the tests in my local testing. remote: https://hg.mozilla.org/integration/fx-team/rev/ddab351e166d

Marco's verified this.

Comment on attachment 8567125 [details] [diff] [review] make one-off buttons accessible, Approval Request Comment [Feature/regressing bug #]: Firefox 34 search UI [User impact if declined]: screenreader users can't use the one-off search buttons, or add search engines, or access the search settings from the search bar popup [Describe test coverage new/current, TreeHerder]: nope... [Risks and why]: very limited, it's super-early-beta and all this stuff landed much later ;-) (more seriously: it really is very limited risk... we're adding attributes specifically for a11y, and visually nothing changes) [String/UUID change made/needed]: nope!

Comment on attachment 8567125 [details] [diff] [review] make one-off buttons accessible, Thanks for verifying, approving uplift.