750054 - Update loading throbbers in secondary UI

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[u428464]

The old load indicator is still present in many areas of the browser (about window when searching an update, list all tab menu, etc.). It should be upgraded to the new one (the one used for page loading). I don't know if something new about that is planned for Australis, but it would be a nice first move.

Comment 1

[Tim Nguyen :ntim]

http://people.mozilla.org/~shorlander/mockups-interactive/australis-spinner-updates-i01/australis-spinner-updates-01.html

Comment 2

[Tim Nguyen :ntim]

2nd mockup : http://people.mozilla.org/~shorlander/mockups-interactive/australis-spinner-updates-i02/australis-spinner-updates-02.html

Comment 3

[u428464]

Note that the find bar also shows an old throbber.

Comment 4

[u428464]

Could this be named [qx] ?

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Yeah, I think it could be…
Can you make a list of all the places we need to update the throbber?

Comment 6

[u428464]

AFAIK there are at least :
-about window when you look for an update
-find bar when you look for a lot of occurrences
-add-on manager when you access "get add-ons"
-list all tabs menu when loading a tab.

Maybe there are more, but that's all the places I know about.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Part 1, v1 - move browser's throbber to toolkit

This is a minimal part 1 - moving the new throbber from browser into toolkit, replacing the old one. It keeps the existing inconsistent naming. That'll be fixed in a second patch (still working on that), along with fixing up usage of the HiDPI version throughout the tree.

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8722415 [details] [diff] [review]
Part 1, v1 - move browser's throbber to toolkit

Review of attachment 8722415 [details] [diff] [review]:
-----------------------------------------------------------------

This looks simple enough... I'd like to see the updated patch with the @2x version added.

Thanks!

::: browser/themes/osx/browser.css
@@ -2467,5 @@
>      list-style-image: url("chrome://browser/skin/tabbrowser/[e-mail]");
>    }
>  
>    .tab-throbber[progress] {
> -    list-style-image: url("chrome://browser/skin/tabbrowser/[e-mail]");

Oops, I didn't see the hg mv for this one!

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Part 2, v1 - rename throbber image, use HiDPI throbber everywhere

And Part 2. This renames the throbber images to be consistent with naming throughout the tree. And also updates *almost* all uses of the throbber to use the HiDPI version when appropriate - I've manually verified the HiDPI usage for every case (albeit only on Windows - but the changes there were the same as on OSX). And that ended up including fixing the odd case where even the normal non-HiDPI throbber was stretched.

Almost all, because:
* Not Linux, because we don't have a HiDPI version there and basically none of the Linux theme has any HiDPI support
* Not the social sidebar, due to other issues I discovered there (see bug 1252006)

Also of note:
* The Add-ons Manager's screenshot loading code needed some extra massaging, due to multiple backgrounds and wanting to keep it DRY

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8722415 [details] [diff] [review]
Part 1, v1 - move browser's throbber to toolkit

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> > -    list-style-image: url("chrome://browser/skin/tabbrowser/[e-mail]");
> 
> Oops, I didn't see the hg mv for this one!

Was confused about this for a sec.... it is there, but Bugzilla's diff/review tools don't show it :\ It's there when you look at the raw diff file (or apply it).

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8722415 [details] [diff] [review]
Part 1, v1 - move browser's throbber to toolkit

Review of attachment 8722415 [details] [diff] [review]:
-----------------------------------------------------------------

Seriously, when I do `open toolkit/themes/osx/global/icons/` on OSX and search for 'loading*.png', I only see 'loading_16.png' and 'loading_32.png', but in patch 2 you change everything to 'loading.png' and '[e-mail]' (as I'd expect!)...

So something is _not_ right here. Can you check it out again?

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8725558 [details] [diff] [review]
Part 2, v1 - rename throbber image, use HiDPI throbber everywhere

Review of attachment 8725558 [details] [diff] [review]:
-----------------------------------------------------------------

I like the changes I see here - but patch 1 is blocking things here ;-)

::: browser/themes/linux/syncSetup.css
@@ +114,5 @@
>  
> +#pairDeviceThrobber,
> +#login-throbber {
> +  -moz-box-align: center;
> +}

