Last Comment Bug 700153 - opening page in new tab opens it in background, but url bar contents in foreground
: opening page in new tab opens it in background, but url bar contents in foreg...
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: ---
Assigned To: Wesley Johnston (:wesj)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-06 10:49 PST by Aakash Desai [:aakashd]
Modified: 2016-07-29 14:20 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Patch v1 (3.19 KB, patch)
2011-11-07 13:24 PST, Wesley Johnston (:wesj)
mark.finkle: review+
Details | Diff | Splinter Review

Description Aakash Desai [:aakashd] 2011-11-06 10:49:52 PST
build: 11/6 birch build

steps to reproduce:
1. go to sports.yahoo.com/nfl/scoreboard
2. long tap on any link
3. select "open page in new tab".

actual results:
the page loads in the background, but the contents of the url bar is updated to the psge title and favicon for the new loaded page.

expected results:
the page loads in the foreground.
Comment 1 Wesley Johnston (:wesj) 2011-11-07 10:54:07 PST
I'll take this since it was my fault.
Comment 2 Wesley Johnston (:wesj) 2011-11-07 13:24:46 PST
Created attachment 572593 [details] [diff] [review]
Patch v1

This adds a parameter to our javascript addTab method, which takes an object which may have a "selected" parameter. The selected value gets passed to Java, and that in turn lets us decide whether or not to select the new tab.
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-07 13:53:15 PST
Comment on attachment 572593 [details] [diff] [review]
Patch v1

># HG changeset patch
># Parent cd32f4fce7ef765b41607f4f7ddfe056fcaeb2fe
>
>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
>--- a/embedding/android/GeckoApp.java
>+++ b/embedding/android/GeckoApp.java
>@@ -770,7 +770,8 @@
>                 Log.i(LOG_NAME, "Created a new tab");
>                 int tabId = message.getInt("tabID");
>                 String uri = message.getString("uri");
>-                handleAddTab(tabId, uri);
>+                Boolean selected = message.getBoolean("selected");

We used "bringToFront" in XUL Fennec. I guess "selected" is ok, just seems odd at first read. Maybe it will grow on me.

>     void handleAddTab(final int tabId, final String uri) {
>+        handleAddTab(tabId, uri, true);
>+    }

Why keep this one at all? Remove please. I didn't see any other callers.

>+
>+    void handleAddTab(final int tabId, final String uri, final boolean selected) {
>         Tab tab = Tabs.getInstance().addTab(tabId, uri);
>+        if (selected) {
>+            Tabs.getInstance().selectTab(tabId);
>+        }
>         mMainHandler.post(new Runnable() { 

Add blank line

>             public void run() {
>-                onTabsChanged();
>                 mBrowserToolbar.updateTabs(Tabs.getInstance().getCount());
>-                mDoorHanger.updateForTab(tabId);
>+                if (selected) {

This is not good enough. By the time the runnable is called, the current tab may not be the selected tab anymore. Use something like

  if (selected && Tabs.getInstance().isSelectedTab(tab)) {

(and make 'tab' final so it can be used in the runnable)

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js


>-                 BrowserApp.addTab(url);
>+                 BrowserApp.addTab(url, {selected: false});

                   BrowserApp.addTab(url, { selected: false });

>       gecko: {
>         type: "Tab:Added",
>         tabID: this.id,
>-        uri: aURL
>+        uri: aURL,
>+        selected: aParams ? aParams.selected : true

This is a bit fragile. We should also check to see if | "selected" in aParams |

Maybe do | let aParams = aParams | { selected: true }; |  above this

and then 

          selected: ("selected" in aParams) ? aParams.selected : true

?


r+ but fix the nits

>       }
>     };
>     sendMessageToJava(message);
Comment 4 Wesley Johnston (:wesj) 2011-11-08 12:50:58 PST
http://hg.mozilla.org/projects/birch/rev/cde2fa77ea5d
Comment 5 Wesley Johnston (:wesj) 2011-11-10 10:33:37 PST
These patches were backed while investigating Talos failures.  Now that tests are green again, we will need to reland.
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2011-11-11 09:02:41 PST
backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547
Comment 7 Aaron Train [:aaronmt] 2011-11-14 06:54:46 PST
20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)

Note You need to log in before you can comment on or make changes to this bug.