BrowserApp.addTab(aURI) doesn't put the new tab in the foreground

VERIFIED FIXED

Status

()

--
major
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: fabrice, Assigned: mfinkle)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
That creates a new tab, and the title and favicon are changed, but not the web content.
An additional call to BrowserApp.selectTab() is needed.
Created attachment 578509 [details] [diff] [review]
patch

This patch was a little more complicated than I thought it would be.

As fabrice menitions, this does not make the new tab active/selected): BrowserApp.addTab(uri, { selected: true });

You need to also call:
BrowserApp.selectTab(tab);

This is because the GeckoApp handleAddTab was only doing a tiny bit of selecting the new tab. It only told the Tabs object that the tab was selected. No other code was informed. Sending a Tab:Selected message, called handleSelectTab and that did a bunch of selection changes.

So, I made the Tab:Add code also call handleSelectTab if needed. In doing so, I could remove the "selected" param in handleAddTab, and let handleSelectTab do all the work. handleSelectTab needed a few checks added to it, since if this was the very first tab, no tab was already selected, so "selTab" could be null.

I also made some changes to browser.js because we had to tell Gecko that the new tab was active/selected too. Some minor cleanup is also in the code. Since BrowserApp.addTab defaults to making the tab active, we could remove some code that was already adding a separate call.

I tested about:home behavior, adding tabs via the UI and adding tabs via the code - all seem to work OK.
Assignee: nobody → mark.finkle
Attachment #578509 - Flags: review?(sriram)
Comment on attachment 578509 [details] [diff] [review]
patch

This patch looks good to me.
Attachment #578509 - Flags: review?(sriram) → review+
would bug 706860 be a dup then since this looks like the underlying cause?
Do we still need things like:
http://mxr.mozilla.org/projects-central/source/birch/mobile/android/chrome/content/browser.js#899

It doesn't look like you're setting BrowserApp.selectedTab either. I don't really know this code. Is that trickling back from Java?
(In reply to Wesley Johnston (:wesj) from comment #4)
> Do we still need things like:
> http://mxr.mozilla.org/projects-central/source/birch/mobile/android/chrome/
> content/browser.js#899
> 
> It doesn't look like you're setting BrowserApp.selectedTab either. I don't
> really know this code. Is that trickling back from Java?

Thanks for taking a look:

BrowserApp.selectTab(...) sends a Tab:Selected" message back to Java. We don't want that. We just want the tab to be selected in browser.js because the "Tab:Added" handler will do call handleSelectTab too, if needed.

| tab.active = true | actually selects and activates a tab in browser.js, which we do in BrowserApp.addTab(...) if needed.

We should have your concerns covered here.
https://hg.mozilla.org/projects/birch/rev/0796b6902075
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
I wasn't able to reproduce this issue on the latest native fennec build:
Build ID: Mozilla/5.0 (Android; Linux armv7l; rv:11.0a1) Gecko/20111206 Firefox/11.0a1 Fennec/11.0a1 - Native Fennec build
Device: Samsung GalaxyS, Android 2.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.