DevTools - Get new throbbers for devtools UI

RESOLVED FIXED in Firefox 38

Status

()

Firefox
Developer Tools: Framework
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

Trunk
Firefox 38
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Also, we don't have the current icon in HDPI format. It'd be nice to also have that.
Blocks: 837188
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/loading-04@2x.png

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?
(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/loading-04@2x.png
> 
> 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?
Flags: needinfo?(shorlander)
(Assignee)

Comment 4

3 years ago
Created attachment 8539799 [details] [diff] [review]
Patch
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Attachment #8539799 - Flags: review?(bgrinstead)
(Assignee)

Comment 5

3 years ago
I used the throbbers in Comment 2. They look fine on light theme, but they look pretty weird on dark theme.
(Assignee)

Comment 6

3 years ago
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 ?
(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).
Flags: needinfo?(shorlander)
(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 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).
Attachment #8539799 - Flags: review?(bgrinstead) → feedback+
(Assignee)

Comment 10

3 years ago
(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/
(Assignee)

Comment 11

3 years ago
Created attachment 8540347 [details] [diff] [review]
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.
Attachment #8539799 - Attachment is obsolete: true
Attachment #8540347 - Flags: ui-review?(shorlander)
Attachment #8540347 - Flags: review?(bgrinstead)
Created attachment 8540350 [details]
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 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
Attachment #8540347 - Flags: review?(bgrinstead)
(Assignee)

Comment 14

3 years ago
(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.
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.
Flags: needinfo?(vporof)
The saving attribute is just for showing the spinner, IIRC. You can probably safely switch to the new class and remove the old attribute.
Flags: needinfo?(vporof)
(Assignee)

Comment 17

3 years ago
Created attachment 8540989 [details] [diff] [review]
Patch v3

Fixed nits.
Attachment #8540347 - Attachment is obsolete: true
Attachment #8540347 - Flags: ui-review?(shorlander)
Attachment #8540989 - Flags: ui-review?(shorlander)
Attachment #8540989 - Flags: review?(bgrinstead)
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?
Attachment #8540989 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 19

3 years ago
(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 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.
Attachment #8540989 - Flags: ui-review?(shorlander) → ui-review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
can we get a try run to make sure nothing breaks ? :)

Thanks!
Flags: needinfo?(ntim007)
Keywords: checkin-needed
(Assignee)

Comment 22

3 years ago
Pushed to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b297acc5384
Flags: needinfo?(ntim007)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/98c00b224521
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/98c00b224521
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.