Closed Bug 1251863 Opened 8 years ago Closed 8 years ago

Browser Console focuses wrong window when I close it after opening from Scratchpad

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(firefox47 wontfix, firefox48 fixed)

VERIFIED FIXED
Firefox 48
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed

People

(Reporter: arni2033, Assigned: rickychien, Mentored)

References

Details

(Keywords: regression, Whiteboard: [btpp-backlog])

Attachments

(1 file, 5 obsolete files)

>>>   My Info:   Win7_64, Nightly 47, 32bit, ID 20160225030209
STR:
1. Launch new profile. Make sure only 1 window is opened and it is in maximized mode.
2. Open devtools (Ctrl+Shift+I) -> Options, enable "Enable browser chrome and add-on debugging
   toolboxes", enable "Enable remote debugging". Close devtools.
3. Open Scratchpad (Shift+F4)
4. Open Browser Console (Ctrl+Shift+J)
5. Close Browser Console (Ctrl+W or at least Alt+F4)

AR:  Scratchpad window disappears (hides under Firefox window). Firefox window is focused
ER:  Scratchpad window should be visible on the screen and focused

This is regression from bug 1241707. Regression range (read Notes 1,2):
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=39742e742b69087a03366c396d365e03e5df997d&tochange=b8003b4c63c378bd43065f6faf540b5288d9f617

Notes:
1) If you do the mozregression, firefox behaves differently. You have to save a build,
   then extract it and launch manually. Create new profile if you want.
2) You can actually do the mozregression too, but Actual Result (AR) is different in this case.
   AR: Scratchpad window is visible and focused, but there's no caret and I can't type a letter
3) This isn't specific to multiprocess mode.
Thanks for the great bug report (with mozregression and everything)! I'm adding this to the backlog, but we probably won't get to it for a bit. If anyone wants to take it on sooner, we'd happily accept a patch for it.
Priority: -- → P3
Whiteboard: [btpp-backlog]
By the way, this might be an easy fix if we don't try to re-focus anything in the case of the browser console here: https://hg.mozilla.org/mozilla-central/rev/b8003b4c63c3#l1.35 since the browser should handle the window management in that case
Mentor: bgrinstead, lclark
Whiteboard: [btpp-backlog] → [btpp-backlog][good first bug][difficulty=easy]
Hey, I'd like to take this simple patch as my first look into firefox devtools.

My local patch is fixed that problem by removing https://hg.mozilla.org/mozilla-central/rev/b8003b4c63c3#l1.35 as Brain mentioned.

Patch will be submitted soon.
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Attached file Github branch (obsolete) —
Hey Lin, sorry for that I'm not so familiar with firefox review process and as a gaia developer we always do this on Github (love Github way).

Is it possible to review without MozReview and submit a git format patch or covert to hg format after getting r+?
Attachment #8732094 - Flags: review?(lclark)
Comment on attachment 8732094 [details]
Github branch

Thanks for the submission!

The basic idea is right, but it needs a little work. We still want this functionality for the web console (which is what the developer uses when debugging a page). What we should do here is check whether we're in the browser console context (which is what browser/add-on developers use when debugging the browser), in which case we shouldn't do anything. But if we aren't in the browser console context, then it's the web console and this code should run.

You can see this with the test in devtools/client/webconsole/test/browser_webconsole_bug_588342_document_focus.js. I did a git blame and found that the code you deleted in your commit was added in the fix for Bug 1241707. In there, you'll find more details about why this code was added. 

For the next attempt, you can submit a patch using a command called hgp. First you'll need to clone the moz-git-tools repo. Then you'll need to follow the instructions here https://developer.mozilla.org/en-US/docs/Tools/Contributing#Set_up_and_run_hgp
Attachment #8732094 - Flags: review?(lclark) → review-
(In reply to Lin Clark [:linclark] from comment #5)
> We still want this functionality for the web console
Is that really so? I just want to make sure. I perform the following steps:
STR_2:
1. Open Firefox window ("1st window"), load http://example.org/ in a new tab.
2. Open devtools toolbox, detach it to a separate window ("2nd window")
3.A) Do nothing
3.B) Open new tab in 1st window and switch to that tab
4. Switch to another window ("3rd window"), different from 1st and 2nd windows.
  (debugging is finished, but I'm not sure if I won't need that 2nd window again)
5. Hover mouse over 2nd window's button on Windows Taskbar, click close button in preview.
  (I made some work in 3rd window, and want to continue it. Now I'm sure I don't need 2nd window)

AR:  (this behavior is also presented on Firefox 44)
 A) 1st window gets focused
 B) 3rd window is still focused

I described the thinking process during scenario (A), so AR(A) is not expected... As a result of STR, it looks like 3rd window should stay focused. Anyway, when somebody closes devtools, the debugging is already done; I don't know a reason to focus the 1st window. I mean, if it's already not focused, it is for reason.

