Update loading throbbers in secondary UI

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: ge3k0s, Assigned: Unfocused)

Tracking

(Depends on 1 bug, {addon-compat})

unspecified
Firefox 48
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: p=0 [qx:spec][qx])

Attachments

(2 attachments)

Reporter

Description

7 years ago
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.
Reporter

Updated

7 years ago
Summary: Upgrade load indicators → Upgrade loading indicators
Reporter

Updated

7 years ago
Summary: Upgrade loading indicators → Upgrade loading throbbers
Reporter

Updated

7 years ago
Keywords: uiwanted
Reporter

Updated

7 years ago
Summary: Upgrade loading throbbers → Upgrade loading throbbers in secondary UI

Updated

7 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: uiwanted
Summary: Upgrade loading throbbers in secondary UI → Update loading throbbers in secondary UI
Reporter

Updated

6 years ago
Blocks: 958480
No longer blocks: 958480
Depends on: 958480
Reporter

Updated

6 years ago
Flags: firefox-backlog? → firefox-backlog+
Reporter

Comment 3

5 years ago
Note that the find bar also shows an old throbber.
No longer blocks: fxdesktopbacklog
Whiteboard: [feature] p=0 → p=0
Reporter

Updated

5 years ago
Depends on: 994954
Reporter

Updated

4 years ago
Whiteboard: p=0 → p=0, [qx]
Reporter

Updated

4 years ago
Whiteboard: p=0, [qx] → p=0
Reporter

Comment 4

4 years ago
Could this be named [qx] ?
Flags: needinfo?(bwinton)
Yeah, I think it could be…
Can you make a list of all the places we need to update the throbber?
Flags: needinfo?(bwinton)
Whiteboard: p=0 → p=0 [qx]
Reporter

Comment 6

4 years ago
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.
Whiteboard: p=0 [qx] → p=0 [qx:spec]
See Also: → 1173612
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Reporter

Updated

3 years ago
Whiteboard: p=0 [qx:spec] → p=0 [qx:spec][qx]
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.
Attachment #8722415 - Flags: review?(mdeboer)
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/connecting@2x.png");
>    }
>  
>    .tab-throbber[progress] {
> -    list-style-image: url("chrome://browser/skin/tabbrowser/loading@2x.png");

Oops, I didn't see the hg mv for this one!
Attachment #8722415 - Flags: review?(mdeboer)
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
Attachment #8725558 - Flags: review?(mdeboer)
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/loading@2x.png");
> 
> 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).
Attachment #8722415 - Flags: review?(mdeboer)
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 'loading@2x.png' (as I'd expect!)...

So something is _not_ right here. Can you check it out again?
Attachment #8722415 - Flags: review?(mdeboer) → review-
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/defaultFavicon@2x.png");
>    }
> +  

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)`
Attachment #8725558 - Flags: review?(mdeboer) → review+
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!
Flags: needinfo?(bmcbride)
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.
Flags: needinfo?(bmcbride)
(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 'loading@2x.png'
> (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/loading@2x.png
$ 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/loading@2x.png

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 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.
Attachment #8722415 - Flags: review- → review+
(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).
(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?
(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.
(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 :)
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/loading@2x.png (HiDPI version).
Keywords: addon-compat

Comment 23

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c55260f51835
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Depends on: 1258512
Depends on: 1278662

Updated

3 years ago
Duplicate of this bug: 1008902
Duplicate of this bug: 1217034
You need to log in before you can comment on or make changes to this bug.