This doesn't really hurt, but why is this needed even if you set  `width: 16px` for the icon below?
Also, but this has nothing to do with this bug, I thought that we normally set the list-image on the parent element, instead of the image directly... might be related, or not ;-)

::: browser/themes/osx/browser.css
@@ +2884,5 @@
>  @media (min-resolution: 2dppx) {
>    .alltabs-item > .menu-iconic-left > .menu-iconic-icon {
>      list-style-image: url("chrome://mozapps/skin/places/[e-mail]");
>    }
> +  

nit: whitespace.

::: browser/themes/osx/syncSetup.css
@@ +112,5 @@
>  }
>  
> +#pairDeviceThrobber,
> +#login-throbber {
> +  -moz-box-align: center;

Same question as the Linux one, really.

::: browser/themes/windows/syncSetup.css
@@ +113,5 @@
>  }
>  
> +#pairDeviceThrobber,
> +#login-throbber {
> +  -moz-box-align: center;

Same question as the Linux one, really.

::: toolkit/themes/shared/extensions/extensions.inc.css
@@ +699,5 @@
>  }
>  
>  #detail-screenshot-box {
>    -moz-margin-end: 2em;
> +  background-image: linear-gradient(rgba(255, 255, 255, 0.5), transparent);

nit: `rgba(255,255,255,.5)`

Comment 13

[Mike de Boer [:mikedeboer]]

Blair, this delayed by two days - my apologies. There's this focus on e10s for some reason ;-) If you're bored, you can always move the Addon-manager to the content process!

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

No worries, Mike (thanks for the review) - I'm concentrating on e10s stuff myself now too. Will circle back around to this when I can, to finish it off.

Comment 15

[Justin Dolske [:Dolske]]

