169380 - RFE: Add LED to tab labels to indicate tab load status

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, enhancement)

Description

[Nick_Jenkins]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1) Gecko/20020826
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1) Gecko/20020826

When tabs load, it would be useful if they gave a colour indication of how
loaded they are (like a very simple reflection of the progress bar that shows in
the status bar). So when a group is opened, initially every tab opened could
start out with a red block in it's tab. As a particular tab loaded, it could go
yellow (perhaps with 2 or 3 substeps of yellow), and then when it is totally
finished go green. 

Currently tabs indicate they are either loaded or loading; The extra information
would be useful. It would for example allow a bookmark group to be opened when
using a dial-up modem, and for the user to then prioritise for reading only
those tabs that have either finished loading or that are quite close to being
fully loaded.

Reproducible: Always

Steps to Reproduce:




There's an example of a 3-tier traffic light system for tab status shown in a
dialog box in point 22 of http://multizilla.mozdev.org/features.html ; Around
two more tiers to indicate various states of 'pending' would be ideal.

Comment 1

[Stefan Borggraefe]

This would be an alternative to Bug 117161.

Comment 2

[Nick_Jenkins]

Attachment: A tab in the different states of loading

I hadn't found bug 117161 previously, so thank you for mentioning that, and
yes, this is an alternative approach.

The screenshots attached to that bug are quite helpful, so to illustrate I've
taken one of those and modified it to show what this idea could look like, and
I'm attaching this now.

Comment 3

[Nick_Jenkins]

There are a few differences to this approach to the one mentioned in 117161, namely:
1) It doesn't use the background of the tab to illustrate load progress. This is
because the size of each tab fluctuates as tabs are opened and closed, so you
can't just look at each progress bar, you have to compare it to the size of the
tab, and secondly this is probably too much information - the user just wants to
know roughly how loaded something is (or not).
2) This does away with the separate page painting progress bar (probably too
much information again, user just wants to know roughly how loaded a page is).
3) This does away with the spinning arrows, because they don't add anything
extra, and thus this suggestion doesn't use any extra space.
4) This also uses the traffic light colours to help convey the information as to
how loaded the page is.

Comment 4

[Asa Dotzler [:asa]]

this should be marked wontfix. we're not going to put a progress indicator in
each tab.  where's our UI czar?

Comment 5

[[e-mail]]

I could live with adding the minimum backend to allow a coloured spinning arrows
extension, i.e. it would be possible to install a small extension that would
override the current spinning arrows to colour-code the loading progress.

Comment 6

[Nick_Jenkins]

A minimal change to allow an extension that does progress indication sounds ideal.

The simplest interface I can conceive of is something that gets passed a
tab-load percentage, and that returns a tab-icon url, along these lines:

// Default implementation, just returns B&W spinning arrows
getLoadingTabIcon : function(percentage)
{
  return url("chrome://global/skin/icons/Loading.gif");
}

// versus an extension that shows colour-coded tab-load progress
getLoadingTabIcon : function(percentage)
{
   if (percentage <= 0)       return
url("chrome://global/skin/icons/Loading-1.gif");
   else if (percentage <= 20) return
url("chrome://global/skin/icons/Loading-2.gif");
   else if (percentage <= 40) return
url("chrome://global/skin/icons/Loading-3.gif");
   else if (percentage <= 60) return
url("chrome://global/skin/icons/Loading-4.gif");
   else if (percentage <= 80) return
url("chrome://global/skin/icons/Loading-5.gif");
   else                       return
url("chrome://global/skin/icons/Loading-6.gif");
}

Comment 7

[[e-mail]]

Heh, I was thinking of something simpler than that, instead of our current
busy="true" implementation, use busy="<percentage value 00-99>" then you can use
CSS to style the busy image e.g. tab[busy^="5"] would style 50-59%.

Comment 8

[Nick_Jenkins]

Ahh - now that's good - and definitely simpler!

So a load-progress extension could be done with CSS + a few icon images, but
when no such extension was installed then a loading tab would look the same as
it does at the moment (i.e. the current spinning arrows icon would match all
busy percentages).

Yes, that's much better.

Comment 9

[[e-mail]]

Attachment: Proposed patch

The changes to browser.css are needed for consistency with Modern.
The changes to tabbrowser.xml do the work that an extension can hook into.

Comment 10

[Nick_Jenkins]

Attachment: Zip file containing aome basic loading Gif icons + a modified userChrome.css

I'm trying to get the load progress icons to work, and not having much luck, so
I strongly suspect I'm doing something wrong. Here's what I've done:

