Closed Bug 448080 Opened 12 years ago Closed 12 years ago

switching between tabs fails to update parts of UI

Categories

(Firefox for Android Graveyard :: General, defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

VERIFIED FIXED
fennec1.0m7

People

(Reporter: mfinkle, Assigned: enndeakin)

References

Details

Attachments

(1 file, 1 obsolete file)

1. The titlebar and URL bar location are not updated.

We could hard code the call to update the UI in BrowserUI.selectTab, but listening for an event from deckbrowser would be preferable.

2. The content scroll position is not updated correctly when switching tabs.
Assignee: nobody → enndeakin
Flags: blocking-fennec1.0+
Priority: -- → P2
Target Milestone: --- → Fennec M7
Attached patch update state when tab switching (obsolete) — Splinter Review
Fixes this bug as well as bugs 448740 and 448078. Haven't tested it extensively, but it seems to work ok.
Attachment #332933 - Flags: review?(gavin.sharp)
Comment on attachment 332933 [details] [diff] [review]
update state when tab switching

>   selectTab : function(aTab) {
>     Browser.content.selectTab(aTab);
>+    var browser = getBrowser();
>+    if (this._caption)
>+      this._caption.value = browser.contentDocument.title;
>+    if (this._favicon)
>+      this._favicon.setAttribute("src", browser.mIconURL ? browser.mIconURL.spec :
>+                                                           kDefaultFavIconURL);

You might need to set the this._edit.value as well. Worth noting that this code won't work if an extension calls deckbrowser.selectTab instead of this method. Maybe we should create a listener for "TabSelect" and move this code there.

> 
>-    SpatialNavigation.init(this.content);
>+    try {
>+      SpatialNavigation.init(this.content);
>+    } catch (ex) { }

Did you want this in the patch? SpatialNavigation should always be present.

Otherwise looks good to me.
Blocks: 448078
Comment on attachment 332933 [details] [diff] [review]
update state when tab switching

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

>+    if (faviconURI.schemeIs("javascript") ||
>+        fis.isFailedFavicon(faviconURI))
>+      faviconURI = ios.newURI(kDefaultFavIconURL, null, null);
>+
>+    var browser = getBrowser();
>+    browser.mIconURL = faviconURI;

mIconURL in tabbrowser is a string, having it be an nsIURI in Fennec will just be confusing.

>   selectTab : function(aTab) {
>     Browser.content.selectTab(aTab);
>+    var browser = getBrowser();
>+    if (this._caption)
>+      this._caption.value = browser.contentDocument.title;
>+    if (this._favicon)

Why the null checks?

>+      this._favicon.setAttribute("src", browser.mIconURL ? browser.mIconURL.spec :
>+                                                           kDefaultFavIconURL);

Can just use browser.mIconURL || kDefaultFavIconURL if mIconURL is a string.

I agree with mfinkle on both his points.
Attachment #332933 - Flags: review?(gavin.sharp) → review+
One thing I thought of just now is that instead of putting dragData on each tab, it might be simpler to just trigger the existing location change code in browser.js's webprogresslistener, the way that Firefox's tabbrowser does. I think that would require reworking our progresslistener setup to be more similar to that one though (have the "deckbrowser" keep a reference to the browser WPL and filter calls from each of it's browsers to it, instead of setting the browser WPL directly on each <browser>). I also noticed while looking into this that we're not using a BrowserStatusFilter, which is likely negatively impacting our pageload perf. I guess I'll file a bug on this.
This patch just removes the bitrot and deals with the review comments.
Attachment #332933 - Attachment is obsolete: true
Attachment #334638 - Flags: review?(gavin.sharp)
The patch also fixes a problem where DOMTitleChanged event handlers needed to be called. I moved the browsr.js DOMTitleChanged handler into browser-ui.js at the same time.
Attachment #334638 - Flags: review?(gavin.sharp) → review+
changeset:   108:db3419045da4
tag:         tip
user:        Mark Finkle <mfinkle@mozilla.com>
date:        Wed Aug 20 02:09:22 2008 -0400
summary:     b=448080, p=enn & mfinkle, r=gavin. switching between tabs fails to update parts UI
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a1pre) Gecko/20090821
Fennec/1.0b3pre

and

Mozilla/5.0 (Macintosh; U; Intel Mac OSX 10.5; en-US; rv:1.9.2a2pre)
Gecko/20090808 Fennec/1.0b3pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.