I think the favicon should always be what the main frame describes in LINK-element, but in the given URL when user changes the content of the IFRAME, a new icon is loaded from server's root. And this happens even if all the pages includes the same LINK-declaration (which does not point to the server's root). This occurs only when using Tabs, not in the 'one-page-one-window'-mode. BTW. The Tab loses also the title of the page.

Have you verified by looking at the server logs that the link icon is loaded each time? It might just be that the spinner replaces the icon while the network library loads the file ad the link icon is cached.

This patch makes the icons and titles of the tabs to keep right values when browsing pages that contains IFRAMES or FRAMES. This works fine for me. (BTW. I don't have a CVS access)

Changing bug-summary. was: Favicon and title change wierdly when using IFRAMES and Tabs now: Favicon changes wierdly when using IFRAMES and Tabs The 'title'-part of this bug is a dublicate of http://bugzilla.mozilla.org/show_bug.cgi?id=101831. I'll try to make a patch which corrects only the 'icon'-bug. (My patch for titles has some problems.)

So this patch is trying to make icons to show up more correctly. It changes tabbrowser so, that when browsing frames, the mainframe's icon will be used (if an icon is defined). Also: Formenly when using a page with an icon in "one-page-per-window"-mode and then starting to work with tabs, the icon was lost. This patch removes that problem too. Probably there is still something that could be made better (or a bit cleaner), but for now this is enough for me.

This is a more polished patch. Could someone try it - I hope it works well enough. Why is this bug UNCONFIRMED? I see icons to disapearing in Windows & Linux when using pages with frames. And also icons get lost sometimes when changing from 'one-tab-mode' to 'multi-tab-mode' (= adding another tab). This patch is trying to make those things to works properly.

->hyatt (patch awaiting review)

I'll send soon a new patch. It does not use the stupid temporary attribute like the patch v. 3.

So this is even more clean patch. Hope it works ok. It is made with Patch Maker using Linux (Moz 2001121008). (There is something wierd when trying to use PM's patches made in Windows in Linux.) I've been using this patch for a while now and it seems to work the way I like.

Comment on attachment 61250 [details] [diff] [review] Patch v. 4 >--- xpfe/global/./resources/content/bindings/tabbrowser.xml.bak Tue Dec 11 14:57:58 2001 >+++ xpfe/global/./resources/content/bindings/tabbrowser.xml Tue Dec 11 16:37:23 2001 >@@ -126,7 +126,10 @@ >+ _mOldURI: "", //This is a hack for keeping the icon of the mainframe >+ _mOldIcon: "", //This is a hack for keeping icon of the mainframe >+ _mTabPanel: null, your indentations should line up. >@@ -156,9 +159,19 @@ > const nsIChannel = Components.interfaces.nsIChannel; > if (!this.mBlank && aStateFlags & nsIWebProgressListener.STATE_START && > aStateFlags & nsIWebProgressListener.STATE_IS_NETWORK) { >+ var i = 0; this thing here spells like a for loop, you're interating over a list: <<<<<< >+ var tab = this.mTabBrowser.mTabContainer.firstChild; >+ do { >+ if (tab == this.mTab) break; >+ ++i; >+ tab = tab.nextSibling >+ }while(tab); ====== for (var tab = this.mTabBrowser.mTabContainer.firstChild; tab && tab != this.mTab; ++i, tab = tab.nextSibling); >>>>>> is it possible for you to run out of tabs? if it is, then this next statement will cause some sort of error. if not, then please remove the tab condition from the loop. >+ this._mTabPanel = this.mTabBrowser.mPanelContainer.childNodes[i]; >+ this._mOldURI = this._mTabPanel.currentURI; > this.mTab.setAttribute("busy", "true"); > this.mTab.label = this.mTabBrowser.mStringBundle.getString("tabs.loading"); > this.mTab.removeAttribute("image"); >+ this._mOldIcon = this.mIcon; > this.mIcon = ""; > > if (this.mTabBrowser.mCurrentTab == this.mTab) >@@ -166,12 +179,22 @@ > } > else if (aStateFlags & nsIWebProgressListener.STATE_STOP && > aStateFlags & nsIWebProgressListener.STATE_IS_NETWORK) { >+ var newuri = ""; >+ if(this._mTabPanel) >+ newuri = this._mTabPanel.currentURI; >+ this._mTabPanel = null; >+ var wasURIChanged = !(newuri == this._mOldURI); >+ this._mOldURI = ""; > if (this.mBlank) > this.mBlank = false; > > this.mTab.removeAttribute("busy"); > > var location = aRequest.QueryInterface(nsIChannel).URI; >+ if(!wasURIChanged) { >+ this.mIcon = this._mOldIcon; >+ } hrm. local style would appear to be if<space>(cond)\n<whitespace-indent>statement\n [no braces] >+ this._mOldIcon = ""; > if (this.mIcon) { > this.mTab.setAttribute("image", this.mIcon); > mIcon = ""; >@@ -526,11 +549,15 @@ > // Create our close box. > this.createCloseBox(); > >- // Hook up our favicon. >- var uri = this.mCurrentBrowser.currentURI; >- if (this.shouldLoadFavIcon(uri)) >- this.loadFavIcon(uri, "image", this.mCurrentTab); >+ // Hook up our favicon. We'll get the same icon what was used already. >+ var iconhref = null; >+ if(gProxyFavIcon){ again, if<space>... >+ iconhref = gProxyFavIcon.getAttribute('src'); >+ if(iconhref) >+ this.mCurrentTab.setAttribute('image', iconhref); >+ } > >+ addition of a random newline ^^ > // Remove all our progress listeners from the active browser. > if (this.mProgressListeners) { > for (var i = 0; i < this.mProgressListeners.length; i++) { clearly a final patch would use mFoo for member variables instead of _mFoo.

Thanks for comments. I'm just starting to make patches to Mozilla, so there are many things to learn about Mozilla's coding style, etc. Those "your indentations should line up." are made by Patch Maker, in my code indentations are ok. "if<space>(cond)\n<whitespace-indent>statement\n [no braces]" I forgot to use this, instead I used what I'm used to do. "clearly a final patch would use mFoo for member variables instead of _mFoo." I saw this _mFoo elsewhere too, so thought it could be allowed to use it. And I think it is even pretty clear this way that those variables are kind of 'private'-variables.

I really hope that this patch might be good enough. (I should think more and write less :) ) This patch is correcting also bugs: http://bugzilla.mozilla.org/show_bug.cgi?id=101831 http://bugzilla.mozilla.org/show_bug.cgi?id=101827 or at least the test cases started to work as expected. (This patch is made in Windows with Patch Maker using Moz 2001121003)

There's a much easier way to fix this that just involves testing window parentage.

Hyatt, I don't understand what you mean, but probably its just so simple that I cannot see it. But which part of the patch v5 did you mean anyway. There are actually fixes for at least these bugs (I suppose): http://bugzilla.mozilla.org/show_bug.cgi?id=113798 http://bugzilla.mozilla.org/show_bug.cgi?id=101831 http://bugzilla.mozilla.org/show_bug.cgi?id=101827 and a bug when icon get lost when changing from 'one-page-one-window'-mode to tabbed mode. (is it http://bugzilla.mozilla.org/show_bug.cgi?id=109942 ?) and maybe: http://bugzilla.mozilla.org/show_bug.cgi?id=109959 (I can't test this currently, because I'm using an old Moz.)

Because I didn't understand what Hyatt was saying I made still a new patch, which actually corrects bugs (I hope): http://bugzilla.mozilla.org/show_bug.cgi?id=113798 http://bugzilla.mozilla.org/show_bug.cgi?id=101831 http://bugzilla.mozilla.org/show_bug.cgi?id=101827 http://bugzilla.mozilla.org/show_bug.cgi?id=109942 http://bugzilla.mozilla.org/show_bug.cgi?id=109959 http://bugzilla.mozilla.org/show_bug.cgi?id=105842 http://bugzilla.mozilla.org/show_bug.cgi?id=108189 http://bugzilla.mozilla.org/show_bug.cgi?id=108350 (Some of these are probably dublicates.) The whole patch is becoming too large, but unfortunately there is pretty many bugs in the handling of titles and icons. Maybe the icon&title -stuff should be somehow rearranged? (This patch was made in Linux with Patch Maker using Moz 2001121108)

This totally rewritten patch is for current tabbrowser. It includes some parts of neil@parkwaycc.co.uk's patch in http://bugzilla.mozilla.org/show_bug.cgi?id=101827. There was not enough parentNodes in <xul:tab onerror="this.parentNode.parentNode.., this patch fixes that too. (PatchMaker / Win98 / Moz 2002010203)

Has anyone tried my patch. For me it has been working well in windows and linux. Unfortunately there are still some bugs in tabs (or maybe those are not tab-bugs but something else). Anyway I think the patch is fixing the most common bugs conserning titles and icons. reviews?

I'm not sure we should be setting an icon property on the document. That seems kind of hacky to me.

What was wrong with the mIcon property being on the listener? Why did it have to be removed?

I think the icon should be in the document-object. It is easier to use, and it is really a property of a document - document might or might not have an icon. So somehow an icon is a 'member' of a document. And I can't understand why an icon should be a property of a TabProgressListener-object. Those are pretty unrelated things.

Still one patch. Polishing syntax and adding 2 hasAttribute -checks (and fixing one bug more, I don't know the bug-id :) . Using mIcon-property would make the code a bit more unclean, especially in the addTab and onLinkAdded -methods. So I do prefer the icon-property in document.

Well, this is the patch which uses mIcon-property in TabProgressListener-objects. (The icon-property in document-objects is removed.) Reviews? (If this is finally OK, I hope someone could check this in.)

I think we should go further and make tabs completely ignore both starts and stops that are initiated because of iframe transitions. It doesn't make sense to put in the animating icon and the "Loading..." message when only an iframe is changing. Thoughts?

I think there should be some thing to indicate that a frame is loading. It should be possible to set a tab to load some big frame and meanwhile browse another tab. And the indicator (=icon) tells to the user when the big-frame-tab is ready to use (=loaded). This is important especially with slow internet connections. A good example is the http://java.sun.com/j2se/1.4/docs/api/index.html. So that's why we need the start-stop -state handling in tabbrowser too, I think. But maybe the animated icon is enough with frames and Loading... -text should be seen only when a totally new page is loading?

This is the diff for current tabbrowser.xml Added some minor things, so that the patch is working even better. This patch is fixing also bug http://bugzilla.mozilla.org/show_bug.cgi?id=107669 (at least for me). (PatchMaker / Linux / Moz 2002012106)

This patch fixes even more bugs. It also includes a hack for http://bugzilla.mozilla.org/show_bug.cgi?id=103452. (That might be just a temporary fix, but it is still better than nothing.) I had to change tabbox.xml#tab a bit too, so that an icon of a tab is always painted (the default icon or a site/favicon). (PatchMaker/Linux/Moz2002012921)

Oops, one 'removeAttribute' was missing.

Well, this patch could be ready for reviews. It fixes still more bugs: 1.when using href="#xyz" -style links the icon was lost (some cases). 2.when switching tabs the urlbars icon disapeared (in one case). (Linux / Moz 2002020222)

Sorry for all these obsolete patches... But I still found one case where the icon disapeared. (Image-element's onload is sometimes called even if there is no image to paint, so there must be a hasAttribute('src')-check.) Reviews?

The patch for current tree is in http://www.cs.helsinki.fi/u/pettay/bugzilla/

I have used the patch for 9 days and it is working very well, I think. Maybe someone doesn't like the window.close() -hack, but otherwise the code should be pretty clean. So reviews or at least some comments?

Would you mind splitting this patch into patches for each of the issues it tries to address? You can lump things together if the issues are related or interact. Attach these patches to whichever bug report covers the issue (see your comment #14 for a list). The reason I ask this is because that way we can more easily accept or reject parts of the current patch, instead of having to reject the whole patch because of disagreement over a small part of it.

I'm afraid, it is impossible to split the patch, the changes to the code of the tabbrowser are to big. (Only the window.close() -hack could be easily remowed.) (And I do not have time to do the splitting.) But I made a commented version of this patch. http://www.cs.helsinki.fi/u/pettay/bugzilla/bug113798.20020228.diff_with_extra_comments (All the // -starting lines are comments.) I hope it makes things a bit more clear. The newest real patch is still in http://www.cs.helsinki.fi/u/pettay/bugzilla/

Smaug: I took the parts of the patch that apply to bug 101723, created a seperate patch, and attached it to that bug. Hope it helps you if you need te split the patch.

The bug 101723 is now fixed (?), so this patch is a bit smaller again.

I've extracted the part (hack) that fixes window.close(). Diff attached to bug 103452.

Oh, great :( I just didn't happen to notice the comment about the most recent version until now... well, got to do it again then, I guess.

The current version fixes http://bugzilla.mozilla.org/show_bug.cgi?id=110421 too. (Actually there has been a test for image blocking for a long time but it was not working properly.) And also fixing some (old) mixed using of URI and URL.

Tabbrowser focus bugs, fix part 1: The focus of the elements is now remembered in tabbrowser, when switching tabs with mouse. Unfortunately I haven't got it to work with ctrl-PgUp / ctrl-PgDn. Yet?

I removed all the hacks so now the patch is fixing only (many) icon and title related bugs in tabbrowser.

Still a bit better patch, fixing more bugs. (for example trying to load icon a bit fewer times in some cases.) Now when <link>-icon cannot be loaded, trying to load favicon. http://www.cs.helsinki.fi/u/pettay/bugzilla/

I'm still using the patch all the time. It just fixes so many annoying but not so critical bugs.

The newest patch works at least with Moz20020610/WinXP http://www.cs.helsinki.fi/u/pettay/bugzilla/

The current patch fixes one title related bug more. Tested with WinXP 20020701. http://www.cs.helsinki.fi/u/pettay/bugzilla/

titles were fixed in bug 152127, so changing the title. Current patch is in the old place.

-> me I'll write a new (smaller?) fix soon.

Please re-summarize this bug with a more appropriate summary.

MASS-CHANGE: This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state. If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way. If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar). If no action happens within the next few months, we move this bug report to an EXPIRED state. Query tag for this change: mass-UNCONFIRM-20090614

MASS-CHANGE: This bug report is registered in the SeaMonkey product, but still has no comment since the inception of the SeaMonkey project 5 years ago. Because of this, we're resolving the bug as EXPIRED. If you still can reproduce the bug on SeaMonkey 2 or otherwise think it's still valid, please REOPEN it and if it is a platform or toolkit issue, move it to the according component. Query tag for this change: EXPIRED-20100420