I've made the patch changes to tabbrowser.xml (extracted toolkit.jar, modified,
and rezipped up back into toolkit.jar).

With the browser.css, I'm running Firefox 0.8, and I couldn't find an exact
match for the browser.css changes. (May be that you're running a more recent
build?). The closest match was in classic.jar, namely
classic\skin\classic\global\browser.css. The middle of this file contained
this:
=========
.tab-icon {
  margin-top: 1px;
  margin-right: 3px;
  width: 16px;
  height: 16px;
  list-style-image: url("chrome://global/skin/icons/folder-item.png");
  -moz-image-region: rect(0px, 16px, 16px, 0px);
}

tab[busy] > .tab-icon {
  list-style-image: url("chrome://global/skin/icons/Loading.gif");
}
========

So couldn't see anything that exactly matched this change (was close, but the
file path was different, and the '-moz-image-region' line was gone):
========
  margin-right: 3px;
   width: 16px;
   height: 16px;
+}
+
+tab {
   list-style-image:
url("chrome://communicator/skin/bookmarks/bookmark-item.gif"); 
 }
========

Consequently I didn't make this change.


I tried making this change:
========
-tab[busy] > .tab-icon {
+tab[busy] {
========

However restarting the browser with this resulted in no spinning-arrows icon
showing up when the page was loading. I wasn't sure whether this was good or
bad, but I figured it was a case of the user's custom CSS overriding the
inbuilt CSS, so figured it probably should work as per normal if there was no
user-customised CSS. Conseqently I undid this change, so overall there were no
changes to the browser.css file.

I then made some very basic 16x16 GIF icons, and a very basic userChrome.css.
I'm attaching a ZIP file containing these files. The icons I put into the
classic.jar file, under classic\skin\classic\global\icons, which is also where
the current spinning arrows icon (Loading.gif) is located.

When I restarted the browser though, it had the standard spinning arrows icon,
not the progressive loading icons, so I've done something wrong.

The tabbrowser.xml changes were straightforward, and the Gif icons seem fine,
so that leaves as the most probable mistakes I've made:
* the browser.css changes that I didn't make.
* the userChrome.css, where I'm making a best guess at what the correct syntax
should be.

Is there anything obviously wrong in what I'm doing? Any pointers would be most
appreciated.

Comment 11

[[e-mail]]

Attachment: Better patch

I noticed a bug because I don't enforce two digits :-/
Also I made it easier to customize, there was an issue with the last patch.
+}
+
+tab {
This bit goes after the height line.
Perhaps you were hit by bug 192022 which "disabled" the progress bar?

Comment 12

[[e-mail]]

Attachment: Sample userChome.css

I have this working in conjunction with attachment 145327 [details] [diff] [review].

Comment 13

[Nick_Jenkins]

Attachment: Screenshot of Firefox with tab load indicators

Superb! Thank you! I got it running using a nightly build of Firefox, and it
works great - screenshot is attached.

The only small thing I wondered about was setting 'value' to be equal to zero
when nothing has loaded. Currently it goes: spinning arrows, then the various
tab load icons, then the web page's icon. Basically I wondered if it would be
possible to override the spinning arrows altogether, and just have the
value^="0" case match when nothing has loaded. 

I'm guessing that the reason this doesn't happen currently is because
'onProgressChange' does not get called until some data get loaded, so it
doesn't get the chance to set the value to zero.

I tried doing this ~line 200 of tabbrowser.xml:
===============================================
		if (aWebProgress.DOMWindow == this.mBrowser.contentWindow)
		  this.mBrowser.userTypedValue = null;

		if (!this.mBlank) {
+		   this.mTab.setAttribute("value", "00");
		  this.mTab.setAttribute("busy", "true");
		  this.mTab.label =
this.mTabBrowser.mStringBundle.getString("tabs.loading");
===============================================

This seems to work OK, but my knowledge of Mozilla's internals is pretty scant,
so I'm not sure if I'm making that change in the right place, or if it will
have any unintended consequences.

Comment 14

[[e-mail]]

Perhaps this would also work for you?

.tabbrowser-tab[busy="true"] {
  list-style-image: url("Loading-00.gif") !important;
}

Comment 15

[Nick_Jenkins]

Yep, that works great, and I'd much rather have it in CSS than modify the code. 
Thank you.

It's all working great now, so that by default it looks exactly the same as 
before, or with some small additions to the userChrome the tab load indicators 
appear. Good stuff.

Hopefully the patch will be included in time for the next release.

Comment 16

[[e-mail]]

Well if you want it in FireFox your best bet is to file a new bug under that
component and attach your version of the patch there.

Comment 17

[[e-mail]]

Comment on attachment 145327 [details] [diff] [review]
Better patch

As for Mozilla, let's ask for moa first...

Comment 18

[jag (Peter Annema)]

Comment on attachment 145327 [details] [diff] [review]
Better patch

I like this idea. I'm not sure if "value" is the best attribute name for this
though. Why not call it "progress"?

Could this attribute (for a very short time) be set to "100", causing the rule
for 10% to match? There might be some flicker as it goes from 90% to 100% to
the site icon / bookmark icon.

Comment 19

[[e-mail]]

>+                this.mTab.setAttribute("value", ("0" +
Math.floor(aCurTotalProgress * 100 / aMaxTotalProgress)).slice(-2));

It occurred to me that the second digit is probably unnecessary anyway; how
about if I change the calculation to this.mTab.setAttribute("progress",
Math.floor(aCurTotalProgress * 9.9 / aMaxTotalProgress)); ?

Comment 20

[Nick_Jenkins]

Attachment: Sites that don't show their site icon once loaded

I found the best way to really test the corner cases of tab load indicators was
to load between 30 and 50 tabs at once, which causes everything to go much
slower by saturating my network link, and then leave it until everything
finished loading, and see what happens.

The update in the comment above is definitely better - without this it would
occasionally show the nothing-load-state for a tab that had finished loading -
I'm guessing that the progress value had come out as "100", and then I guess
that became "00" after the "slice(-2)" took effect, resulting in the icon
becoming the 0% nothing-is-loaded icon. That doesn't happen any more with the
change in the above comment.

There is something I noticed though in the process of this stress-testing:
There were a very small number of pages that seem to end up with their tab icon
stuck on the state just before the fully loaded state (i.e. matches the 90-99%
loaded case). They stay in this state, even after the page has completed fully
loading (i.e. the top right of the browser has stopped spinning for that page,
and all network activity has stopped).

I have a sneaking suspicion though that this may be a problem with some other
part of the browser, and not with this patch. In other words, a case of this
change exposing some other pre-existing problem, rather than causing the
problem. I say this for 2 reasons:
1) I couldn't get it to go away by adjusting the patch (such as not setting the
progress value if it was > 99%).
2) It seems to happen particularly on certain web pages, and other pages are
fine.

