1204510 - Convert about:logins animated SVG spinner/throbber to CSS animation

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Nick Alexander :nalexander [he/him]]

Right now, we use an animated SVG [1] for a spinner/throbber in about:logins.  There are two issues with this:

1) SVGs frequently render with scroll bars around them.  This bug is known (no reference handy) and it can be worked around, but it's unlikely to get fixed soon.

2) animated SVGs are incredibly janky.  Due to choices by Google and the Gecko animation team, these are unlikely to ever improve.

This ticket tracks replacing the existing animated SVG with a CSS animation.

CSS animations are *buttery smooth* and a major area of investment for the platform team; it's a good technology bet.

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutLogins.xhtml?offset=300#41

Comment 1

[Nick Alexander :nalexander [he/him]]

ally: can you link this to the about:logins ticket that added the spinner?

Comment 2

[Nick Alexander :nalexander [he/him]]

antlam: right now we are using [1].

There are lots of CSS animations we might use and modify.  If you search for "material design throbber css", you'll find ones similar to what we use now (aping Google's Android platform spinner).  I took a different style from http://tobiasahlin.com/spinkit/ for use in about:accounts, but I'll unify with this.  (This spinner/throbber will be used when loading remote content in Fennec's about:accounts page.)

Could you choose what you want, and I will work on getting it into HTML/CSS and in place ASAP?

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/images/spinning_throbber.svg

Comment 3

[Allison Naaktgeboren :ally]

Bug 1155345 is your culprit.

You also mentioned during the team meeting last week that svg animations are essentially EOLed. Are you tracking removing all the svg animations anywhere? The only blocking bug I see is for adding about:accounts

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Spoke with Nick about this.

tl;dr
 - I think this is something that we should take a step back and unify across mobile Firefox. It can really be one of those things that we make "our own". Probably a topic for our week in Toronto coming up.
 - for the time being, let's unify all our spinners in the Android product (where we can) to the same spinner that is styled to resemble the system spinners (but with our brand colors)
 - we'd like to use a CSS based spinner rather than an SVG
 - will supply Nick with some CSS.

Comment 5

[Brian Birtles (:birtles)]

Note that you can get the same performance using CSS animations/transitions on your existing SVG assets. There's no need to drop SVG for the graphic elements, since (unlike WebKit/Blink) we layerize SVG elements.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Brian Birtles (:birtles) from comment #5)
> Note that you can get the same performance using CSS animations/transitions
> on your existing SVG assets. There's no need to drop SVG for the graphic
> elements, since (unlike WebKit/Blink) we layerize SVG elements.

