1050439 - Focus should be restored to previously active element after split console is closed

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Brian Grinstead [:bgrins]]

If you have a text box focused, then ESC to open split console, then ESC to close split console then the text box should be refocused for convenience.  Same with the debugger/style editor source editor elements once Bug 1043154 lands.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: restore-split-console-focus-WIP.patch

Joe, what do you think about this approach?  The idea is that we want to restore the active element once the split console is closed (either via ESC or via clicking the split console button).  The way I'm doing it here is tracking the focus event when the split console is opened and storing a reference to a node only if it is not a child of the console panel itself.

I'm using document.activeElement to store the original element then using the target for the focus event for subsequent focused elements.  There may be some better way that I'm not thinking of to handle this.

There are probably cases where something funny could happen, like if I focus on a node in the markup view, then enter $0.remove() in the console and close split console.  In this case the focused element is not a child of the document anymore, but it doesn't throw an exception or anything, I think the call to .focus() just doesn't do anything.

Comment 2

[Joe Walker [:jwalker]]

Comment on attachment 8469591 [details] [diff] [review]
restore-split-console-focus-WIP.patch

Review of attachment 8469591 [details] [diff] [review]:
-----------------------------------------------------------------

What happens to the focus if we start with the focus in the style editor, press escape to open the split console, then select the debugger then press escape again to close the split console?

::: browser/devtools/framework/toolbox.js
@@ +1010,5 @@
> +    // If the console is split and we are focusing an element outside
> +    // of the console, then store the newly focused element, so that it
> +    // can be restored once the split console closes.
> +    // For instance -> open split console, select a text box, focus console
> +    // again, then close the console.  The text box should be refocused.

Maybe this should be a doc comment? /** ... */

@@ +1019,5 @@
> +          originalTarget.ownerDocument.defaultView;
> +
> +        // Only track elements if it doesn't belong to the web console
> +        if (webconsolePanel._frameWindow != activeElementWindow) {
> +          this._savedActiveElement = originalTarget;

Do we need to start this function with:

    this._savedActiveElement = null;

So we don't end up going back to where we were some time ago if we don't end up storing an _savedActiveElement this time?

@@ +1050,5 @@
>      Services.prefs.setBoolPref(SPLITCONSOLE_ENABLED_PREF, false);
>      this._refreshConsoleDisplay();
>      this.emit("split-console");
> +
> +    console.log("Console has been closed... restoring focus", this._savedActiveElement);

Can probably remove this

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Joe Walker [:jwalker] from comment #2)
> Comment on attachment 8469591 [details] [diff] [review]
> restore-split-console-focus-WIP.patch
> 
> Review of attachment 8469591 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What happens to the focus if we start with the focus in the style editor,
> press escape to open the split console, then select the debugger then press
> escape again to close the split console?

This is tricky to do without focusing something in the process, but I've been able to do it by using cmd+] to switch panels.  Nothing really happens.. it calls focus on the hidden element, which doesn't seem to do anything.

> 
> @@ +1019,5 @@
> > +          originalTarget.ownerDocument.defaultView;
> > +
> > +        // Only track elements if it doesn't belong to the web console
> > +        if (webconsolePanel._frameWindow != activeElementWindow) {
> > +          this._savedActiveElement = originalTarget;
> 
> Do we need to start this function with:
> 
>     this._savedActiveElement = null;
> 
> So we don't end up going back to where we were some time ago if we don't end
> up storing an _savedActiveElement this time?

Yes, good idea.

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: restore-split-console-focus.patch

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: restore-split-console-focus.patch

Comment 6

[Brian Grinstead [:bgrins]]

https://tbpl.mozilla.org/?tree=Try&rev=d477341bce2a

Comment 7

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/b4ea3496213f

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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