opening page in new tab opens it in background, but url bar contents in foreground

VERIFIED FIXED

Status

()

Firefox for Android
General
P1
normal
VERIFIED FIXED
6 years ago
10 months ago

People

(Reporter: aakashd, Assigned: wesj)

Tracking

unspecified
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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.

Updated

6 years ago
OS: Linux → Android
Hardware: Other → ARM

Updated

6 years ago
Assignee: nobody → sriram
Priority: -- → P1
(Assignee)

Comment 1

6 years ago
I'll take this since it was my fault.
Assignee: sriram → wjohnston
(Assignee)

Comment 2

6 years ago
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.
Attachment #572593 - Flags: review?(mark.finkle)
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);
Attachment #572593 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 4

6 years ago
http://hg.mozilla.org/projects/birch/rev/cde2fa77ea5d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

6 years ago
These patches were backed while investigating Talos failures.  Now that tests are green again, we will need to reland.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)
Status: RESOLVED → VERIFIED
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.