1297713 - Remove usage of document.commandDispatcher in breadcrumbs.js

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Julian Descottes [:jdescottes]]

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.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1297713 - remove calls to document.commanddispatcher in breadcrumbs.js;

Review commit: https://reviewboard.mozilla.org/r/73856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73856/

Comment 2

[Julian Descottes [:jdescottes]]

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.
>  */

Comment 3

[Julian Descottes [:jdescottes]]

It looks like the focus behavior was implemented in Bug 1272011, which confirms only the container will ever be focused here.

Comment 4

[Patrick Brosset <:pbro>]

(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

[Patrick Brosset <:pbro>]

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

https://reviewboard.mozilla.org/r/73856/#review71942

Comment 6

[Julian Descottes [:jdescottes]]

Thanks for the review, try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=561e51191017

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bbc3d258eb35
remove calls to document.commanddispatcher in breadcrumbs.js;r=pbro

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bbc3d258eb35