Patrick Brosset <:pbro> Reporter
	Description • 8 years ago

In the scope of the devtools.html (track 3) project, we need to get rid of the FocusManager.

See a list of usages in devtools/client:
https://dxr.mozilla.org/mozilla-central/search?q=focusmanager+path%3Adevtools%2Fclient&redirect=true

	Brian Grinstead [:bgrins]
	Comment 1 • 8 years ago

Scanning through the list, the only two cases that look relevant to the toolbox are in inplace-editor and the breadcrumbs

	Brian Grinstead [:bgrins]
	Comment 2 • 8 years ago

There are also some constants used in markup and rule view that need to be shimmed like Ci.nsIFocusManager.MOVEFOCUS_FORWARD and Ci.nsIFocusManager.MOVEFOCUS_BACKWARD

	Tom Tromey :tromey Assignee
	Comment 3 • 8 years ago

I've been researching this a bit and thought I'd write up my findings.

The constants are simple to replace.
The difficult bit seems to be the calls to moveFocus.

In inplace-editor we can almost rely on the built-in handling of tab and not
call preventDefault on the event.  However, only "almost", because this
widget also shifts focus on enter.

toolbox.js allows arrow keys as well as tab to shift the focus.

breadcrumbs.js could almost do the not-call-preventDefault thing; except that
it does most of the event-handling work in a promise callback.

I wrote a test program to synthesize a Tab keypress in the hopes that this would
cause focus switching, but I couldn't get this to work.  I may try debugging
the event handling code to see what is going on.

Given all this I think perhaps the only solution is to reimplement focus
handling ourselves, by searching through the DOM for the correct element to
focus on.  This seems difficult given the algorithm in question.

	Tom Tromey :tromey Assignee
	Comment 4 • 8 years ago

I filed bug 1289170 for some existing oddities.

I also searched through devtools and the only uses of tabindex > 0 were in webconsole.xul.
I think this greatly simplifies the task of writing our own focus handling functions.

	Tom Tromey :tromey Assignee
	Comment 5 • 8 years ago

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

	Tom Tromey :tromey Assignee
	Comment 6 • 8 years ago

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

	Greg Tatum [:gregtatum]
	Comment 7 • 8 years ago

Comment on attachment 8774745 [details]
Bug 1278473 - write shim Services.focus;

https://reviewboard.mozilla.org/r/67194/#review64462

LGTM!

::: devtools/client/shared/shim/Services.js:563
(Diff revision 1)
> +          if (tabIndex === "-1") {
> +            return NodeFilter.FILTER_SKIP;
> +          }
> +          node.focus();
> +          if (document.activeElement == node) {
> +            return NodeFilter.FILTER_ACCEPT;

Wow.

	Greg Tatum [:gregtatum]
	Comment 8 • 8 years ago

Comment on attachment 8774744 [details]
Bug 1278473 - use Services.focus, not nsIFocusManager, in devtools;

https://reviewboard.mozilla.org/r/67192/#review64480

LGTM!

	Pulsebot
	Comment 9 • 8 years ago

Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2e786301b6e8
use Services.focus, not nsIFocusManager, in devtools; r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/cd08b203bbae
write shim Services.focus; r=gregtatum

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2e786301b6e8
https://hg.mozilla.org/mozilla-central/rev/cd08b203bbae