1300053 - One-off search engine buttons and the search settings button have duplicate ids

Status: RESOLVED FIXED

(Firefox :: Search, defect, P2)

Description

[Dão Gottwald [:dao]]

The id attributes were unfortunately added in bug 1107695, and then bug 1180944 (not unreasonably) reused the binding, so now we have duplicate ids. I imagine this breaks a11y.

By the way, the search settings button's id is "-anon-search-settings". It looks like there was supposed to be a prefix but it's an empty string.

Comment 1

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #0)
> The id attributes were unfortunately added in bug 1107695,

When focus stays in one place (the textbox), and simulated focus needs to move, to the best of my knowledge the only way to do that is using aria-owns and aria-activedescendant. These both use ids. I believe there is no other way to do this. So it is necessary for the elements to have an id.

(Arguably this is a flaw in the aria-activedescendant spec, because it causes problems not just for this particular binding but for any reusable component code, be it a web component, a react one, a jquery library, ... the generic problem occurs everywhere.)

> and then bug 1180944 (not unreasonably) reused the binding,

Well, a number of the bindings in urlbarbindings.xml, where the code was written, are unique, and explicitly rely on being bound to gURLBar, or invoke document.getElementById() on hardcoded strings. I don't think the assumption that the binding would not be reused was unreasonable at the time the code was written. :-\

> so now we have duplicate ids.
> I imagine this breaks a11y.

Maybe. I kind of expect we would have heard by now if it did - though maybe not, because the last time this broke we heard quite late as well. I think aria-owns might be saving us here in that focus can only move to descendants, or descendants of an aria-owns element.

Of course, we should still fix it either way.

> By the way, the search settings button's id is "-anon-search-settings". It
> looks like there was supposed to be a prefix but it's an empty string.

Yes, it uses `this.id` as a prefix. That was originally the search popup, but that got broken when the popup's XBL component got broken up into smaller component, and presumably now the bound element no longer has an id.


Was there a specific question you wanted to ask?

Comment 2

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #1)
> > so now we have duplicate ids.
> > I imagine this breaks a11y.
> 
> Maybe. I kind of expect we would have heard by now if it did - though maybe
> not, because the last time this broke we heard quite late as well. I think
> aria-owns might be saving us here in that focus can only move to
> descendants, or descendants of an aria-owns element.

Ids are supposed to be unique, so I totally expect a11y code to utilize some method based on the document's id table and use whatever element it gets from there. If it's smart enough to realize that the found element is not where it should be, that still wouldn't magically make it find the right one.

> Was there a specific question you wanted to ask?

No, I wanted you and adw to be aware of the bug.

Comment 3

[Drew Willcoxon :adw]

The patch in bug 1297976 fixes this.

Comment 4

[Drew Willcoxon :adw]

Fixed in bug 1297976.