On the other hand, when I just investigate someting in detached toolbox, focus some windows in the process, and as a result, I sometimes lose the tab and window connected to toolbox. My only option is to attach it to the tab using button "Dock to the bottom", then detach it again. That is not perfect, because I usually detach toolbox when site works differently with different window size, and I need to be sure that I make no changes to the site behavior.

In my opinion, a button "focus connected tab" in devtools could solve both cases in this comment,
and make that mechanism (that _sometimes_ focuses window when I close toolbox) unnecessary.
(In reply to arni2033 from comment #6)
> In my opinion, a button "focus connected tab" in devtools could solve both
> cases in this comment,
> and make that mechanism (that _sometimes_ focuses window when I close
> toolbox) unnecessary.

Sounds like an interesting feature to add for detached toolboxes, although focusing the tab could cause the detached devtools window to disappear behind the browser window so then you'd need to go back and find the correct devtools window if there is more than one.  Another idea is to make the 'pick element' button switch to the correct tab when clicked (but not focus it).
(In reply to Lin Clark [:linclark] from comment #5)
> Comment on attachment 8732094 [details]
> Github branch
> 
> What we should do here is check whether we're in the
> browser console context (which is what browser/add-on developers use when
> debugging the browser), in which case we shouldn't do anything. But if we
> aren't in the browser console context, then it's the web console and this
> code should run.

I'm trying to understand your comment here. My patch does nothing for both kind of cases whatever browser console context is focused on. Your suggestion is that we should focus on firefox window in the case if user isn't in browser console but close while closing it? (such as closing a browser window by Windows 7's Taskbar, I'm not sure this is another way to achieve that on Mac OS X or Linux)
Flags: needinfo?(lclark)
s/but close while closing it/but close by another way/g
My suggestion is more about keeping current functionality. 

If the user is debugging something using the web console (which is in the toolbox that is attached to the window), then when they close the web console, the focus should be restored to wherever it was. For example, if it was in a form field it should be restored to the form field. 

You can find steps to reproduce this in the issue that I mentioned, Bug 1241707. You can also use the test (devtools/client/webconsole/test/browser_webconsole_bug_588342_document_focus.js) to make sure that the functionality continues to work as expected after your change.
Flags: needinfo?(lclark)
Attached patch bug-1251863.patch (obsolete) — Splinter Review
(In reply to Ricky Chien [:rickychien] from comment #11)
> Created attachment 8733799 [details] [diff] [review]
> bug-1251863.patch

Update my patch but it doesn't fix the issue actually.

My solution is that try to invoke getMostRecentWindow() and then focus() to that window. However, I don't understand why it's different with this.target.activeTab.focus() which is able to focus on an element inside content window rather than a browser.xul window.

Is there anything I'm wrong or it's not appropriate to use this method to focus a window?
Flags: needinfo?(lclark)
I guess that this.target.activeTab.focus() is different here [1].

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/client/main.js#1323-1325
It might be worth trying the suggestion in Comment 2 to see if that works. What's being suggested there is basically this:

> if (not running in browser console) {
>   yield this.target.activeTab.focus()
> }

The trick will be figuring out whether this is being run in the browser console or not. I haven't looked at this part of the code base recently enough to remember what variables are available here, so you might need to dig around in the variables. At least sometimes there is a _browserConsole boolean set. Here are some examples: https://dxr.mozilla.org/mozilla-central/search?q=regexp%3A_browserConsole%5Cb&redirect=false&case=true
Flags: needinfo?(lclark)
(In reply to Lin Clark [:linclark] from comment #15)
> It might be worth trying the suggestion in Comment 2 to see if that works.
> What's being suggested there is basically this:
> 
> > if (not running in browser console) {
> >   yield this.target.activeTab.focus()
> > }
> 
> The trick will be figuring out whether this is being run in the browser
> console or not. I haven't looked at this part of the code base recently
> enough to remember what variables are available here, so you might need to
> dig around in the variables. At least sometimes there is a _browserConsole
> boolean set. Here are some examples:
> https://dxr.mozilla.org/mozilla-central/
> search?q=regexp%3A_browserConsole%5Cb&redirect=false&case=true

From there I think this._browserConsole would be true (since this is in WebConsole destroy and BrowserConsole is applying webconsole function on itself when being constructed: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/hudservice.js#635).  Or maybe `this instanceof BrowserConsole`.
Attachment #8734224 - Flags: review?(lclark)
Attachment #8733799 - Attachment is obsolete: true
Test passed on my local machine (devtools/client/webconsole/test/browser_webconsole_bug_588342_document_focus.js)

However, it merely fix in the case of browser console not for the case of detached webconsole or other detached window cases. I'd like to fix it by focusing on previous window for covering general cases. Or do you think it's ok to fix browser console case?
Flags: needinfo?(lclark)
Attachment #8734224 - Attachment is obsolete: true
Attachment #8734224 - Flags: review?(lclark)
Update my patch again. From now on, it will detect whether getMostRecentWindow is browser.xul after console is destoryed. If that is true, we are going to invoke original activeTab.focus as usual.

Note: I've no idea is that a good way to detect particular window with a href
Attachment #8734258 - Attachment is obsolete: true
(In reply to Ricky Chien [:rickychien] from comment #21)
> Created attachment 8734260 [details] [diff] [review]
> Browser Console focuses wrong window

It works well and test has passed.
I haven't seen that issue again after my manual testing, especially for closing a detached webconsole. It will focus on previous window (ex: scratchpad) if it existed otherwise focus on active tab as usual.
Flags: needinfo?(lclark)
Attachment #8734260 - Flags: review?(lclark)
(In reply to Ricky Chien [:rickychien] from comment #18)
> Test passed on my local machine
> (devtools/client/webconsole/test/
> browser_webconsole_bug_588342_document_focus.js)
> 
> However, it merely fix in the case of browser console not for the case of
> detached webconsole or other detached window cases. I'd like to fix it by
> focusing on previous window for covering general cases. Or do you think it's
> ok to fix browser console case?

I think it's fine if detached windows that belong to a tab re-focus that tab when closed.  I'd stick with checking if it's a browser console (should be this._browserConsole).
Attachment #8734260 - Attachment is obsolete: true
Attachment #8734260 - Flags: review?(lclark)
Comment on attachment 8735210 [details] [diff] [review]
Browser Console focuses wrong window

ok, detect by !this._browserConsole.
Attachment #8735210 - Flags: review?(lclark)
Comment on attachment 8735210 [details] [diff] [review]
Browser Console focuses wrong window

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

I tested this out. It turns out that this._browserConsole is not set here, even when it is the browser console. I figured this out by adding this line inside the if statement

> console.log("inside if statement", this._browserConsole)

Then, I opened the browser console, and then closed it. When I looked in the terminal, I saw that my console.log statement had been printed out.

This might be more complicated to fix than originally anticipated. You could try using the debugger or console.log statements to see what variables are available here, but if you'd rather move on to a different bug, we'll totally understand.
Attachment #8735210 - Flags: review?(lclark) → review-
Thank you for your suggestions.

On the other hand, would you like to take a look into the patch on comment 22?

https://bugzilla.mozilla.org/attachment.cgi?id=8734260&action=diff

Did not ask you whether it is a better approach to fix by calling getMostRecentWindow(). But I'm still thinking that is worth to try. :)
Flags: needinfo?(lclark)
arni, would you mind checking if the patch in Comment 22 fixes the issue for you?
Flags: needinfo?(lclark) → needinfo?(arni2033)
Whiteboard: [btpp-backlog][good first bug][difficulty=easy] → [btpp-backlog]
I'd still prefer to use the this._browserConsole approach over anything that does wm.getMostRecentWindow and checks for browser.xul (which is Firefox specific).

In fact, I applied the latest patch https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1251863&attachment=8735210 and I believe it's working as expected for browser console and web console.  Lin, can you double check with that patch applied and make confirm that it's not working?
Flags: needinfo?(arni2033) → needinfo?(lclark)
Comment on attachment 8735210 [details] [diff] [review]
Browser Console focuses wrong window

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

After discussing with :bgrins it looks like my STR were slightly off. This is working as expected. Thanks for the patch!
Attachment #8735210 - Flags: review- → review+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Bugs are only FIXED when the patch reaches mozilla-central.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/b932cef38db8
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
The regression was from Firefox 47 but it is getting late in the beta cycle to uplift this. Ricky or Lin, if you disagree and think this must be fixed in 47, and is safe to uplift, please n-i to :ritu.   Otherwise, the fix is going to be in 48.
Flags: needinfo?(rchien)
This is a low priority bug that's unlikely to affect users too much. It's fine for it to land in 48.
Flags: needinfo?(rchien)
I have reproduced this bug with Nightly 47.0a1 (2016-02-25) on Windows 7, 64Bit!


This bug's fix is verified on latest Beta (48.0b8) , Nightly , Developer Edition.


 Build ID     20160714050942
 User Agent   Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0


 Build ID     20160714030208
 User Agent   Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

 Build ID     20160708004052
 User Agent   Mozilla/5.0 (WindowsNT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0
Status: RESOLVED → VERIFIED
Summary: Browser Console focuses wrong window when I close it after openning from Scratchpad → Browser Console focuses wrong window when I close it after opening from Scratchpad
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.