After testing several hundred bookmarks, I was able to make a short list of
some pages that tended to have this problem, namely:

http://h71000.www7.hp.com/wizard/wiz_5486.html
http://www.ibj.usyd.edu.au/
http://www.abc.net.au/science/k2/stn/default.htm
http://www.measureup.com/Cart/more_product_infoMUP.asp?id=478
http://www.askmecorp.com/Notfound.asp?404;http://www.askmecorp.com/BusinessSplash.asp

http://www.telusplanet.net/public/sparkman/netcalc.htm
http://www.pbs.org/wgbh/pages/frontline/shows/dotcon/historical/bubbles.html
http://www.jvc-australia.com/products/_products_sub_group_details.asp?ID=1083
http://www.maxtor.com/en/support/service/index.htm
http://websupport.wdc.com/websupport/clearexp_scripts/warrantystart.asp

I've attached an image to this comment of what it looks like when just these
problematic pages have finished loading. Should I maybe be filing a separate
bug report for this?

Comment 21

[Nick_Jenkins]

Doh! I take it back about it being unrelated - and it can be fixed with a very
small addition to the 'if' condition in the patch, like so:

======================
            {
+              if (aMaxTotalProgress > 0 && this.mTab.hasAttribute("busy")) 
+                this.mTab.setAttribute("progress", Math.floor(aCurTotalProgress
* 9.9 / aMaxTotalProgress)); 
              if (!this.mBlank && this.mTabBrowser.mCurrentTab == this.mTab) {
======================

With this, I couldn't get any pages to stuff up, even the ones listed above that
previously were reproducibly causing problems. 

Sorry for the confusion! 

Comment 22

[[e-mail]]

Comment on attachment 145327 [details] [diff] [review]
Better patch

>@@ -229,6 +231,7 @@
>                   this.mBlank = false;
> 
>                 this.mTab.removeAttribute("busy");
>+                this.mTab.removeAttribute("value");
> 
>                 var location = aRequest.QueryInterface(nsIChannel).URI;
>                 if (this.mIcon) {
Nick, I hope you changed this to say "progress" instead of "value"...

jag, assuming Nick did, then it might just be easier to require the extension
to style .tabbrowser-tab[busy="true"][progress="N"] and not bother about
removing the progress attribute?

Comment 23

[Nick_Jenkins]

> Nick, I hope you changed this to say "progress" instead of "value"...

Yes, I did.

I know that's weird though, because it suggests that "onProgressChange" gets 
called after busy=false, which I wouldn't expect in theory, but it seems to 
happen on some pages in practice.

> require the extension to style .tabbrowser-tab[busy="true"][progress="N"]

I've tried this, and it seems to works fine on all sites, including the ones 
that were causing problems, and it's also fine with heaps of tabs loading at 
once.

I.e.: userChrome.css contains lines for the various cases, like this one:
.tabbrowser-tab[busy="true"][progress="0"] {
  list-style-image: url("Loading-00.gif") !important;
}

And the tabbrowser.xml contains additions like so in the appropriate places:
+              if (aMaxTotalProgress > 0)
+                this.mTab.setAttribute("progress", Math.floor
(aCurTotalProgress * 9.9 / aMaxTotalProgress)); 
Plus:
+                this.mTab.removeAttribute("progress");
Plus:
+            t.className = "tabbrowser-tab";

And the browser.css changes as per standard.

With this setup it all works fine, no problems.

Comment 24

[[e-mail]]

>+                this.mTab.removeAttribute("progress");
Actually the point is that having the [busy="true"] in the CSS means that we
don't have to remove the progress attribtue any more.

Comment 25

[Nick_Jenkins]

Attachment: Screenshot with progress attribute not removed

> don't have to remove the progress attribute any more.

OK. I've just tested this though, and I'm sorry to say it doesn't work so well
if the progress attribute is not removed. 

The best way to see why is with the attached screenshot. To create it, I loaded
about 20 tab at once. I let that completely finish. Then I loaded about 40 tabs
at once, overwriting the previous 20 tabs, and soon as that started, I took a
screenshot. What happens is that the first 20 tabs still have their progress
value set to around 90% (because the attribute was not removed), so those first
20 tabs show as nearly fully loaded, and then they jump back down to 10 or 20%
loaded, as data starts to arrive for those tabs. The last 20 tabs start at 0%
progress, as expected.

Comment 26

[Nick_Jenkins]

Attachment: Screenshot of reloading fussy pages even *with* progress attribute removed.

Actually, I've just realised that something like the above happens with those
10 pages listed before that were still displaying the load indicators after
finishing loading. This can happens even if you *do* remove the progress
attribute, because there's something weird about those 10 pages that causes
their progress to get set again even *after* busy=false.

So, in this test scenario, you load the 10 fussy pages, and it looks normal.
Then clear the cache. Then load the same 10 pages again. Immediately take a
screenshot. They start at 90% loaded (image attached), because their progress
value got set again (after it was removed) when they were loading the first
time.

The easiest way to fix this seems to be to explicitly initialize progress to
zero when the tab starts loading. [The hard way is to work out why
'onProgressChange' gets called after busy=false, but that's way beyond the
scope of this bug.] Also, it's probably best to initialize progress before
setting busy=true as well, for good measure. I.e. make an additional
modification to 'onStateChange' like so:
==========================================================
		if (!this.mBlank) {
+		   this.mTab.setAttribute("progress", "0");		   
		  this.mTab.setAttribute("busy", "true");
		  this.mTab.label =
this.mTabBrowser.mStringBundle.getString("tabs.loading");
==========================================================

Note that this is a pretty nasty and non-obvious test case, which is why I
didn't find it before. Works fine now though if progress is initialized to
zero.

Comment 27

[[e-mail]]

Attachment: New approach

Ok, so again this is with the .tabbrowser[busy="true"][progress="N"] style, but
now this removes the progress attribute before setting busy="true", which is a
minor variation on Nick's idea because I want the spinning arrows while we're
trying to establish a connection. Nick can always change his CSS to
.tabbrowser-tab[busy="true"] { list-style-image: url(progress0.gif); }
if he never wants the spinning arrows.

Comment 28

[jag (Peter Annema)]

Comment on attachment 145734 [details] [diff] [review]
New approach

sr=jag

You know someone's gonna file a bug on the inaccurracy of that progress * 9.9
thing ;-)

Comment 29

[Nick_Jenkins]

Confirming that new approach patch works great, including for all previously
described test cases, and that no problems were encountered. Excellent work!

Comment 30

[Mike Connor [:mconnor]]

Comment on attachment 145734 [details] [diff] [review]
New approach

r=[e-mail]

Comment 31

[[e-mail]]

Fix checked in to seamonkey.

Comment 32

[Robin Monks]

*** Bug 271665 has been marked as a duplicate of this bug. ***