1019186 - DevTools - Get new throbbers for devtools UI

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Tim Nguyen :ntim]

It'd be nice to get new a new loading throbber for the devtools UI.
The current one we use doesn't fit into the devtools theme, and exists since Firefox 1.

It'd be nice to get a fresh, unicolored icon.

Comment 1

[Tim Nguyen :ntim]

Also, we don't have the current icon in HDPI format. It'd be nice to also have that.

Comment 2

[Brian Grinstead [:bgrins]]

I've seen these: https://people.mozilla.org/~shorlander/mockups-interactive/australis-spinner-updates-i02/australis-spinner-updates-02.html.  Possibly one of the blue ones would look good with dark/light devtools themes.

We could try these and see how they look:

https://people.mozilla.org/~shorlander/mockups-interactive/australis-spinner-updates-i02/images/loading-04.png
https://people.mozilla.org/~shorlander/mockups-interactive/australis-spinner-updates-i02/images/[e-mail]

No matter what, we are going to need a 2x version of some kind of spinner to provide for hidpi.  Stephen, could you provide a loading indicator in 1x and 2x versions that we can use?

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> I've seen these:
> https://people.mozilla.org/~shorlander/mockups-interactive/australis-spinner-
> updates-i02/australis-spinner-updates-02.html.  Possibly one of the blue
> ones would look good with dark/light devtools themes.
> 
> We could try these and see how they look:
> 
> https://people.mozilla.org/~shorlander/mockups-interactive/australis-spinner-
> updates-i02/images/loading-04.png
> https://people.mozilla.org/~shorlander/mockups-interactive/australis-spinner-
> updates-i02/images/[e-mail]
> 
> No matter what, we are going to need a 2x version of some kind of spinner to
> provide for hidpi.  Stephen, could you provide a loading indicator in 1x and
> 2x versions that we can use?

Meant to needinfo shorlander here.  Should we be using the spinners from this URL for devtools loading indicators or something else?

Comment 4

[Tim Nguyen :ntim]

Attachment: Patch

Comment 5

[Tim Nguyen :ntim]

I used the throbbers in Comment 2. They look fine on light theme, but they look pretty weird on dark theme.

Comment 6

[Tim Nguyen :ntim]

I'm been thinking of using CSS only throbbers, kinda like this : http://jsfiddle.net/ntim/cubd7g5t/
They integrate well on both themes, and scale at all sizes.
What do you think ?

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #6)
> I'm been thinking of using CSS only throbbers, kinda like this :
> http://jsfiddle.net/ntim/cubd7g5t/
> They integrate well on both themes, and scale at all sizes.
> What do you think ?

Sounds good to me as long as it works well with XUL and HTML elements - that should handle theming and hidpi well.  We should ui-review? shorlander once you have a patch (and a try push).

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> (In reply to Tim Nguyen [:ntim] from comment #6)
> > I'm been thinking of using CSS only throbbers, kinda like this :
> > http://jsfiddle.net/ntim/cubd7g5t/
> > They integrate well on both themes, and scale at all sizes.
> > What do you think ?
> 
> Sounds good to me as long as it works well with XUL and HTML elements - that
> should handle theming and hidpi well.  We should ui-review? shorlander once
> you have a patch (and a try push).

How easy will it be to reuse the CSS throbber though?  I wouldn't want to have to copy 10-20 lines of CSS across every time we wanted to use one, and I notice that some of the existing usages are in ::before elements, so I'm not sure how easy it would be to just add a .throbber class to things (maybe if we do some markup changes that would be possible).

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8539799 [details] [diff] [review]
Patch

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

The code changes look good.  Let's either proceed with this and get a ui review from shorlander (specifically making sure it is OK with the dark theme), or go forward with the proposal in Comment 6 (assuming it doesn't result in too much duplicated CSS).