(In reply to Mike de Boer [:mikedeboer] from comment #11)
> Comment on attachment 8722415 [details] [diff] [review]
> Part 1, v1 - move browser's throbber to toolkit
> 
> Review of attachment 8722415 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seriously, when I do `open toolkit/themes/osx/global/icons/` on OSX and
> search for 'loading*.png', I only see 'loading_16.png' and 'loading_32.png',
> but in patch 2 you change everything to 'loading.png' and '[e-mail]'
> (as I'd expect!)...
> 
> So something is _not_ right here. Can you check it out again?

Hmm, seems ok to me?

(starting with a plain m-c tree)
$ ls -l browser/themes/osx/tabbrowser/load*
-rw-r--r--  1 dolske  staff  12585 Feb 21  2015 browser/themes/osx/tabbrowser/loading.png
-rw-r--r--  1 dolske  staff  40165 Feb 21  2015 browser/themes/osx/tabbrowser/[e-mail]
$ ls -l toolkit/themes/osx/global/icons/load*
-rw-r--r--  1 dolske  staff  9025 Sep  5  2015 toolkit/themes/osx/global/icons/loading_16.png

$ hg qpush
applying 750054-p1
now at: 750054-p1

$ ls -l browser/themes/osx/tabbrowser/load*
ls: browser/themes/osx/tabbrowser/load*: No such file or directory
$ ls -l toolkit/themes/osx/global/icons/load*
-rw-r--r--  1 dolske  staff  12585 Mar  7 15:49 toolkit/themes/osx/global/icons/loading_16.png
-rw-r--r--  1 dolske  staff  40165 Mar  7 15:49 toolkit/themes/osx/global/icons/loading_32.png

$ hg qpush
applying 750054-p2
now at: 750054-p2

$ ls -l browser/themes/osx/tabbrowser/load*
ls: browser/themes/osx/tabbrowser/load*: No such file or directory
$ ls -l toolkit/themes/osx/global/icons/load*
-rw-r--r--  1 dolske  staff  12585 Mar  7 16:35 toolkit/themes/osx/global/icons/loading.png
-rw-r--r--  1 dolske  staff  40165 Mar  7 16:35 toolkit/themes/osx/global/icons/[e-mail]

Part 1 seems to just be putting the files into /toolkit (with the _16/_32 naming), and Part 2 is renaming them to drop the _16/_32 in favor of the @2x format?

Would it make sense to fold these patches for landing? Otherwise I can see it being a bit weird to be renaming "@2x" references to "_32" (or not, and having it be broken), only to rename "_32" to _@2x" again with part 2. Maybe that's what Mike is talking about?

Comment 16

[Justin Dolske [:Dolske]]

Comment on attachment 8722415 [details] [diff] [review]
Part 1, v1 - move browser's throbber to toolkit

The main issue for the r- seems ok to me (comment 15). I don't see any other significant issues in the previous review, and LGTM.

I suppose we'll want to flag this for addon compat? It's changing the location of a fairly commonly used theme image, but it's pretty trivial to fix.

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mike de Boer [:mikedeboer] from comment #12)
> ::: browser/themes/linux/syncSetup.css
> @@ +114,5 @@
> >  
> > +#pairDeviceThrobber,
> > +#login-throbber {
> > +  -moz-box-align: center;
> > +}
> 
> This doesn't really hurt, but why is this needed even if you set  `width:
> 16px` for the icon below?
> Also, but this has nothing to do with this bug, I thought that we normally
> set the list-image on the parent element, instead of the image directly...
> might be related, or not ;-)

Just verified this is indeed needed, although it's an existing bug. That CSS file is used in 3 places, but using setup.xul as an example: The throbber is a sibling of a text node (xul:label), which forces the parent box (#login-throbber) to have a height greater than 16px. And the parent box by default stretches its children to fit (ie, the default is -moz-box-align:stretch), which overrides any explicitly set height value. So without that, we get a noticeably stretched throbber (19.5px in HiDPI on Windows for me). And setting list-style-image directly on the xul:image vs the parent is unrelated (it's just a shorthand).

Comment 18

[Mike de Boer [:mikedeboer]]

(In reply to Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) from comment #17)
> Just verified this is indeed needed, although it's an existing bug. That CSS
> file is used in 3 places, but using setup.xul as an example: The throbber is
> a sibling of a text node (xul:label), which forces the parent box
> (#login-throbber) to have a height greater than 16px. And the parent box by
> default stretches its children to fit (ie, the default is
> -moz-box-align:stretch), which overrides any explicitly set height value. So
> without that, we get a noticeably stretched throbber (19.5px in HiDPI on
> Windows for me). And setting list-style-image directly on the xul:image vs
> the parent is unrelated (it's just a shorthand).

OK! Thanks for investigating this... maybe you'd like to document this in-situ when landing?

Comment 19

[Mike de Boer [:mikedeboer]]

(In reply to Justin Dolske [:Dolske] from comment #16)
> Comment on attachment 8722415 [details] [diff] [review]
> Part 1, v1 - move browser's throbber to toolkit
> 
> The main issue for the r- seems ok to me (comment 15). I don't see any other
> significant issues in the previous review, and LGTM.
> 
> I suppose we'll want to flag this for addon compat? It's changing the
> location of a fairly commonly used theme image, but it's pretty trivial to
> fix.

It was me after all - I think landing a folded patch is a good idea, Blair, because that's what fooled me.

r=me as well, if you'll take it.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c55260f51835

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mike de Boer [:mikedeboer] from comment #18)
> OK! Thanks for investigating this... maybe you'd like to document this
> in-situ when landing?

Oh, didn't see this... changeset blame will point back here anyway, I guess.

> I think landing a folded patch is a good idea

Done :)

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

And yep, a little addon-compat impact here... 

This changes the URL for the loading throbber animated image. Previously you could use chrome://global/skin/icons/loading_16.png (old style) or chrome://browser/skin/tabbrowser/loading.png (newer style used in tabs). Only the newer style is available now, at chrome://global/skin/icons/loading.png and chrome://global/skin/icons/[e-mail] (HiDPI version).

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c55260f51835