Closed Bug 1050439 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: Framework, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8469591 - Flags: feedback?(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
Attachment #8469591 - Flags: feedback?(jwalker) → feedback+
(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.
Attachment #8469591 - Attachment is obsolete: true
Attachment #8473428 - Flags: review?(jwalker)
Attachment #8473428 - Attachment is obsolete: true
Attachment #8473428 - Flags: review?(jwalker)
Attachment #8473430 - Flags: review?(jwalker)
Attachment #8473430 - Flags: review?(jwalker) → review+
https://hg.mozilla.org/mozilla-central/rev/b4ea3496213f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
See Also: → 957589
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.