birtles: this surprises me.  I saw terrible jank rendering [1] in Fennec, as embedded at [2].  (There were some small changes, but I don't know of any particular reason they would impact.)  Is that expected?

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/images/spinning_throbber.svg

[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutLogins.xhtml?offset=300#41

Comment 7

[Brian Birtles (:birtles)]

(In reply to Nick Alexander :nalexander from comment #6)
> (In reply to Brian Birtles (:birtles) from comment #5)
> > Note that you can get the same performance using CSS animations/transitions
> > on your existing SVG assets. There's no need to drop SVG for the graphic
> > elements, since (unlike WebKit/Blink) we layerize SVG elements.
> 
> birtles: this surprises me.  I saw terrible jank rendering [1] in Fennec, as
> embedded at [2].  (There were some small changes, but I don't know of any
> particular reason they would impact.)  Is that expected?

That's because you're animating stroke-dashoffset as well as transform. As a result it will re-render every tick. You can see that if you turn on paint-flashing.

If you can express your animation in terms of transform/opacity you'll avoid rendering every tick which will make things considerably smoother. Furthermore, the animation can run on the compositor so you'll avoid jank when the main thread gets busy.

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1204510 - Replace animated SVG spinner with non-SVG equivalent. r?ally

Bug 1204510 - Replace animated SVG spinner with non-SVG equivalent. r?ally

The spinner itself is 25px square, with 20px of padding, for a total
of 65px square.

The HTML and CSS was hacked out of
https://github.com/lightningtgc/material-refresh, tree
https://github.com/lightningtgc/material-refresh/tree/6b1be0046d58230ecc00dc929557ad4b3459e304.
The original code is licensed MIT.  I pruned things we don't use,
adjusted the box model for our use, and changed the spinner color.

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: New spinner in about:logins

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: New spinner in about:accounts

Comment 11

[Nick Alexander :nalexander [he/him]]

antlam: please OK the attached videos.  The only significant change (to my eye) is the smooth rendering and the size, which is somewhat smaller than it used to be.

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Nick Alexander :nalexander from comment #11)
> antlam: please OK the attached videos.  The only significant change (to my
> eye) is the smooth rendering and the size, which is somewhat smaller than it
> used to be.

Yeah, it looks a bit small. Can we get the spinner itself to be 60px here (same as the empty panel state's icons)? Also, are you using the action orange and fennec orange here? it looks like it's just 1 orange.

I've placed them in here for reference: http://codepen.io/anon/pen/BojRVv if we could do two shades of orange it would be nice.

Comment 13

[Allison Naaktgeboren :ally]

btw, RB did not notify bugzilla that you asked for a review on this bug :/

Comment 14

[Allison Naaktgeboren :ally]

Comment on attachment 8662000 [details]
MozReview Request: Bug 1204510 - Replace animated SVG spinner with non-SVG equivalent. r?ally

https://reviewboard.mozilla.org/r/19471/#review17645

r+ with nits and followup up bug to clean up the css. You can assign it to me, even better for you.

::: mobile/android/chrome/content/aboutLogins.xhtml:44
(Diff revision 1)
> +            <div class="mui-refresh-wrapper ">

nit space between mui-refresh-wrapper and ending "

::: mobile/android/chrome/content/aboutLogins.xhtml:48
(Diff revision 1)
> +                    <div class="mui-half-circle">

since this has no children, why not   <div class="mui-half-circle"/> ?

::: mobile/android/chrome/content/aboutLogins.xhtml:52
(Diff revision 1)
> +                    <div class="mui-half-circle">

same

::: mobile/android/themes/core/spinner.css:6
(Diff revision 1)
> +	padding: 20px;

I know 4 space is what we use for java, but in css/html/js code we stick to the convention of 2 space tabs (see aboutLogins.css).

::: mobile/android/themes/core/spinner.css:56
(Diff revision 1)
> +.mui-spinner-main .mui-spinner-right .mui-half-circle {

descendant selectors are expensive and touchy/fragile. They don't seem to be adding anything here that can't be done with just hte class name or an id.

I'm also not convinced that classes are best for this. It looks like of this could be removed if you used ids.

::: mobile/android/themes/core/spinner.css:65
(Diff revision 1)
> +.mui-theme .mui-spinner-main .mui-spinner-left .mui-half-circle {

the grandparent/great grandparent are almost certainly not needed here. This seems to be the case for a number of the selectors.

Comment 15

[Nick Alexander :nalexander [he/him]]

(In reply to Anthony Lam (:antlam) from comment #12)
> (In reply to Nick Alexander :nalexander from comment #11)
> > antlam: please OK the attached videos.  The only significant change (to my
> > eye) is the smooth rendering and the size, which is somewhat smaller than it
> > used to be.
> 
> Yeah, it looks a bit small. Can we get the spinner itself to be 60px here
> (same as the empty panel state's icons)? Also, are you using the action
> orange and fennec orange here? it looks like it's just 1 orange.
> 
> I've placed them in here for reference: http://codepen.io/anon/pen/BojRVv if
> we could do two shades of orange it would be nice.

60px is no trouble.  Two colors is harder: blending or animating is not feasible.  Screen caps incoming.

Comment 16

[Nick Alexander :nalexander [he/him]]

Attachment: 60px square spinner

Comment 17

[Nick Alexander :nalexander [he/him]]

Attachment: 60px square spinner, two colors

Comment 18

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Let's stick to 1 color then. Viva la Fennec orange!

Comment 19

[Pulsebot]

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

Comment 20

[Nick Alexander :nalexander [he/him]]

ally: I simplified the CSS considerably, but file follow-up if you're not satisfied with what landed.  Thanks for your guidance!

Comment 21

[Allison Naaktgeboren :ally]

I think that's good enough. So much easier to read. :)

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/77cf41f9a222

Comment 23

[Teodora Vermesan (:TeoVermesan)]

Tested using:
Device: LG G4 (Android 5.1)
Builds: Firefox for Android 43.0a2 (2015-09-22) and Firefox for Android 44.0a1 (2015-09-24)

I don't see any spinner. When going to Tools -> Logins, a blank page is displayed for a few seconds and after that the saved logins are displayed. Please take a look at the following video: https://www.youtube.com/watch?v=FQx2Wz-kRwM&feature=youtu.be

Sometimes a white portion remains on the right side of the screen.