Comment 10

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to Brian Grinstead [:bgrins] from comment #7)
> > (In reply to Tim Nguyen [:ntim] from comment #6)
> > > I'm been thinking of using CSS only throbbers, kinda like this :
> > > http://jsfiddle.net/ntim/cubd7g5t/
> > > They integrate well on both themes, and scale at all sizes.
> > > What do you think ?
> > 
> > Sounds good to me as long as it works well with XUL and HTML elements - that
> > should handle theming and hidpi well.  We should ui-review? shorlander once
> > you have a patch (and a try push).
> 
> How easy will it be to reuse the CSS throbber though?  I wouldn't want to
> have to copy 10-20 lines of CSS across every time we wanted to use one, and
> I notice that some of the existing usages are in ::before elements, so I'm
> not sure how easy it would be to just add a .throbber class to things (maybe
> if we do some markup changes that would be possible).

I could move the throbber to ::before, and introduce a .throbber class that would add a throbber before any element where the class is applied : http://jsfiddle.net/ntim/cubd7g5t/

Comment 11

[Tim Nguyen :ntim]

Attachment: Patch v2

Try push : https://tbpl.mozilla.org/?tree=Try&rev=658509cdb13c
(The jemalloc patch applied on the try build is just to work around Bug 1112566 )
---

This uses the CSS throbber demonstrated in previous comments.

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: throbbers-1.png

Screenshot of the the throbbers mid-spin.  Stephen, the easiest way to check this out is to user the Web Audio panel on pretty much any page, then press the reload button.  It will spin forever while it waits for a web audio context to be created.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8540347 [details] [diff] [review]
Patch v2

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

Mostly just a few nits, this is really good progress.  Main thing I'd like to figure out is how the canvas debugger throbber looks with and without the special margin set by the 'saving' attribute.  If it's reasonable to remove that margin, I'd like to see the saving attribute removed in both JS and CSS.

::: browser/themes/shared/devtools/canvasdebugger.inc.css
@@ -128,1 @@
>    margin-top: -2px;

I wonder if we need to keep the [saving] attribute around, just for this margin-top.  Is that lining anything up in particular, or could we just get rid of the attribute altogether?  I'm not really able to test this part at the moment, for some reason I'm having trouble getting this state to show up in the canvas debugger panel

::: browser/themes/shared/devtools/splitview.css
@@ +10,5 @@
>  %define smw_itemDarkBottomBorder rgba(128,128,128,0.15)
>  %define smw_itemLightTopBorder rgba(128,128,128,0.15)
>  %define smw_itemLightBottomBorder transparent
>  
> +.devtools-throbber {

I think we should prefix this with `.splitview-nav-container` just to be clear that it's only intentionally applying to this one element

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +870,5 @@
> +  vertical-align: bottom;
> +  -moz-margin-end: 0.5em;
> +  width: 1em;
> +  height: 1em;
> +  border: 2px solid currentColor;

Nice usage of currentColor to make sure it will fit in with any text.  We could also use var(--theme-body-color) but it's probably just as good this way.

@@ +873,5 @@
> +  height: 1em;
> +  border: 2px solid currentColor;
> +  border-right-color: transparent;
> +  border-radius: 50%;
> +  animation: 1.1s linear spin infinite;

It feels a little slow to me, but that is more of a UI review thing.  I'd suggest just a bit faster though, I think it makes the UI feel for snappy

@@ +876,5 @@
> +  border-radius: 50%;
> +  animation: 1.1s linear spin infinite;
> +}
> +
> +@keyframes spin {

Let's name this something more specific, maybe throbber-spin

Comment 14

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #13)
> ::: browser/themes/shared/devtools/canvasdebugger.inc.css
> @@ -128,1 @@
> >    margin-top: -2px;
> 
> I wonder if we need to keep the [saving] attribute around, just for this
> margin-top.  Is that lining anything up in particular, or could we just get
> rid of the attribute altogether?  I'm not really able to test this part at
> the moment, for some reason I'm having trouble getting this state to show up
> in the canvas debugger panel
Will see if I can remove that. It might break other things (tests mainly) if I remove it, vporof knows best about this.

> ::: browser/themes/shared/devtools/splitview.css
> @@ +10,5 @@
> >  %define smw_itemDarkBottomBorder rgba(128,128,128,0.15)
> >  %define smw_itemLightTopBorder rgba(128,128,128,0.15)
> >  %define smw_itemLightBottomBorder transparent
> >  
> > +.devtools-throbber {
> 
> I think we should prefix this with `.splitview-nav-container` just to be
> clear that it's only intentionally applying to this one element
Will do.

> @@ +873,5 @@
> > +  height: 1em;
> > +  border: 2px solid currentColor;
> > +  border-right-color: transparent;
> > +  border-radius: 50%;
> > +  animation: 1.1s linear spin infinite;
> 
> It feels a little slow to me, but that is more of a UI review thing.  I'd
> suggest just a bit faster though, I think it makes the UI feel for snappy
Actually it feels the same (and even a bit faster) than the ones in comment 2.

> @@ +876,5 @@
> > +  border-radius: 50%;
> > +  animation: 1.1s linear spin infinite;
> > +}
> > +
> > +@keyframes spin {
> 
> Let's name this something more specific, maybe throbber-spin

Will do.

Comment 15

[Brian Grinstead [:bgrins]]

Victor, I have a question about the "saving" attribute that gets set on the footer element in canvas debugger - is that purely for the loading spinner styling?

The patch here is adding a class specific for the spinner, so we can just add/remove that instead and get rid of the reference to the [saving] attribute in JS/CSS, unless if used for something else that I'm missing.

Comment 16

[Victor Porof [:vporof][:vp]]

The saving attribute is just for showing the spinner, IIRC. You can probably safely switch to the new class and remove the old attribute.

Comment 17

[Tim Nguyen :ntim]

Attachment: Patch v3

Fixed nits.

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8540989 [details] [diff] [review]
Patch v3

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

Code changes look good, awaiting a UI review.  Tim, are you able to push this to try so there can be a binary for UI review?

Comment 19

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #18)
> Comment on attachment 8540989 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 8540989 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Code changes look good, awaiting a UI review.  Tim, are you able to push
> this to try so there can be a binary for UI review?

For UI review, please check the try push from comment 11.

Comment 20

[Stephen Horlander [:shorlander]]

Comment on attachment 8540989 [details] [diff] [review]
Patch v3

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

Looks good to me. My only feedback is that it looks a little jaggy/fuzzy on normal DPI screens. Which seems to be a problem with our rendering.

Comment 21

[Carsten Book [:Tomcat]]

can we get a try run to make sure nothing breaks ? :)

Thanks!

Comment 22

[Tim Nguyen :ntim]

Pushed to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b297acc5384

Comment 23

[Matthew N. [:MattN]]

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

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/98c00b224521