Alex, beltzner, and mconnor designed this, I'm summarizing: * Remove the throbber from its current location on the toolbar. * Add throbber to where the favicon is in the URL bar. * The throbber, like the favicon and the URL, is tied to the tab being shown. * While the page is loading, you see the throbber, and when it finishes you get the favicon. [Just like how the throbber/icon on tabs currently works.] Why: * Brings load activity closer to the UI that affects throbbing (eg Stop, enter a URL, forward/back/reload). * A throbber here is useful for indicating activity as a Places search runs (as you're typing in the field) * One less icon in the primary UI (favicon is now overloaded, no standalone throbber) Known nits: * The "inactive throbber" state ceases to exist. [More precisely, a favicon is shown when the throbber is inactive.] This is highly desirable for integration consistency on OS X. * The throbber is no longer global, so if the only tab loading is hidden in tab overflow, there's no at-a-glance activity indication. * The favicon can load before the page is finished loading, but won't be shown until the page finishes loading and the throbber stops. Stalled loads would be the worst example of this. * This could be perceived as a flashing effect, where navigating within a site causes icon --> throbber --> icon --> throbber --> icon, etc. We already flash a little between the site icon and generic icon, although it can be subtle.

Rough cut for pinstripe (OS X). This probably still needs some work on the stack/desk interaction, e.g. if the dragable stuff is expected to work while throbbing. It might make sense to eliminate the deck entirely and toggle CSS visibility instead, although that's a bit more work.

Is this something we can take this late?

>Is this something we can take this late? We are currently working under the impression that a variety of theme changes have been scheduled to land between beta 1 and beta 2. Perhaps mconnor or beltzner should chime in so that we are all on the same page regarding if these UI improvements are acceptable at this stage of our development cycle.

I know we scheduled native theming, e.g. the sidebar stuff you've worked on. Perhaps best to explain somewhere outside of bugzilla.

(In reply to comment #0) > * The throbber is no longer global As far as I know, it has never been global.

Sorry, I meant "global to the window".

That's what I meant, too. The throbber is only spinning for the active tab.

>That's what I meant, too. The throbber is only spinning for the active tab. It looks like this change will clear up that conceptual problem as well (I also actually thought it was global to the window).

Huh, I don't know what the heck I was thinking! So, yeah, the toolbar throbber is tab-specific, which means moving it to the URL field is mostly just a positional change. (The slight behavioral change being that the idle state is now the favicon, instead of a static throb frame.)

>Huh, I don't know what the heck I was thinking! blame the interface, not the user :)

Wanted, but not blocking.

I damn hope it will be possible to undo this so-called "enhancement" by means of a theme or an extension.

I still need to test this on Windows, but it looks right on Linux and OS X.

Comment on attachment 291345 [details] [diff] [review] Patch for review, v.1 please try gavin/dao/mconnor first.

Comment on attachment 291345 [details] [diff] [review] Patch for review, v.1 switching to beltzner since I lack ui-rness :)

Comment on attachment 291345 [details] [diff] [review] Patch for review, v.1 With this patch, throbberItem.title should be removed from browser.dtd. >Index: browser/themes/winstripe/browser/browser.css >+#navigator-throbber { >+ list-style-image: url("chrome://global/skin/throbber/Throbber-small.gif"); >+ display: none; >+} >+ >+#navigator-throbber[busy="true"] { >+ background-color: -moz-dialog; >+ display: block; >+} The background of the parent node (#identity-box) isn't solid on Windows, and I think it's not going to be solid on Mac. So that's not going to look properly. But I'll leave that to beltzner. Even if it would look right, I'd change the CSS rules like this: #navigator-throbber { list-style-image: url("chrome://global/skin/throbber/Throbber-small.gif"); background-color: -moz-dialog; } #navigator-throbber:not([busy="true"]) { visibility: hidden; }

