691925 - Replace content.focus() with gBrowser.selectedBrowser.focus() and/or window.focus()

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

gBrowser.selectedBrowser.focus() is more e10s-friendly than window.content.focus(), and should generally be equivalent.

As I understand it, the only difference is whether the windows in question will be raised and/or scrolled to, and in a lot of cases we don't care about that distinction (because the window in question is being interacted with, and therefore is already raised).

Comment 1

[Dão Gottwald [:dao]]

content.focus() does two things:

1) it focuses the content area in case some chrome element such as the location bar was previously focused -- gBrowser.selectedBrowser.focus() should be used for this

2) it tells the operating system's window manager to raise the browser window (e.g. if it was minimized or covered by some other window) -- window.focus() should be used for this

We need to decide case-by-case whether we want 1) or 2) or both:
http://mxr.mozilla.org/mozilla-central/search?string=content.focus&find=browser%2F&filter=[+.]content

Comment 2

[Abhishek Potnis [:avp]]

Hi Dao,
I am interested on working on this bug. Could you please guide me on getting started with this bug.

Thanks.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Abhishek Potnis [:abhishekp] from comment #2)
> Hi Dao,
> I am interested on working on this bug. Could you please guide me on getting
> started with this bug.

Do you already have a local copy of the Firefox code base? Have you built Firefox?

Comment 4

[Abhishek Potnis [:avp]]

Yes I am having a local copy of mozilla-central and have also built firefox.

Comment 5

[Dão Gottwald [:dao]]

Ok, is commt 1 clear enough? Do you have any specific questions?

Comment 6

[Dão Gottwald [:dao]]

I mean comment 1.

Comment 7

[Abhishek Potnis [:avp]]

Could you please tell me which must be the file/files that need to be edited and also should I replace content.focus() with gBrowser.selectedBrowser.focus() or window.focus() ?

Comment 8

[Dão Gottwald [:dao]]

Sorry for the late response, I wasn't CC'd.

Here's the list of files:
http://mxr.mozilla.org/mozilla-central/search?string=content.focus&find=browser%2F&filter=[+.]content

Can you try to guess for each instance whether it wants to focus the content area (i.e. gBrowser.selectedBrowser.focus()) or raise the window (i.e. window.focus())? It's ok if you get it wrong in some cases, I'll double-check.

Comment 9

[Abhishek Potnis [:avp]]

Attachment: Replaced content.focus() with gBrowser.selectedBrowser.focus() and/or window.focus()

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 661480 [details] [diff] [review]
Replaced content.focus() with gBrowser.selectedBrowser.focus() and/or window.focus()

Looks good at first glance. To make this easier to review, please add this to your ~/.hgrc file:

[diff]
git = 1
showfunc = 1
unified = 8

Comment 11

[Abhishek Potnis [:avp]]

Attachment: Replaced content.focus() with gBrowser.selectedBrowser.focus() and/or window.focus()

