Tabbrowser icon(s) do not work properly

RESOLVED EXPIRED

Status

SeaMonkey
Tabbed Browser
RESOLVED EXPIRED
16 years ago
4 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks: 1 bug, {polish})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(15 attachments)

3.07 KB, patch
Details | Diff | Splinter Review
4.93 KB, patch
Details | Diff | Splinter Review
5.43 KB, patch
Details | Diff | Splinter Review
3.85 KB, patch
Details | Diff | Splinter Review
5.55 KB, patch
Details | Diff | Splinter Review
6.12 KB, patch
Details | Diff | Splinter Review
9.33 KB, patch
Details | Diff | Splinter Review
9.56 KB, patch
Details | Diff | Splinter Review
7.88 KB, patch
Details | Diff | Splinter Review
9.04 KB, patch
Details | Diff | Splinter Review
22.25 KB, patch
Details | Diff | Splinter Review
22.29 KB, patch
Details | Diff | Splinter Review
23.19 KB, patch
Details | Diff | Splinter Review
24.73 KB, patch
Details | Diff | Splinter Review
26.33 KB, patch
Details | Diff | Splinter Review
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.

Comment 1

16 years ago
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.
Created attachment 60794 [details] [diff] [review]
This is a patch v1

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.)
Summary: Favicon and title change wierdly when using IFRAMES and Tabs → Favicon changes wierdly when using IFRAMES and Tabs
Created attachment 60828 [details] [diff] [review]
Patch v.2 to correct some favicon/tabs bugs

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.
Created attachment 60929 [details] [diff] [review]
Patch v.3

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.

Comment 6

16 years ago
->hyatt (patch awaiting review)
Assignee: asa → hyatt
Status: UNCONFIRMED → NEW
Ever confirmed: true
I'll send soon a new patch. It does not use the stupid 
temporary attribute like the patch v. 3.
Created attachment 61250 [details] [diff] [review]
Patch v. 4

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 9

16 years ago
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.
Attachment #61250 - Flags: needs-work+
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.  

Created attachment 61298 [details] [diff] [review]
Patch v. 5 !

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)

Comment 12

16 years ago
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.)
Created attachment 61433 [details] [diff] [review]
Patch v.6 (wondering, how many there will be ;)

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)
Created attachment 63674 [details] [diff] [review]
Patch 2, v.1 

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?


Comment 17

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

Comment 18

16 years ago
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.
Created attachment 64152 [details] [diff] [review]
Patch 2, v.1.1

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.
Created attachment 64515 [details] [diff] [review]
Patch 2, v.1.2

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.)

Comment 22

16 years ago
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?

Updated

16 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.8
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?

Updated

16 years ago
Target Milestone: mozilla0.9.8 → mozilla0.9.9

Updated

16 years ago
Blocks: 120352
Created attachment 65916 [details] [diff] [review]
Patch 2, v.1.3

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)

Updated

16 years ago
Target Milestone: mozilla0.9.9 → Future
Summary: Favicon changes wierdly when using IFRAMES and Tabs → Favicon changes weirdly when using IFRAMES and Tabs
Component: Browser-General → Tabbed Browser

Updated

16 years ago
QA Contact: doronr → sairuh
Created attachment 67087 [details] [diff] [review]
Patch 3, v1

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)
Created attachment 67243 [details] [diff] [review]
Patch 3, v1.01

Oops, one 'removeAttribute' was missing.
Created attachment 67757 [details] [diff] [review]
Patch 3, v1.1

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)
Created attachment 68890 [details] [diff] [review]
Patch 3, v1.2

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?
QA Contact: sairuh → claudius
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?
Keywords: review

Comment 31

16 years ago
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.
Assignee: hyatt → jaggernaut
Status: ASSIGNED → NEW
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.


OS: Windows 98 → All

Comment 35

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

Comment 36

16 years ago
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?
Summary: Favicon changes weirdly when using IFRAMES and Tabs → Tabbrowser hacks
Created attachment 77636 [details] [diff] [review]
Patch 20020404

I removed all the hacks so now the patch is fixing only
(many) icon and title related bugs in tabbrowser.
Summary: Tabbrowser hacks → [PATCH]Tabbrowser title and icon fixes
Summary: [PATCH]Tabbrowser title and icon fixes → [PATCH]Tabbrowser icon(s) and title(s) do not work properly
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/
Keywords: patch, polish, ui
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.
Summary: [PATCH]Tabbrowser icon(s) and title(s) do not work properly → [PATCH]Tabbrowser icon(s) do not work properly
-> me
I'll write a new (smaller?) fix soon.
Assignee: jaggernaut → smaug
Summary: [PATCH]Tabbrowser icon(s) do not work properly → Tabbrowser icon(s) do not work properly
Please re-summarize this bug with a more appropriate summary.
Product: Core → SeaMonkey
QA Contact: claudius → tabbed-browser
Target Milestone: Future → ---

Comment 47

9 years ago
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
Status: NEW → UNCONFIRMED

Comment 48

8 years ago
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
Status: UNCONFIRMED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → EXPIRED
You need to log in before you can comment on or make changes to this bug.