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

RESOLVED FIXED in Firefox 51

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dao, Assigned: adw)

Tracking

({access, regression})

Trunk
Firefox 51
access, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 unaffected, firefox49 unaffected, firefox50 unaffected, firefox51 fixed)

Details

(Whiteboard: [fxsearch])

(Reporter)

Description

2 years ago
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.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(adw)

Comment 1

2 years ago
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dao+bmo)

Updated

2 years ago
Whiteboard: [fxsearch]
(Reporter)

Comment 2

2 years ago
(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.
Flags: needinfo?(dao+bmo)
Whiteboard: [fxsearch]
(Reporter)

Updated

2 years ago
Whiteboard: [fxsearch]
(Assignee)

Updated

2 years ago
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
See Also: → bug 1297976
(Reporter)

Updated

2 years ago
Blocks: 1297976
status-firefox48: --- → unaffected
status-firefox49: --- → unaffected
status-firefox50: --- → unaffected
(Assignee)

Comment 3

2 years ago
The patch in bug 1297976 fixes this.
(Assignee)

Comment 4

2 years ago
Fixed in bug 1297976.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.