>The background of the parent node (#identity-box) isn't solid on Windows, and I >think it's not going to be solid on Mac. >So that's not going to look properly. The plan is to eventually use a new apng image that works well on the site button's background.

(In reply to comment #16) > The background of the parent node (#identity-box) isn't solid on Windows, and I > think it's not going to be solid on Mac. > So that's not going to look properly. But I'll leave that to beltzner. I'm not sure I understand why that's a problem, unless it's just the APNG vs GIF blending Alex mentioned. > #navigator-throbber:not([busy="true"]) { > visibility: hidden; > } Unfortunately the animation still seems plays with |visibility: hidden|; there's a bit of CPU usage being used when I did it that way, whereas with |display: none| it was fully idle.

(In reply to comment #18) > (In reply to comment #16) > > The background of the parent node (#identity-box) isn't solid on Windows, and I > > think it's not going to be solid on Mac. > > So that's not going to look properly. But I'll leave that to beltzner. > I'm not sure I understand why that's a problem, unless it's just the APNG vs > GIF blending Alex mentioned. I guess you've added the -moz-field background color to overlay the favicon, but it will also overlay the appearance of the identity box -- which can be a gradient, for instance. Otherwise I don't understand the point of background-color: -moz-dialog;. Note that this is based on reading the patch, I haven't tested it yet. How about making the throbber the first child of the stack and using his rule: #navigator-throbber:not([busy="true"]) , #navigator-throbber[busy="true"] + #page-proxy-favicon { visibility: hidden; } > > #navigator-throbber:not([busy="true"]) { > > visibility: hidden; > > } > Unfortunately the animation still seems plays with |visibility: hidden|; > there's a bit of CPU usage being used when I did it that way, whereas with > |display: none| it was fully idle. OK.

(In reply to comment #19) > How about making the throbber the first child of the stack and using his rule: > > #navigator-throbber:not([busy="true"]) , > #navigator-throbber[busy="true"] + #page-proxy-favicon { > visibility: hidden; > } err, display: none

(In reply to comment #19) > I guess you've added the -moz-field background color to overlay the favicon, > but it will also overlay the appearance of the identity box -- which can be a > gradient, for instance. The identity-box wraps the page-proxy-stack, so the appearance is the same. Indeed, the #identity-box rule is just a few lines down, and I'm setting the background-color as it is. > How about making the throbber the first child of the stack and using his rule: > > #navigator-throbber:not([busy="true"]) , > #navigator-throbber[busy="true"] + #page-proxy-favicon { > visibility: hidden; > } Yeah, that should work too. One catch is that Alex wanted to be able to have a fade effect when the throbber starts/stops, and that would require the ordering of the <stack> as I have it. Although, hmmm, I suppose that could be done either way. In any case, it doesn't need to be a design requirement here.

(In reply to comment #21) > The identity-box wraps the page-proxy-stack, so the appearance is the same. > Indeed, the #identity-box rule is just a few lines down, and I'm setting the > background-color as it is. The background-color for #identity-box is effectless if -moz-appearance:toolbox kicks in, which is the case for most non-Classic Windows themes. > One catch is that Alex wanted to be able to have a fade effect when the > throbber starts/stops, and that would require the ordering of the <stack> as I > have it. Although, hmmm, I suppose that could be done either way. In any case, > it doesn't need to be a design requirement here. That should really work either way, I think.

Comment on attachment 291345 [details] [diff] [review] Patch for review, v.1 I'm not a peer so this doesn't mean much, but I'm minusing this now because of the background color, even though this is probably more a UI than a code issue, and because you removed throbberItem.title from the markup but not from the dtd. Otherwise I think this patch is OK. Theoretically, you really want a <deck> rather than a <stack>, but I like how you can hide / display the elements with pure CSS. A stack also allows better fading, in case we want to add that later.

A general note on the UI: (In reply to comment #0) > Why: ... > * A throbber here is useful for indicating activity as a Places search runs (as > you're typing in the field) This sounds like the user could confuse Places search with content activity. But in any case, I don't think fetching autocomplete results takes or should ever take long enough to justify an indicator. > * One less icon in the primary UI (favicon is now overloaded, no standalone > throbber) That doesn't seem to really simplify the UI, as the "annoying movement" is happening in a more prominent place.

>This sounds like the user could confuse Places search with content activity. To clarify, we will only indicate both firefox activity and network activity if we have a two variable throbber. That will need to be a separate bug, and I'm not sure if we will be able to pull it off in time for this release. Otherwise, searching the location bar will provide feedback in the first result if nothing has been returned yet. >I don't think fetching autocomplete results takes or should >ever take long enough to justify an indicator. There are a lot of cases where the search may take longer than 50-100ms (especially if the user has increased the size of their history), so we really should be providing some type of feedback to the user. I've personally found myself wondering if the search was still progressing or if nothing was going to be returned before. (as a another separate issue we need to say if no results were returned).

(In reply to comment #23) > Created an attachment (id=291561) [details] > WinXP screenshot Note that the background color is also wrong when #identity-box is green for EV certs (currently only in Pinstripe, bug 406321).

Updated with review comments. Checked on Windows/Linux/OS X. Tryserver builds with this patch are at https://build.mozilla.org/tryserver-builds/2007-12-06_17:08-jdolske@mozilla.com-1196989652/

Oh, I should have noted... On the Vista screenshot, the top 3 images are on the current trunk nightly (ie, without this patch). The bottom 4 images are with this patch.

Comment on attachment 292172 [details] [diff] [review] Patch for review, v.2 >- <toolbaritem id="throbber-box" title="&throbberItem.title;" align="center" pack="center"> You still didn't remove throbberItem.title from browser.dtd. > function PageProxyClearIcon () > { >- if (gProxyDeck.selectedIndex != 0) >- gProxyDeck.selectedIndex = 0; > if (gProxyFavIcon.hasAttribute("src")) > gProxyFavIcon.removeAttribute("src"); > } While you're at it, please remove the third line. >+#navigator-throbber:not([busy="true"]), >+#navigator-throbber[busy="true"] + #page-proxy-favicon { >+ display: none; >+} This should go into browser/base/content/browser.css (which I should have said earlier). From winstripe and gnomestripe: > #page-proxy-favicon:not([src]) { > list-style-image: url("chrome://global/skin/icons/folder-item.png") !important; > -moz-image-region: rect(0px, 16px, 16px, 0px); > } > > #page-proxy-favicon[pageproxystate="invalid"] { > list-style-image: url("chrome://global/skin/icons/folder-item.png") !important; > -moz-image-region: rect(32px, 16px, 48px, 0px) !important; > } #page-proxy-favicon doesn't have a src attribute if pageproxystate is "invalid", and the !important flags aren't needed. So instead, use this: > #page-proxy-favicon:not([src]) { > list-style-image: url("chrome://global/skin/icons/folder-item.png"); > -moz-image-region: rect(0px, 16px, 16px, 0px); > } > > #page-proxy-favicon[pageproxystate="invalid"] { > -moz-image-region: rect(32px, 16px, 48px, 0px); > } r=me with that fixed, but I'm still not a peer.

(In reply to comment #31) > >+#navigator-throbber:not([busy="true"]), > >+#navigator-throbber[busy="true"] + #page-proxy-favicon { > >+ display: none; > >+} > > This should go into browser/base/content/browser.css (which I should have said > earlier). What's the rule-of-thumb for putting things into that file, vs the per-theme file? If all the common rules should go there, should a bug be filed on that -- it looks like there's already a lot of duplication.

(In reply to comment #32) > What's the rule-of-thumb for putting things into that file, vs the per-theme > file? If all the common rules should go there, should a bug be filed on that -- > it looks like there's already a lot of duplication. Basically anything that makes it difficult for the themeing community to make third-party themes for Firefox 3, anything prevents them from themeing certain widgets such as the URL bar, or that raises their collective blood pressure should go into browser/base/content/browser.css.

(In reply to comment #32) > What's the rule-of-thumb for putting things into that file, vs the per-theme > file? If all the common rules should go there, should a bug be filed on that -- > it looks like there's already a lot of duplication. The general rules is that behavioral rules belong in /content, presentational rules in the appropriate /themes files. The rules in /themes aren't used when you install a third-party theme, but the rules in /content are.

(In reply to comment #31) > r=me with that fixed, but I'm still not a peer. My reading of comment #14 was that Mano wanted someone else to make a first-pass review. That's done, so now I'll request the official Mano-Seal-Of-Approval.

[Hmm, apparently I can't count. There's no "Patch v.3"]

Same as v.3, with recent merge conflicts fixed. Mano, can you get to this soon?

Comment on attachment 293056 [details] [diff] [review] Patch for review, v.4 I find that this actually makes pageloads feel a little slower, as the throbber is more in your face and you can see exactly how long it's taking. I'd prefer that we switch as soon as the page starts to render, perhaps even doing a nice crossfade from the throbber to the favicon for extra slickness. (leave the throbber on the tab, though, until the entire page loads)

So, I'm thinking that the implication of beltzner's comment 38 (which I agree with) is that we not make this change. At least, not as-is. I did a quick hack to do the throbbing-->not-throbbing transition when the favicon is available. This seemed to help with having the throbber run less, but creates a new problem... The throbber in the URL bar stops throbbing before the throbber on the tab, which is really confusing -- is the page loading, or not? I think any transition point before onload is going to be perceived as randomly stopping the throbber before the page has loaded. How to proceed? 1) Do nothing, ship with throbber as-is. An option, but I do like the cleaner UI without the dedicated throbber element. 2) Somehow make the throbber-in-urlbar less, well, in-your-face. I don't have a good proposal for this, but a conceptual ideas would be to overlay the favicon with a small throbber (ala Gnome icon emblems), an animated outline, something translucent, etc. 16x16 is a tight space to work in, though. 3) Get rid of the existing throbber, but leave the favicon area in the urlbar alone. I'm actually leaning in favor of #3. We've already got a throbber on the tab, and bug 392870 seeks to always show the tab bar by default. For cases where we don't show a tabbar, bug 400398 would be a clean solution. [Indeed, #3 is just implementing the idea in 400398.]

I'd be happy with option #3, fwiw.

I'm leaning towards #3 as well. This is because I would like to use a slightly different throbber on the site button to indicate that you are searching the awesome bar (the current lack of feedback can be very confusing). This would be similar to the position of the throbber in spotlight, and we shouldn't mix the meaning of this throbber to cover both searching and loading.

WONTFIX per above, reopened bug 400398 for option #3.