Remove usage of document.commandDispatcher in breadcrumbs.js

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Developer Tools: Inspector
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

Trunk
Firefox 51
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [devtools-html])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Breadcrumbs are still relying on the fact that the parent document is a XUL document, and use the CommandDispatcher interface.

>   let cmdDispatcher = this.chromeDoc.commandDispatcher;
>   this.hadFocus = (cmdDispatcher.focusedElement &&
>                    cmdDispatcher.focusedElement.parentNode == this.container);

This code should be removed in favor of document.activeElement.

Moreover, chromeDoc should be renamed to no longer hint at a chrome specific document.
Flags: qe-verify-
Whiteboard: [devtools-html] [triage]
Comment hidden (mozreview-request)
Pbro: correct me if I'm wrong, but from what I tested and what I see in handlerFocus(), it looks like the focus can no longer reach the inner buttons, and this was therefore just dead code.

> /**
>  * Focus event handler. When breadcrumbs container gets focus,
>  * aria-activedescendant needs to be updated to currently selected
>  * breadcrumb. Ensures that the focus stays on the container at all times.
>  * @param {DOMEvent} event.
>  */
Assignee: nobody → jdescottes
Blocks: 1263750
Status: NEW → ASSIGNED
Iteration: --- → 51.2 - Aug 29
Priority: -- → P1
Whiteboard: [devtools-html] [triage] → [devtools-html]
It looks like the focus behavior was implemented in Bug 1272011, which confirms only the container will ever be focused here.
See Also: → bug 1272011
(In reply to Julian Descottes [:jdescottes] from comment #2)
> Pbro: correct me if I'm wrong, but from what I tested and what I see in
> handlerFocus(), it looks like the focus can no longer reach the inner
> buttons, and this was therefore just dead code.
You're right, focus always stays at the "container" level now, we just use aria-activedescendant to tell assistive technology which button is active, but we don't want to have people tab through many buttons to navigate the UI.
I tested quickly, and didn't find any way to get into this branch of the code.
Good catch on cleaning up this dead code.

Comment 5

a year ago
mozreview-review
Comment on attachment 8784394 [details]
Bug 1297713 - remove calls to document.commanddispatcher in breadcrumbs.js;

https://reviewboard.mozilla.org/r/73856/#review71942
Attachment #8784394 - Flags: review?(pbrosset) → review+
Thanks for the review, try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=561e51191017

Comment 7

a year ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bbc3d258eb35
remove calls to document.commanddispatcher in breadcrumbs.js;r=pbro

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bbc3d258eb35
Status: ASSIGNED → RESOLVED
Last Resolved: a year 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.