Made the changes to the .hgrc file and resubmitting the patch

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 661784 [details] [diff] [review]
Replaced content.focus() with gBrowser.selectedBrowser.focus() and/or window.focus()

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -1179,17 +1179,17 @@
>           }
> 
>           if (!aLoadInBackground) {
>             if (firstTabAdded) {
>               // .selectedTab setter focuses the content area
>               this.selectedTab = firstTabAdded;
>             }
>             else
>-              window.content.focus();
>+              window.focus();

This code really wants to focus the content area.

>diff --git a/browser/base/content/test/browser_bug455852.js b/browser/base/content/test/browser_bug455852.js
>--- a/browser/base/content/test/browser_bug455852.js
>+++ b/browser/base/content/test/browser_bug455852.js
>@@ -1,12 +1,12 @@
> function test() {
>   is(gBrowser.tabs.length, 1, "one tab is open");
> 
>-  content.focus();
>+  window.focus();
>   isnot(document.activeElement, gURLBar.inputField, "location bar is not focused");

This code wants to focus the content area too.

>--- a/browser/base/content/test/browser_bug520538.js
>+++ b/browser/base/content/test/browser_bug520538.js
>@@ -1,11 +1,11 @@
> function test() {
>   var tabCount = gBrowser.tabs.length;
>-  content.focus();
>+  window.focus();
>   browserDOMWindow.openURI(makeURI("about:blank"),
>                            null,
>                            Ci.nsIBrowserDOMWindow.OPEN_NEWTAB,
>                            Ci.nsIBrowserDOMWindow.OPEN_EXTERNAL);

ditto

>--- a/browser/base/content/utilityOverlay.js
>+++ b/browser/base/content/utilityOverlay.js
>@@ -318,17 +318,17 @@ function openLinkIn(url, where, params) 
>   }
> 
>   // If this window is active, focus the target window. Otherwise, focus the
>   // content but don't raise the window, since the URI we just loaded may have
>   // resulted in a new frontmost window (e.g. "javascript:window.open('');").
>   var fm = Components.classes["@mozilla.org/focus-manager;1"].
>              getService(Components.interfaces.nsIFocusManager);
>   if (window == fm.activeWindow)
>-    w.content.focus();
>+    window.focus();
>   else
>     w.gBrowser.selectedBrowser.focus();

You need to use w instead of window here. Also, this code wants to both raise the window and focus the content area.

Looks good otherwise.

Comment 13

[Abhishek Potnis [:avp]]

Attachment: Patchv 2.0

I did a hg pull -u before applying my patch and some files appeared to be have been modified, but I am confused because I could not see the changes in http://dxr.lanedo.com/mozilla-central, I am submitting the patch with the suggested changes.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 662070 [details] [diff] [review]
Patchv 2.0

It looks like you committed the previous patch before generating this one. You need to revert this commit (via hg strip, for instance) and generate the diff from there.

Comment 15

[Josh Matthews [:jdm]]

(In reply to Abhishek Potnis [:abhishekp] from comment #13)
> Created attachment 662070 [details] [diff] [review]
> Patchv 2.0
> 
> I did a hg pull -u before applying my patch and some files appeared to be
> have been modified, but I am confused because I could not see the changes in
> http://dxr.lanedo.com/mozilla-central, I am submitting the patch with the
> suggested changes.

For future reference, dxr.lanedo.org is no longer being updated. Use mxr.mozilla.org for guaranteed results.

Comment 16

[Abhishek Potnis [:avp]]

thanks Josh !

Comment 17

[Abhishek Potnis [:avp]]

Attachment: Replaced content.focus() with gBrowser.selectedBrowser.focus() and/or window.focus()

Comment 18

[Dão Gottwald [:dao]]

Abhishek, could you please re-add the stuff mentioned in comment 10 to your ~/.hgrc file? Thanks!

Comment 19

[Abhishek Potnis [:avp]]

Attachment: Replaced content.focus() with gBrowser.selectedBrowser.focus() and/or window.focus()

Sorry about that ! had forgotten to modify the .hgrc file

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 662384 [details] [diff] [review]
Replaced content.focus() with gBrowser.selectedBrowser.focus() and/or window.focus()

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -1182,17 +1182,17 @@
>           }
> 
>           if (!aLoadInBackground) {
>             if (firstTabAdded) {
>               // .selectedTab setter focuses the content area
>               this.selectedTab = firstTabAdded;
>             }
>             else
>-              window.content.focus();
>+              gBrowser.selectedBrowser.focus();

nit: gBrowser is the tabbrowser node itself, so you can just write this.selectedBrowser.focus() here.

>--- a/browser/base/content/utilityOverlay.js
>+++ b/browser/base/content/utilityOverlay.js

>+  if (window == fm.activeWindow) {
>+    w.focus();
>+    w.gBrowser.selectedBrowser.focus();
>+  }
>   else
>     w.gBrowser.selectedBrowser.focus();

This can be simplified to:

  if (window == fm.activeWindow)
    w.focus();
  w.gBrowser.selectedBrowser.focus();

Comment 21

[Abhishek Potnis [:avp]]

Attachment: Made the suggested changes

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 662455 [details] [diff] [review]
Made the suggested changes

>--- a/browser/components/sessionstore/src/SessionStore.jsm
>+++ b/browser/components/sessionstore/src/SessionStore.jsm

>     if (this.windowToFocus && this.windowToFocus.content) {
>-      this.windowToFocus.content.focus();
>+      this.windowToFocus.focus();
>     }

It just occurred to me that the if condition doesn't need to check this.windowToFocus.content anymore.

I'll push this to the try server to make sure this doesn't break any tests.

Comment 23

[Abhishek Potnis [:avp]]

Attachment: Made the suggested changes

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 662466 [details] [diff] [review]
Made the suggested changes

Thanks!

Comment 25

[Dão Gottwald [:dao]]

Try server results will appear here: https://tbpl.mozilla.org/?tree=Try&rev=0bbe5b398691

Comment 26

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #25)
> Try server results will appear here:
> https://tbpl.mozilla.org/?tree=Try&rev=0bbe5b398691

no failures

https://hg.mozilla.org/integration/mozilla-inbound/rev/376d08b69d82

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/376d08b69d82