Use CSS animations for the loading and connecting throbbers

RESOLVED DUPLICATE of bug 1352119

Status

()

Firefox
Theme
RESOLVED DUPLICATE of bug 1352119
5 years ago
a day ago

People

(Reporter: jaws, Unassigned)

Tracking

(Blocks: 3 bugs)

Trunk
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite -
qe-verify +

Firefox Tracking Flags

(firefox32 disabled, firefox33 disabled, firefox34 disabled, firefox35 wontfix)

Details

(Whiteboard: [Snappy][qf])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments, 9 obsolete attachments)

30.88 KB, image/png
Details
30.90 KB, image/png
Details
32.49 KB, image/png
Details
32.19 KB, image/png
Details
979 bytes, image/svg+xml
Details
525 bytes, image/svg+xml
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details | Review
56.48 KB, image/gif
Details
Created attachment 627860 [details] [diff] [review]
WIP patch for bug (Windows only)

We should switch to using CSS animations for the loading & connecting throbbers. This should give us a perf win and remove allow us to guarantee smooth animation of the throbbers (with bug 706179 fixed).
Created attachment 627865 [details] [diff] [review]
Patch for bug

I'd like to hold off on landing this patch until more of the OMTC and async animations work is completed (hopefully Firefox 16).
Attachment #627860 - Attachment is obsolete: true
Attachment #627865 - Flags: review?(dao)
Created attachment 627867 [details] [diff] [review]
Patch for bug v2

Removed some duplicate CSS.
Attachment #627865 - Attachment is obsolete: true
Attachment #627865 - Flags: review?(dao)
Attachment #627867 - Flags: review?(dao)

Comment 3

5 years ago
Comment on attachment 627867 [details] [diff] [review]
Patch for bug v2

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

This looks good, except I think we can make the animation code a bit simpler. See below:

::: browser/themes/gnomestripe/browser.css
@@ +1559,5 @@
>  .tab-throbber {
>    list-style-image: url("chrome://browser/skin/tabbrowser/connecting.png");
> +  -moz-animation-duration: 960ms;
> +  -moz-animation-iteration-count: infinite;
> +  -moz-animation-name: connecting;

I think we should set:

-moz-animation-name: rotate;

for both states and set:

-moz-animation-direction: reverse;

on the [progress] one or on a sepearate :not([progress]) rule.

@@ +1575,5 @@
> +  }
> +
> +  to {
> +    -moz-transform: rotate(360deg);
> +  }

Intuitively, we shouldn't end up with two separate frames for 0deg and 360deg by using this, and hopefully, we don't. I'll ask dbaron about that.
Attachment #627867 - Flags: feedback-
Created attachment 628498 [details] [diff] [review]
Patch for bug v3

We can't use -moz-animation-direction:reverse because it hasn't been implemented in Gecko yet. See bug 655920 for more information.

I changed the names of the animations to be more generic (rotate-clockwise, rotate-counterclockwise) and also set the animation-timing-function to linear so the animation speed will be consistent.

After some more thought, I now think that we should land this for Firefox 15 and not wait for the async CSS animations. The current animation is already janky, and async CSS animations don't have a quick timeline for Windows support (it looks like work is only on Mac/Android at the moment -- since this is where OMTC is currently).
Attachment #627867 - Attachment is obsolete: true
Attachment #627867 - Flags: review?(dao)
Attachment #628498 - Flags: review?(fryn)
Created attachment 628518 [details] [diff] [review]
Patch for bug v3

And now with optimized PNGs :-)
Attachment #628498 - Attachment is obsolete: true
Attachment #628498 - Flags: review?(fryn)
Attachment #628518 - Flags: review?(fryn)

Comment 6

5 years ago
Comment on attachment 628518 [details] [diff] [review]
Patch for bug v3

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

Looks good to me. :)

