818442 - [toolbox] Re-focus devtools window after locking the highlighter

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

STR:
- Open the DevTools and undock into a separate window.
- Start the inspector
- Select and element on the current page.

Expected:
Devtools window is raised and focused so you can start debugging the selection.

Actual:
Browser window remains focused.

Original Github bug:
https://github.com/joewalker/devtools-window/issues/239

Comment 1

[Paul Rouget [:paul]]

Do we really want to do that?

That would mean that every time we click on a node, we would lose the focus from the browser window and the devtools window would be raised, but you'll still have to move your mouse to reach the devtools window.

I you just want to inspect a node (get information from the inspector or from the infobar), this could be very very annoying.

If you want to edit the style or the markup, you'll will have to reach the window anyway.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I agree with paul.

Harth, this was your bug ... what do you think?

Comment 3

[Paul Rouget [:paul]]

WONTFIX. If anyone disagree, please re-open.

Comment 4

[Heather Arthur [:harth]]

I still want this.

The problem is that the devtools window gets lost behind the other windows. If you're using the inspector, most likely you'll want to see the styles or navigate further with the breadcrumbs after you've selected a node (at least that's what it's like for me). Right now I have to spend a distracting several seconds finding that window after I've selected my node.

Also, try this:

1) Open tools
2) Select Inspector tool
3) Undock
4) Close the devtools window
5) Open tools

Expected:
Devtools window appears.

Actual:
Devtools window is opened, but inspector has focused the browser window so you don't see it, even after you've selected a node.

Comment 5

[Joe Walker [:jwalker]]

Attachment: v1

Hacked up a trivial patch to see how it feels. This feels OK to me.
Other browsers to the same.

Comment 6

[Paul Rouget [:paul]]

(In reply to Joe Walker [:joe_walker] [:jwalker] from comment #5)
> Created attachment 699723 [details] [diff] [review]
> v1
> 
> Hacked up a trivial patch to see how it feels. This feels OK to me.
> Other browsers to the same.

We're working on the same patch at the same time :)
I'm working on a broader approach.

Comment 7

[Paul Rouget [:paul]]

Attachment: v2

I introduce a new function: lockAndFocus. We still need to keep the lock function because it's useful in some situations. This addresses comment 0 and comment 4.

Comment 8

[Paul Rouget [:paul]]

s/lock/unlock/g in comment 7

Comment 9

[Heather Arthur [:harth]]

Comment on attachment 699757 [details] [diff] [review]
v2

Paul, this isn't working for me on OS X. It appears to focus the devtools window but then immediately re-focus the browser window.

Joe's works for me.

Comment 10

[Paul Rouget [:paul]]

(In reply to Heather Arthur [:harth] from comment #9)
> Comment on attachment 699757 [details] [diff] [review]
> v2
> 
> Paul, this isn't working for me on OS X. It appears to focus the devtools
> window but then immediately re-focus the browser window.

STR in comment 0 or comment 4?

I think I know what's wrong...

Comment 11

[Paul Rouget [:paul]]

(In reply to Paul Rouget [:paul] from comment #10)
> (In reply to Heather Arthur [:harth] from comment #9)
> > Comment on attachment 699757 [details] [diff] [review]
> > v2
> > 
> > Paul, this isn't working for me on OS X. It appears to focus the devtools
> > window but then immediately re-focus the browser window.
> 
> STR in comment 0 or comment 4?
> 
> I think I know what's wrong...

No, I don't. On Linux, it appears to work fine.

Do you use the STR from comment 0?

Comment 12

[Heather Arthur [:harth]]

(In reply to Paul Rouget [:paul] from comment #11)
> (In reply to Paul Rouget [:paul] from comment #10)
> > (In reply to Heather Arthur [:harth] from comment #9)
> > > Comment on attachment 699757 [details] [diff] [review]
> > > v2
> > > 
> > > Paul, this isn't working for me on OS X. It appears to focus the devtools
> > > window but then immediately re-focus the browser window.
> > 
> > STR in comment 0 or comment 4?
> > 
> > I think I know what's wrong...
> 
> No, I don't. On Linux, it appears to work fine.
> 
> Do you use the STR from comment 0?

Yep, using the STR.

Comment 13

[Heather Arthur [:harth]]

Comment on attachment 699757 [details] [diff] [review]
v2

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

::: browser/devtools/inspector/Highlighter.jsm
@@ +295,5 @@
>      if (this.locked === false) return;
>      this.outline.removeAttribute("locked");
>      this.nodeInfo.container.removeAttribute("locked");
>      this.attachMouseListeners();
>      this.locked = false;

We shouldn't get rid of this. Without it, it's confusing to use the highlighter on Mac.

Comment 14

[Heather Arthur [:harth]]

Comment on attachment 699757 [details] [diff] [review]
v2

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

Whoops, last comment was supposed to be:

::: browser/devtools/inspector/Highlighter.jsm
@@ -296,5 @@
>      this.outline.removeAttribute("locked");
>      this.nodeInfo.container.removeAttribute("locked");
>      this.attachMouseListeners();
>      this.locked = false;
> -    this.chromeWin.focus();

We shouldn't get rid of this. Without it, it's confusing to use the highlighter on Mac.

Comment 15

[Heather Arthur [:harth]]

Disregard previous two comments, I got confused. Anyways, I'm currently debugging this on Mac.

Comment 16

[Heather Arthur [:harth]]

Attachment: modification for Mac

This is Paul's patch, but I removed a `win.focus()` that was re-focusing the browser window right after focusing the devtools window on Mac.

I don't know the exact implications of removing that focus() call, but I found that it was added in bug 642471 and looks like it could be unnecessary now. Let me know if it works on Win/Linux.

Comment 17

[Heather Arthur [:harth]]

Attachment: Modifications 2

This one adds one more modification: starting the highlighter will focus the browser window for easier selection, the original behavior regressed by the first patch.

Comment 18

[Paul Rouget [:paul]]

Comment on attachment 702995 [details] [diff] [review]
Modifications 2

Delete this line:
>  let win = aEvent.target.ownerDocument.defaultView

Comment 19

[Paul Rouget [:paul]]

Attachment: v3

Includes Heather's changes.

Comment 20

[Heather Arthur [:harth]]

Comment on attachment 703937 [details] [diff] [review]
v3

awesome, thanks.

Comment 21

[Paul Rouget [:paul]]

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

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/6905b4296e9f

Comment 23

[Dragory]

Is this intended to not work anymore, now that the highlighter doesn't get locked anymore?

Comment 24

[Dragory]

This appears to be currently broken on Windows. Anyone know if the behavious was reverted elsewhere or if it's a regression?