Closed Bug 1278473 Opened 3 years ago Closed 3 years ago

Remove usage of Services.FocusManager in devtools/client

Categories

(DevTools :: Framework, enhancement, P1)

49 Branch
enhancement

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.4 - Aug 1
Tracking Status
firefox50 --- fixed

People

(Reporter: pbro, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(2 files)

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
Severity: normal → enhancement
No longer blocks: devtools-html-3
Scanning through the list, the only two cases that look relevant to the toolbox are in inplace-editor and the breadcrumbs
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
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html][triage] → [devtools-html]
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 50.4 - Aug 1
Priority: P2 → P1
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.
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.
Attachment #8774745 - Flags: review?(gtatum) → review+
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.
Comment on attachment 8774744 [details]
Bug 1278473 - use Services.focus, not nsIFocusManager, in devtools;

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

LGTM!
Attachment #8774744 - Flags: review?(gtatum) → review+
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
https://hg.mozilla.org/mozilla-central/rev/2e786301b6e8
https://hg.mozilla.org/mozilla-central/rev/cd08b203bbae
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.