::: browser/themes/gnomestripe/browser.css
@@ +1572,5 @@
> +
> +@-moz-keyframes rotate-clockwise {
> +  from {
> +    -moz-transform: rotate(0deg);
> +  }

The `from` clause is actually optional in this case for our implementation of CSS animations.
Attachment #628518 - Flags: review?(fryn) → review+
Thanks!

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/970abbd6e5b0
Flags: in-testsuite-
Target Milestone: --- → Firefox 15
This caused some regressions in a number of memory and latency benchmarks across all platforms:
https://groups.google.com/d/topic/mozilla.dev.tree-management/4APYK_sA7e0/discussion
Backed out on inbound due the perf regressions.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a095777e385
Target Milestone: Firefox 15 → ---
Comment on attachment 628518 [details] [diff] [review]
Patch for bug v3

>+@-moz-keyframes rotate-clockwise {

rename to throbber-connecting

>+@-moz-keyframes rotate-counterclockwise {

rename to throbber-loading

>+  from {
>+    -moz-transform: rotate(0deg);
>+  }
>+
>+  to {
>+    -moz-transform: rotate(-360deg);
>+  }

make this from 360 to 0 instead

I thought I made these comments on the first patch, but apparently I didn't...
Attachment #628518 - Flags: review-
(In reply to Jared Wein [:jaws] from comment #4)
> After some more thought, I now think that we should land this for Firefox 15
> and not wait for the async CSS animations. The current animation is already
> janky,

The APNGs have a deliberately low frame rate to reduce the perf impact.
(In reply to Dão Gottwald [:dao] from comment #10)
> >+@-moz-keyframes rotate-clockwise {
> 
> rename to throbber-connecting
> 
> >+@-moz-keyframes rotate-counterclockwise {
> 
> rename to throbber-loading

... or the other way around ...
I think what we'll do for now is to just update the APNGs since the computational overhead of CSS animations isn't worth it.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
What computational overhead are you referring to?
It seems to me that this just needs to wait for bug 706179.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(In reply to Dão Gottwald [:dao] from comment #14)
> What computational overhead are you referring to?

The stuff Jared and I were talking about between landing and backout. :)

My concern about this patch is that image throbbers should be fairly efficient -- they get decoded once, so the only work as it plays is a timer firing on the main thread ("paint next frame, please") and a blit. [In theory I think that could even be entirely off-main-thread and even purely on the GPU, but not today.]

CSS transforms, though, have to be computed each "frame". So ~60 times a second we're computing a transform of an image. Multiplied by each throbber on screen, since they'd all be rotating at different offsets (which seems undesirable to me). [HW accel for transforms will help -- I think that's not yet implemented? -- but for those without the CPU has to do all that work every frame.]

Also, oddly enough, from Jared's demo the APNG was actually smoother under load than the transform was. Which was unexpected.

I think we should call this WONTFIX, the jankyness we're seeing today isn't inherent to the APNG throbber. We can reconsider later if things change.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → WONTFIX

Comment 17

4 years ago
Once we enable OMTA for desktop, this will be cheap and smooth.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Depends on: 913249, 756601
There's renewed interest in this bug - and the demo we just put together was pretty compelling.

Try builds coming: https://tbpl.mozilla.org/?tree=Try&rev=309cefb6ca13

These try builds put perma-spinners on the tabs and force OMTC and OMTA on.
That try build looks great!
I'd like to adjust the timing a little, but this is definitely a huge step forward.
Spec/demo for the timing will be posted here soon.

Comment 20

3 years ago
Philipp, did you have the time to finish the spec yet?
Flags: needinfo?(philipp)
Why do we need a spec here rather than translating the APNG to CSS? Seems like further design changes and enhancements could happen independently.
Dão is right, the technical part can be done without a spec.
Nonetheless, here's a spec ;)
http://phlsa.github.io/ff-loading-indicators/

The idea is that we can improve the perceived speed of the page load when we make the loading indicator dynamic. It makes sense to deal with this issue now to avoid a rewrite later on.
Flags: needinfo?(philipp)

Comment 23

3 years ago
For the scaling, I'd use an easing function (like ease-in), feels smoother that way.

As for a dynamic loading indicator, I don't feel much of a difference between 1 & 2 (apart from the dynamic indicator running irregularly sometimes, but I'm using Fx 27 Beta, maybe Nightly is better to test this with). How do you determine the run time for #1, though? That governs the timing function … do you want it get faster quickly and then run that fast for the rest of the loading time (which can be >10s in quite a few cases and regions)?
(In reply to Philipp Sackl [:phlsa] from comment #22)
> The idea is that we can improve the perceived speed of the page load when we
> make the loading indicator dynamic. It makes sense to deal with this issue
> now to avoid a rewrite later on.

Except that translating the current throbber is straightforward whereas a design change is not. Can we please discuss the questions asked in comment 23 in a separate bug and in the meantime make progress here?

Updated

3 years ago
Blocks: 958480

Updated

3 years ago
Blocks: 950073
Whiteboard: [Snappy] → [Snappy] p=0

Updated

3 years ago
Whiteboard: [Snappy] p=0 → [Snappy] p=5

Updated

3 years ago
No longer blocks: 950073
Flags: firefox-backlog+
Created attachment 8407797 [details] [diff] [review]
Patch v4

Addressed feedback and rebased. Dao, I'm not sure why but the images weren't showing up when using list-style-image. They do appear with background-image.

Applying a transform:rotate() to the list-style-image causes it to go blank on my Windows8 machine. This does not happen for background-image. Do you know why?
Attachment #628518 - Attachment is obsolete: true
Attachment #8407797 - Flags: review?(dao)
Status: REOPENED → ASSIGNED
Attachment #8407797 - Flags: review?(bmcbride)
Comment on attachment 8407797 [details] [diff] [review]
Patch v4

This needs to be updated for bug 994954. (Not sure if the new throbber will be compatible with this at all -- I haven't seen it in action yet.)
Attachment #8407797 - Flags: review?(dao)
Attachment #8407797 - Flags: review?(bmcbride)
Darrin, IIRC you did the animation for the bookmark star bounce. Would you be able to recreate the new progress throbber with CSS animations?
Flags: needinfo?(dhenein)
Duplicate of this bug: 1009126
Comment on attachment 8407797 [details] [diff] [review]
Patch v4

Re-requesting review now that the new throbber was reverted.
Attachment #8407797 - Flags: review?(dao)
Flags: needinfo?(dhenein)
Comment on attachment 8407797 [details] [diff] [review]
Patch v4

>-  list-style-image: url("chrome://browser/skin/tabbrowser/connecting.png");
>+  background-image: url("chrome://browser/skin/tabbrowser/connecting.png");

browser/themes/osx/browser.css is still using list-style-image for connecting@2x.png and loading@2x.png and would need to be updated, otherwise you end up with both a list-style-image and a background-image. That said, I'd prefer if we kept using list-style-image. Can you ping somebody from the platform team or file a bug on the images disappearing?
Attachment #8407797 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #30)
> I'd prefer if we kept using list-style-image. Can you ping somebody from the
> platform team or file a bug on the images disappearing?

Removing layer="true" from the image element seems to fix it.
Created attachment 8426349 [details] [diff] [review]
Patch v5

Thanks for finding that the culprit was the layer="true"
Attachment #8407797 - Attachment is obsolete: true
Attachment #8426349 - Flags: review?(dao)
Comment on attachment 8426349 [details] [diff] [review]
Patch v5

Afaik layer="true" was added to improve performance. So please still ping sb. from the layout team and ask whether this is fine to remove and tell them that it's causing problems.
Attachment #8426349 - Flags: review?(dao) → review+
Matt, you added this attribute in bug 781053. Is it alright by you to remove this attribute now that we are switching to using a CSS animation?
Flags: needinfo?(matt.woodrow)
Yes, absolutely.
Flags: needinfo?(matt.woodrow)
Keywords: checkin-needed
<jaws> mattwoodrow: i forgot to mention in my needinfo that the presence of layer="true" meant that the transform animation caused the image to not be painted
<jaws> is this a known issue? is it something we should get a bug on file for?
<mattwoodrow> jaws: Worth filing a bug, but it’s not really high priority since layer=“true” is a weird xul hack that is barely used
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #36)
> <jaws> mattwoodrow: i forgot to mention in my needinfo that the presence of
> layer="true" meant that the transform animation caused the image to not be
> painted
> <jaws> is this a known issue? is it something we should get a bug on file
> for?
> <mattwoodrow> jaws: Worth filing a bug, but it’s not really high priority
> since layer=“true” is a weird xul hack that is barely used

Filed bug 1014230.
Jared, is your patch still Windows only or does it also affect other platforms?
(In reply to Philipp Sackl [:phlsa] from comment #38)
> Jared, is your patch still Windows only or does it also affect other
> platforms?

It is for all platforms.
https://hg.mozilla.org/integration/fx-team/rev/a9329165ace3
Keywords: checkin-needed
Whiteboard: [Snappy] p=5 → [Snappy] p=5[fixed-in-fx-team]

Updated

3 years ago
Depends on: 994541
https://hg.mozilla.org/mozilla-central/rev/a9329165ace3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago3 years ago
Resolution: --- → FIXED
Whiteboard: [Snappy] p=5[fixed-in-fx-team] → [Snappy] p=5
Target Milestone: --- → Firefox 32
Added to Iteration 32.2
Whiteboard: [Snappy] p=5 → [Snappy] p=5 s=it-32c-31a-30b.2 [qa?]

Updated

3 years ago
Depends on: 1014808

Updated

3 years ago
Depends on: 1014811
Hi Juan, can you review the bug to determine if further verification is required.
Flags: needinfo?(jbecerra)

Updated

3 years ago
Depends on: 1015569

Comment 44

3 years ago
This makes the throbber really blurry :(.
Depends on: 1015317
Is there a bug on setting layers.offmainthreadcomposition.async-animations to true on desktop? That's needed for the CSS animation to really be smooth under load, isn't it?
https://bugzilla.mozilla.org/show_bug.cgi?id=788522 ?
Actually, it seems like bug 980770 is the better response to comment #45.

Updated

3 years ago
Depends on: 980770

Updated

3 years ago
Whiteboard: [Snappy] p=5 s=it-32c-31a-30b.2 [qa?] → [Snappy] p=5 s=it-32c-31a-30b.3 [qa?]

Comment 48

3 years ago
Is this why the connecting/loading spinners are circling around out-of-whack on Nightly lately?
(In reply to alex_mayorga from comment #48)
> Is this why the connecting/loading spinners are circling around out-of-whack
> on Nightly lately?

Can you attach a screencast/video showing the issue you're seeing? Do you have any tab-related add-ons installed?
Flags: needinfo?(alex_mayorga)

Updated

3 years ago
Depends on: 1016434

Comment 50

3 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #49)
> (In reply to alex_mayorga from comment #48)
> > Is this why the connecting/loading spinners are circling around out-of-whack
> > on Nightly lately?
> 
> Can you attach a screencast/video showing the issue you're seeing? Do you
> have any tab-related add-ons installed?

Filed bug 1016434 , with screenshot.
No longer depends on: 1016434

Updated

3 years ago
Depends on: 1016434

Comment 51

3 years ago
Please see http://screencast.com/t/fViRLzExHEh and let me know if you need further information from the system.

These are the add-ons, none of them is tab-related:

Adblock Plus 2.6
Diccionario de Español/México 1.1.3
Firefox OS Simulator 4.0.1
Iconic Firefox Menu 2.0
InlineDisposition 1.0.2.4 [DISABLED]
Logitech SetPoint 6.5 [DISABLED]
Nightly Tester Tools 3.7
Pixlr Grabber 2.1.4
Flags: needinfo?(alex_mayorga) → needinfo?(jaws)
Flags: needinfo?(jaws)
Whiteboard: [Snappy] p=5 s=it-32c-31a-30b.3 [qa?] → [Snappy] p=5 s=it-32c-31a-30b.3 [qa+]

Comment 52

3 years ago
Confirmed on WinXP using Nightly in Safe Mode

Updated

3 years ago
Depends on: 1016621

Updated

3 years ago
Depends on: 1016643
Depends on: 1016942
Flags: needinfo?(jbecerra)
QA Contact: alexandra.lucinet

Updated

3 years ago
No longer depends on: 1015317
Hi Alexandra, will it be possible to have this bug verified by end of day this Friday?  Our current Iteration ends on Monday June 9.
Flags: needinfo?(alexandra.lucinet)

Comment 54

3 years ago
Verified as fixed with latest Nightly (Build ID: 20140602072051) on Windows 7 64bit, Mac OS X 10.9.2 and Ubuntu 14.04 32bit:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0		
Mozilla/5.0 (X11; Linux i686; rv:32.0) Gecko/20100101 Firefox/32.0

During the above testing, I've encountered bug 1016434 and bug 998267;
Since both issues are tracked separately and none are blockers, marking as verified fixed.
Status: RESOLVED → VERIFIED
Flags: needinfo?(alexandra.lucinet)
Whiteboard: [Snappy] p=5 s=it-32c-31a-30b.3 [qa+] → [Snappy] p=5 s=it-32c-31a-30b.3 [qa!]
This was backed out from Fx32 and Fx33 in bug 1016434.
status-firefox32: --- → wontfix
status-firefox33: --- → wontfix
status-firefox34: --- → fixed
status-firefox32: wontfix → disabled
status-firefox33: wontfix → disabled
I've filed bug 1059557 about the negative performance impact of this change.

Updated

3 years ago
Depends on: 1059557
This is being backed out in https://hg.mozilla.org/integration/fx-team/rev/4af29b25a7bf, mainly because of bug 1059557. Should re-land when bug 980770 is fixed.
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/4af29b25a7bf
Status: VERIFIED → REOPENED
status-firefox34: fixed → wontfix
status-firefox35: --- → affected
Resolution: FIXED → ---
Target Milestone: Firefox 32 → ---
Assignee: jaws → nobody
Status: REOPENED → NEW

Updated

3 years ago
Points: --- → 5
Flags: qe-verify+
Whiteboard: [Snappy] p=5 s=it-32c-31a-30b.3 [qa!] → [Snappy]

Updated

3 years ago
status-firefox34: wontfix → disabled
status-firefox35: affected → wontfix
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Created attachment 8774521 [details] [diff] [review]
Patch v5 (rebased)
Attachment #8426349 - Attachment is obsolete: true
Keywords: checkin-needed
Has the performance impact of this been mesaured?
Created attachment 8774528 [details]
with-sync-throbbers.png

This screenshot is of the performance view of Process Explorer with 4 tabs open (about:home, about:newtab, about:newtab, about:newtab), idle without the patch.
Created attachment 8774529 [details]
with-async-throbbers.png

This screenshot is of the performance view of Process Explorer with 4 tabs open (about:home, about:newtab, about:newtab, about:newtab), idle with the patch.
Created attachment 8774530 [details]
end-showing-three-websites-loaded-sync-throbber.png

This screenshot is of the performance view of Process Explorer with 4 tabs open (about:home, about:newtab, about:newtab, about:newtab) without the patch. The last about:newtab tab then loads http://www.freep.com, http://www.detnews.com, and finally http://www.lsj.com, making sure to finish each load before starting the next website load.

The spike in CPU coincides with the loading of the webpages.
Created attachment 8774531 [details]
end-showing-three-websites-loading-async-throbber.png

This screenshot is of the performance view of Process Explorer with 4 tabs open (about:home, about:newtab, about:newtab, about:newtab) with the patch. The last about:newtab tab then loads http://www.freep.com, http://www.detnews.com, and finally http://www.lsj.com, making sure to finish each load before starting the next website load.

The spike in CPU coincides with the loading of the webpages.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #60)
> Has the performance impact of this been mesaured?

Based off of how bug 1059557 was filed, I measured the impact of this change by using Process Explorer and watching the performance measures of firefox.exe (the chrome process).

I see no discernible difference between the performance output with and without the patch. The animation is noticeably smoother with the patch.
The other problem with this patch (or at least with the version that was checked in last time) is that it makes the "connecting" animation look slightly wrong, at least on macOS. The macOS connecting throbber is not completely symmetrical because it has a slight inner shadow towards the bottom, and this shadow is not supposed to rotate along with the rest of the icon.
I don't have a good solution for this though, and somebody else will have to decide whether this is an acceptable trade-off to make.

Updated

9 months ago
Duplicate of this bug: 1288986

Comment 68

9 months ago
In bug 1288986, Steven made a SVG throbber with CSS animations [1] to rotate it. And I think replace the PNG by SVG is better because the CSS animation code will be inside the SVG which we don't need to add the code to all the CSS using the png [2].

However, he noticed "it looks great on hi-res displays, it looks pretty blurry on normal displays."

Steven, my desktop is 1080p, notebook is 1366x768 and both look great. What are the normal displays? I couldn't find a 1024x768 one in my office.

[1] http://people.mozilla.org/~shorlander/mockups-interactive/spinner-test/spinner-01-svg.html
[2] https://dxr.mozilla.org/mozilla-central/search?q=loading.png&redirect=false

Comment 69

9 months ago
NI for comment 68.
Flags: needinfo?(shorlander)
(In reply to Ting-Yu Chou [:ting] from comment #68)
> And I think replace the PNG by SVG is better because the CSS animation
> code will be inside the SVG which we don't need to add the code to all the
> CSS using the png [2].

I believe the animation will only run on the compositor if the SVG itself is put directly into the XUL DOM, or at least embedded as a document, but not if it's embedded as an image.
(In reply to Markus Stange [:mstange] from comment #66)
> The other problem with this patch (or at least with the version that was
> checked in last time) is that it makes the "connecting" animation look
> slightly wrong, at least on macOS. The macOS connecting throbber is not
> completely symmetrical because it has a slight inner shadow towards the
> bottom, and this shadow is not supposed to rotate along with the rest of the
> icon.
> I don't have a good solution for this though, and somebody else will have to
> decide whether this is an acceptable trade-off to make.

Yeah, this patch does have the rotating shadow. Clearing the checkin-needed until we can get a connecting throbber without this issue.
Keywords: checkin-needed

Comment 72

9 months ago
(In reply to Markus Stange [:mstange] from comment #70)
> (In reply to Ting-Yu Chou [:ting] from comment #68)
> > And I think replace the PNG by SVG is better because the CSS animation
> > code will be inside the SVG which we don't need to add the code to all the
> > CSS using the png [2].
> 
> I believe the animation will only run on the compositor if the SVG itself is
> put directly into the XUL DOM, or at least embedded as a document, but not
> if it's embedded as an image.

Too bad :(, :birtles, is this the case?
Flags: needinfo?(bbirtles)
(In reply to Ting-Yu Chou [:ting] from comment #72)
> (In reply to Markus Stange [:mstange] from comment #70)
> > (In reply to Ting-Yu Chou [:ting] from comment #68)
> > > And I think replace the PNG by SVG is better because the CSS animation
> > > code will be inside the SVG which we don't need to add the code to all the
> > > CSS using the png [2].
> > 
> > I believe the animation will only run on the compositor if the SVG itself is
> > put directly into the XUL DOM, or at least embedded as a document, but not
> > if it's embedded as an image.
> 
> Too bad :(, :birtles, is this the case?

CSS Animations in SVG images don't work at all due to bug 1190881 (which is a really bad regression we've been shipping for a long time--I keep campaigning to get someone to look at it but with no luck yet).
Flags: needinfo?(bbirtles)

Comment 74

9 months ago
Clear NI because of comment 73.
Flags: needinfo?(shorlander)

Comment 75

9 months ago
Sorry for another NI, but maybe you can help with comment 71?
Flags: needinfo?(shorlander)
Comment on attachment 8774521 [details] [diff] [review]
Patch v5 (rebased)

I need to retract my r+ from two years ago because loading.png is now used in more contexts, as mentioned in comment 68. Short of being able to put the CSS animation in an SVG, I suppose we could add a CSS class for this, e.g. "loading-icon" or just "throbber", and define the animation in global.css.
Attachment #8774521 - Flags: review-
Depends on: 1190881

Comment 77

8 months ago
(In reply to Ting-Yu Chou [:ting] from comment #68)
> In bug 1288986, Steven made a SVG throbber with CSS animations [1] to rotate
>
> However, he noticed "it looks great on hi-res displays, it looks pretty
> blurry on normal displays."

Probably bug 1271983.
Hey birtles - it looks like bug 1190881 got fixed about 2 months ago. Does that mean that the throbber in this patch will animate off the main thread now?
Flags: needinfo?(bbirtles)
Looking at the CSS there I can't see any reason why it would not. It's worth checking though because there are some special cases where we might not run it on the compositor (e.g. layer size too large, conflicting animations of left/top etc.).

If you're able to inspect the content using DevTools then you can quickly confirm if the animation is running on the compositor since there will be a lightning bolt mark next to any animations running on the compositor. For transform/opacity animations that *aren't* running on the compositor you can typically expand the animation and look at the tool tip which will normally tell you *why* it's not running on the compositor.

If you can't inspect the content using DevTools, then your best bet is just to artificially stall the main thread with JS and check if it keeps animating or not.
Flags: needinfo?(bbirtles)

Comment 80

6 months ago
(In reply to Mike Conley (:mconley) - (high latency on reviews and needinfos) from comment #78)
> Hey birtles - it looks like bug 1190881 got fixed about 2 months ago. Does
> that mean that the throbber in this patch will animate off the main thread
> now?

If you're asking is CSS transform in SVG image animating off the main thread with bug 1190881, the answer is no as what I've learned from :cjku.
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Yeah, we don't seem to do animations inside image tags on the compositor. Test case: http://codepen.io/birtles/pen/bwXqMb

If you right-click the image and choose 'View Image' then look at the Animations panel in the inspector you'll see it's running on the compositor: i.e. runs on the compositor when its loaded normally but not when loaded as an image document.
Blocks: 1325171
(In reply to Dão Gottwald [::dao] from comment #76)
> Comment on attachment 8774521 [details] [diff] [review]
> Patch v5 (rebased)
> 
> I need to retract my r+ from two years ago because loading.png is now used
> in more contexts, as mentioned in comment 68. Short of being able to put the
> CSS animation in an SVG, I suppose we could add a CSS class for this, e.g.
> "loading-icon" or just "throbber", and define the animation in global.css.

Having re-read this bug in preparation for Photon, I've noticed that the only thing really blocking us from an r+ here isn't some low-level platform thing, but a refactor for the other users of the loading icon.

Dao, would you be willing to accept a patch that adds a new image, loading-stillframe.png and loading-stillframe@2x.png so that we can get this into the tree, and then I'll file (and self-assign) a follow-up bug to add a new class which uses the CSS animation in all places where we use the loading throbber?
Flags: needinfo?(dao+bmo)

Comment 83

2 months ago
(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #82)
> Having re-read this bug in preparation for Photon, I've noticed that the
> only thing really blocking us from an r+ here isn't some low-level platform
> thing, but a refactor for the other users of the loading icon.
> 
> Dao, would you be willing to accept a patch that adds a new image,
> loading-stillframe.png and loading-stillframe@2x.png so that we can get this
> into the tree, and then I'll file (and self-assign) a follow-up bug to add a
> new class which uses the CSS animation in all places where we use the
> loading throbber?

Actually this is not needed anymore, CSS animations in embed SVG have been supported since a few releases now.

So this should be as easy as a simple image swap.

Comment 84

2 months ago
I take what I said back, comment 81 suggests CSS animations in embed SVG are not optimized yet.
Right, CSS animations in SVG run on the compositor, but not when they are loaded via an <img> tag. Instead, it's better to load the SVG as a <img> tag, and then apply the transformation (rotation etc.) via CSS animation to the <img> element itself (or a containing div).
(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #82)
> (In reply to Dão Gottwald [::dao] from comment #76)
> > Comment on attachment 8774521 [details] [diff] [review]
> > Patch v5 (rebased)
> > 
> > I need to retract my r+ from two years ago because loading.png is now used
> > in more contexts, as mentioned in comment 68. Short of being able to put the
> > CSS animation in an SVG, I suppose we could add a CSS class for this, e.g.
> > "loading-icon" or just "throbber", and define the animation in global.css.
> 
> Having re-read this bug in preparation for Photon, I've noticed that the
> only thing really blocking us from an r+ here isn't some low-level platform
> thing, but a refactor for the other users of the loading icon.
> 
> Dao, would you be willing to accept a patch that adds a new image,
> loading-stillframe.png and loading-stillframe@2x.png so that we can get this
> into the tree, and then I'll file (and self-assign) a follow-up bug to add a
> new class which uses the CSS animation in all places where we use the
> loading throbber?

Yes, except that we'll need proper images (comment 66) and SVG would be preferable over PNG.
Flags: needinfo?(dao+bmo)

Updated

2 months ago
Flags: needinfo?(shorlander)
Created attachment 8848319 [details]
Loading Spinner - SVG
Created attachment 8848320 [details]
Connecting Spinner - SVG
Mind to rework the patch a bit?
Flags: needinfo?(jaws)
Created attachment 8848457 [details] [diff] [review]
Updated patch

This isn't quite ready since fill-opacity doesn't yet work in combination with context-fill. I had to use context-fill for connecting.svg since Stephen's original image wouldn't work on dark backgrounds.
Attachment #8774521 - Attachment is obsolete: true
Flags: needinfo?(jaws)

Updated

a month ago
Depends on: 1058040
No longer depends on: 1190881

Updated

a month ago
Blocks: 1348289
No longer blocks: 1325171

Updated

a month ago
Depends on: 1350015
Comment hidden (mozreview-request)

Updated

a month ago
Attachment #8848457 - Attachment is obsolete: true
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
(Reporter)

Comment 92

a month ago
mozreview-review
Comment on attachment 8851655 [details]
Bug 759252 - Use CSS animations for the loading and connecting throbbers.

https://reviewboard.mozilla.org/r/123918/#review126420

::: browser/themes/shared/tabbrowser/connecting.svg:3
(Diff revision 1)
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="16" height="16" viewBox="0 0 16 16">

Please remove the xlink DTD since it's unused.
Attachment #8851655 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)

Comment 94

a month ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76ad1c764e5c
Use CSS animations for the loading and connecting throbbers. r=jaws

Updated

a month ago
Blocks: 1351038

Updated

a month ago
Blocks: 1351039

Updated

a month ago
Blocks: 1351042

Comment 95

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/76ad1c764e5c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years agoa month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1351323
No longer depends on: 1016621
No longer depends on: 1016643
Created attachment 8852133 [details]
End result
Whiteboard: [Snappy] → [Snappy][qf]
To understand how a smoother animations impact users, what is the effort to have this behind a pref so this can be run as controlled experiment?
Flags: needinfo?(wkocher)
Flags: needinfo?(mconley)
Impact users in what way? What's the hypothesis and what would you measure in such an experiment?
I just merged the patch to mozilla-central as part of a normal merge. I'm not the right person to ask.
Flags: needinfo?(wkocher)
I personally don't think it's worth the engineering time to try to instrument this with a pref, but I'm open to argument.
Flags: needinfo?(mconley)
The SVG file probably shouldn't link to a DTD.  I saw this in the output of a debug build:

[Parent 23032] WARNING: Failed to open external DTD: publicId "-//W3C//DTD SVG 1.1//EN" systemId "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd" base "file:///home/dbaron/builds/clean-mozilla-central/mozilla/browser/themes/shared/tabbrowser/connecting.svg" URL "resource://gre/res/dtd/svg11.dtd": file /home/dbaron/builds/clean-mozilla-central/mozilla/parser/htmlparser/nsExpatDriver.cpp, line 702

(It really doesn't need a DOCTYPE declaration at all.)
Flags: needinfo?(dao+bmo)
(In reply to David Baron :dbaron: ⌚️UTC-4 from comment #101)
> The SVG file probably shouldn't link to a DTD.  I saw this in the output of
> a debug build:
> 
> [Parent 23032] WARNING: Failed to open external DTD: publicId "-//W3C//DTD
> SVG 1.1//EN" systemId "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd"
> base
> "file:///home/dbaron/builds/clean-mozilla-central/mozilla/browser/themes/
> shared/tabbrowser/connecting.svg" URL "resource://gre/res/dtd/svg11.dtd":
> file
> /home/dbaron/builds/clean-mozilla-central/mozilla/parser/htmlparser/
> nsExpatDriver.cpp, line 702
> 
> (It really doesn't need a DOCTYPE declaration at all.)

filed bug 1351986
Depends on: 1351986
Flags: needinfo?(dao+bmo)
Depends on: 1352085
Depends on: 1352199

Updated

29 days ago
Depends on: 1352258
Do we care about the power usage regression that this will cause?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #103)
> Do we care about the power usage regression that this will cause?

Are you asking about power usage while the throbber is displayed or power usage after pages have been loaded?

When the throbber is not busy (connecting or showing progress), it has display:none, and the `animation-name` CSS property is set to its initial value ("none"). Therefore, we shouldn't have a power regression when it's not visible.

If you're asking about only when it is visible, it is a more nuanced question since it won't be visible for the whole time that Firefox is running. At this point, is it large enough of an issue to prioritize it?
Flags: needinfo?(jmuizelaar)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #104)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #103)
> > Do we care about the power usage regression that this will cause?
> 
> Are you asking about power usage while the throbber is displayed or power
> usage after pages have been loaded?

While the throbber is displayed.

> If you're asking about only when it is visible, it is a more nuanced
> question since it won't be visible for the whole time that Firefox is
> running. At this point, is it large enough of an issue to prioritize it?

I guess it could be interesting to have telemetry of what percentage of the time is Firefox trying to paint at 60fps and see how that changes with this patch. That would help evaluate the importance.
Flags: needinfo?(jmuizelaar)
For future reference, when converting UI images to SVG it would be preferable to have the switch to SVG as a separate changeset if possible. I know that would have involved making a non-animated version of connecting.png in this case, but in general it will help if we encounter Talos/perf regressions to more quickly figure out whether a perf issue is due to the use of SVG or not. In some cases the switch to SVG may not cause regressions, but in other cases it certainly will. In general be aware that SVG can be much more expensive than raster image formats, depending on the image.

Comment 107

25 days ago
setting gfx.font_rendering.opentype_svg.enabled false
for rendering hw/software/security reasons,
breaks this and turns it into black.
can it be fixed?
Flags: needinfo?(jaws)
Flags: needinfo?(dao+bmo)
(In reply to Sunil Kumar from comment #107)
> setting gfx.font_rendering.opentype_svg.enabled false
> for rendering hw/software/security reasons,
> breaks this and turns it into black.
> can it be fixed?

Jonathan, is this a pref that we should still support?

Sunil, can you explain for what security reasons you are disabling this pref?
Flags: needinfo?(jfkthame)
Flags: needinfo?(jaws)
Flags: needinfo?(dao+bmo)

Comment 109

25 days ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #108)
> (In reply to Sunil Kumar from comment #107)
> > setting gfx.font_rendering.opentype_svg.enabled false
> > for rendering hw/software/security reasons,
> > breaks this and turns it into black.
> > can it be fixed?
> 
> Jonathan, is this a pref that we should still support?
> 
> Sunil, can you explain for what security reasons you are disabling this pref?

In tor it allows fingerprinting, CVE-2016-9079(fixed),
disabling the preference help in rendering fonts correctly/better(just like woff disabling helps sometimes and more security enhance) on some hardware.

P.S: sorry for bad english
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #108)
> (In reply to Sunil Kumar from comment #107)
> > setting gfx.font_rendering.opentype_svg.enabled false
> > for rendering hw/software/security reasons,
> > breaks this and turns it into black.
> > can it be fixed?
> 
> Jonathan, is this a pref that we should still support?

I'd be inclined to keep it around, as opentype-svg support is still a relatively little-used and therefore probably less-tested feature; though IIUC support has now been added in Edge, which may lead to wider adoption to the point where it becomes a required part of the platform.

My question here, though, is why it has any relation to this bug. AFAICS, the patch here added some SVG images (not an OpenType-SVG font), so that pref shouldn't have any effect on it. If it's confirmed that this pref breaks the rendering of SVG images in general, that sounds like something that needs debugging. (I'm sure it didn't originally!)
Flags: needinfo?(jfkthame)

Comment 111

25 days ago
(In reply to Jonathan Kew (:jfkthame) from comment #110)
> I'd be inclined to keep it around, as opentype-svg support is still a
> relatively little-used and therefore probably less-tested feature; though
> IIUC support has now been added in Edge, which may lead to wider adoption to
> the point where it becomes a required part of the platform.
> 
> My question here, though, is why it has any relation to this bug. AFAICS,
> the patch here added some SVG images (not an OpenType-SVG font), so that
> pref shouldn't have any effect on it. If it's confirmed that this pref
> breaks the rendering of SVG images in general, that sounds like something
> that needs debugging. (I'm sure it didn't originally!)


So tested it again on different platforms to see if works some thing else but it's this patch

after the patch

https://hg.mozilla.org/mozilla-central/rev/38894655c89e68bcd8f45d31a0d3005f2c2b53db

breaks

but before this patch works

https://hg.mozilla.org/mozilla-central/rev/7a3f514cf8490d271ee373a1d2999e4ea4dee2d7
Depends on: 1353422
Depends on: 1354080
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #104)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #103)
> > Do we care about the power usage regression that this will cause?
> 
> Are you asking about power usage while the throbber is displayed or power
> usage after pages have been loaded?
> 
> When the throbber is not busy (connecting or showing progress), it has
> display:none, and the `animation-name` CSS property is set to its initial
> value ("none"). Therefore, we shouldn't have a power regression when it's
> not visible.
> 
> If you're asking about only when it is visible, it is a more nuanced
> question since it won't be visible for the whole time that Firefox is
> running. At this point, is it large enough of an issue to prioritize it?

FTR, I have just filed bug 1354080 about this, because I've been noticing excessive CPU usage while the throbber is running; IMO it has significantly degraded the user experience in my Nightly.

Comment 113

22 days ago
svg.disabled=true

does not cause this.
why?

should not all svg be disabled?
'svg.disabled' only disables SVG support for websites content.
The browser UI uses a lot of SVGs, so it can not be disabled globally.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1216893 for more.

Updated

18 days ago
No longer depends on: 1352258
Backed out for causing severe talos regressions (bug 1352085):

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f1f01d369641c422db403eb5550b40cae59889
https://hg.mozilla.org/integration/mozilla-inbound/rev/5110f7c040365cacc750021b6b2aeeaa74728e7e

It looks like this is a dead end, and now superseded by bug 1352119.
Assignee: dao+bmo → nobody
status-firefox55: fixed → ---
Resolution: FIXED → DUPLICATE
Target Milestone: Firefox 55 → ---
Duplicate of bug: 1352119
See Also: → bug 1357093
https://treeherder.mozilla.org/#/jobs?repo=try&revision=47a969433c6a9471ee48ef3839a81abe8c23997b
You need to log in before you can comment on or make changes to this bug.