The default bug view has changed. See this FAQ.

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
8 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

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

Comment 5

5 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 ago5 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.