The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 18

Status

()

Firefox
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Gavin, Assigned: avp)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=dao][lang=js])

Attachments

(1 attachment, 6 obsolete attachments)

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).
Summary: replace calls to content.focus with calls to gBrowser.selectedBrowser.focus() → replace calls to content.focus() with calls to gBrowser.selectedBrowser.focus()
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
Summary: replace calls to content.focus() with calls to gBrowser.selectedBrowser.focus() → Replace content.focus() with gBrowser.selectedBrowser.focus() and/or window.focus()
Whiteboard: [good first bug][mentor=dao][lang=js]
(Assignee)

Comment 2

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

Thanks.
(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?
(Assignee)

Comment 4

5 years ago
Yes I am having a local copy of mozilla-central and have also built firefox.
Ok, is commt 1 clear enough? Do you have any specific questions?
Assignee: nobody → abhishekp.bugzilla
I mean comment 1.
(Assignee)

Comment 7

5 years ago
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() ?
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.
(Assignee)

Comment 9

5 years ago
Created attachment 661480 [details] [diff] [review]
Replaced content.focus() with gBrowser.selectedBrowser.focus() and/or window.focus()
Attachment #661480 - Flags: feedback?(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
Attachment #661480 - Flags: feedback?(dao) → feedback+
(Assignee)

Comment 11

5 years ago
Created attachment 661784 [details] [diff] [review]
Replaced content.focus() with gBrowser.selectedBrowser.focus() and/or window.focus()

Made the changes to the .hgrc file and resubmitting the patch
Attachment #661480 - Attachment is obsolete: true
Attachment #661784 - Flags: feedback?(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.
Attachment #661784 - Flags: feedback?(dao) → feedback+
(Assignee)

Comment 13

5 years ago
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.
Attachment #661784 - Attachment is obsolete: true
Attachment #662070 - Flags: feedback?(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.
Attachment #662070 - Flags: feedback?(dao)
(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.
(Assignee)

Comment 16

5 years ago
thanks Josh !
(Assignee)

Comment 17

5 years ago
Created attachment 662203 [details] [diff] [review]
Replaced content.focus() with gBrowser.selectedBrowser.focus() and/or window.focus()
Attachment #662070 - Attachment is obsolete: true
Attachment #662203 - Flags: feedback?(dao)
Abhishek, could you please re-add the stuff mentioned in comment 10 to your ~/.hgrc file? Thanks!
(Assignee)

Comment 19

5 years ago
Created attachment 662384 [details] [diff] [review]
Replaced content.focus() with gBrowser.selectedBrowser.focus() and/or window.focus()

Sorry about that ! had forgotten to modify the .hgrc file
Attachment #662203 - Attachment is obsolete: true
Attachment #662203 - Flags: feedback?(dao)
Attachment #662384 - Flags: feedback?(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();
Attachment #662384 - Flags: feedback?(dao) → feedback+
(Assignee)

Comment 21

5 years ago
Created attachment 662455 [details] [diff] [review]
Made the suggested changes
Attachment #662384 - Attachment is obsolete: true
Attachment #662455 - Flags: review?(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.
Attachment #662455 - Flags: review?(dao) → review+
(Assignee)

Comment 23

5 years ago
Created attachment 662466 [details] [diff] [review]
Made the suggested changes
Attachment #662455 - Attachment is obsolete: true
Attachment #662466 - Flags: review?(dao)
Comment on attachment 662466 [details] [diff] [review]
Made the suggested changes

Thanks!
Attachment #662466 - Flags: review?(dao) → review+
Try server results will appear here: https://tbpl.mozilla.org/?tree=Try&rev=0bbe5b398691
(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
https://hg.mozilla.org/mozilla-central/rev/376d08b69d82
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Depends on: 802024
You need to log in before you can comment on or make changes to this bug.