1052503 - [Rocketbar] Update loading progress bar

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Search, defect)

Description

[Eric Pang [:epang] UX]

Update both indeterminate and determinant progress bars across OS.

Visual design/specs to follow.

Comment 1

[Kevin Grandon :kgrandon]

Eric - It looks like we currently use a simple gif for this located here: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/shared/progress.gif

I think it makes sense to continue to use a gif for this, could you upload one here when you get a chance? Length does not matter, but if repeated across the x-axis, it should seamlessly loop. Thanks!

Comment 2

[Kevin Grandon :kgrandon]

Let's fix this first to close the user story.

Comment 3

[Ben Francis [:benfrancis]]

Please be very careful when creating this GIF. High framerate progress bar animations have been a big source of performance problems in the past.

Comment 4

[Eric Pang [:epang] UX]

(In reply to Ben Francis [:benfrancis] from comment #3)
> Please be very careful when creating this GIF. High framerate progress bar
> animations have been a big source of performance problems in the past.

Hi, Guillaume has been working on an implementation in the notification tray which should be able to be applied here.  I believe he is using css determinant progress and a png + css for the in-determinant progress bar.  Guillaume, how do you find the performance to be?

Kevin, do you know if there's a way to pull all the locations the progress bars show up?

Thanks!
Eric

Comment 5

[Guillaume Marty [:gmarty]]

I haven't noticed any performance issue from using CSS animation to animate a PNG background on the indeterminate progress bar. That said we need scientific measures. Kevin, any suggestions on how to do it?

The progress bars with known %age are styled in pure CSS and require no image.

Comment 6

[Kevin Grandon :kgrandon]

I'm not sure of the best methodology to measure this TBH, we may want to loop in some gfx guys.

Can you link me to your code? It seems like we should build a simple progress web component and use it in both places.

Comment 7

[Guillaume Marty [:gmarty]]

Sure, you can find the styling in the branch I'm working on here:
https://github.com/gmarty/gaia/commit/6662b12132001dc8cedca449b50ae6829f45918c#diff-482a02d8c8f0d3d3e5279bee5b62e05aR77
The background image is here:
https://mozilla.app.box.com/s/m5r5lj53c87zrwxbn1cs/1/2315915821
The markup is just a regular <progress> element.

Essentially translateX is animated from 0 to the width of the background image.

Do we really need a web component only for styling when the features are in par with the native HTML element?

Comment 8

[Kevin Grandon :kgrandon]

(In reply to gmarty from comment #7)
> Do we really need a web component only for styling when the features are in
> par with the native HTML element?

If we have a UI control that contains some amount of self-contained HTML markup, CSS, and potentially javascript in the future, I think a web component here definitely makes sense. It might be small, but less duplication code ftw :)

I think we should have a gaia-progress component here, and it should live in: /shared/elements/gaia_progress

Comment 9

[Kevin Grandon :kgrandon]

Don't we need progress assets for all resolutions? In the patch and box folder I only see a single progress asset. To prevent this from being pixelated at higher resolutions, I would assume that we would still want a @1.5x, @2x, @2.25x, etc images?

Comment 10

[Guillaume Marty [:gmarty]]

You're absolutely right Kevin, we need an image for higher resolutions.
However, as it is a pattern, we can simply create a taller version of the image (i.e 9 pixels height) and it can then be cut at 4, 6, 8 or 9 pixels with the same visual result.

The other solution is to use pure CSS gradient like this:
background-image: repeating-linear-gradient(135deg, #92F4FE, #92F4FE 50px, #00CDF0 50px, #00CDF0 100px);

It scales across resolutions and I didn't notice any performance issue on a 256Mb Flame. What do you think?

Comment 11

[Eric Pang [:epang] UX]

(In reply to gmarty from comment #10)
> You're absolutely right Kevin, we need an image for higher resolutions.
> However, as it is a pattern, we can simply create a taller version of the
> image (i.e 9 pixels height) and it can then be cut at 4, 6, 8 or 9 pixels
> with the same visual result.
> 
> The other solution is to use pure CSS gradient like this:
> background-image: repeating-linear-gradient(135deg, #92F4FE, #92F4FE 50px,
> #00CDF0 50px, #00CDF0 100px);
> 
> It scales across resolutions and I didn't notice any performance issue on a
> 256Mb Flame. What do you think?

clearing ni since Guillaume answered the question :)

Comment 12

[Kevin Grandon :kgrandon]

(In reply to gmarty from comment #10)
> You're absolutely right Kevin, we need an image for higher resolutions.
> However, as it is a pattern, we can simply create a taller version of the
> image (i.e 9 pixels height) and it can then be cut at 4, 6, 8 or 9 pixels
> with the same visual result.

The image is diagonal so I don't think this is an option as the image would distort.  Also I was mainly concerned about higher pixel densities, we generally need the build system to pick the right @1.5x, @2x, etc image for this.

> The other solution is to use pure CSS gradient like this:
> background-image: repeating-linear-gradient(135deg, #92F4FE, #92F4FE 50px,
> #00CDF0 50px, #00CDF0 100px);
> 
> It scales across resolutions and I didn't notice any performance issue on a
> 256Mb Flame. What do you think?

I think an image is going to be more performant, but CSS would be easier to manage long term.

Comment 13

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Comment 14

[Kevin Grandon :kgrandon]

Taking this since I'm working on it and the web component.

Comment 15

[Kevin Grandon :kgrandon]

I think this just needs to block the progress bar user story.

Comment 16

[Kevin Grandon :kgrandon]

Going to track this for only rocketbar. There's likely other stuff to do here, but the "entire OS" is out of the question at this point. We will likely need to do a system-wide audit to get everything, and I'm not sure if that's possible in 2.0 at this point.

Comment 17

[Kevin Grandon :kgrandon]

Comment on attachment 8474796 [details] [review]
Github pull request

Anyone have time for a review for this simple one? Thanks!

Comment 18

[Ben Francis [:benfrancis]]

Comment on attachment 8474796 [details] [review]
Github pull request

Nice!

Comment 19

[Kevin Grandon :kgrandon]

Thanks for the quick review. This is now in master: https://github.com/mozilla-b2g/gaia/commit/546eb9172f7e75603da8b94264d80a44d85e1e5c

Comment 20

[Ed Morley [:emorley]]

Reverted on suspicion of causing B2G debug emulator xpcshell timeouts like:
https://tbpl.mozilla.org/php/getParsedLog.php?id=46342534&tree=B2g-Inbound

https://tbpl.mozilla.org/?tree=B2g-Inbound&jobname=b2g_emulator_vm%20b2g-inbound%20debug%20test%20xpcshell&tochange=19e59c14b0aa&fromchange=de62aa662be4

https://github.com/mozilla-b2g/gaia/commit/71247126001aee77424f753afdfb0c3832118d9e

Comment 21

[Ed Morley [:emorley]]

The push prior has just completed its retrigger and is green fwiw:
https://tbpl.mozilla.org/php/getParsedLog.php?id=46358101&tree=B2g-Inbound

Comment 22

[Kevin Grandon :kgrandon]

(In reply to Ed Morley [:edmorley] from comment #20)
> Reverted on suspicion of causing B2G debug emulator xpcshell timeouts like:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=46342534&tree=B2g-Inbound

I think we have confirmed that this did not cause the xpc crashes, but that was caused by bug 1049240. None the less, this seems to cause a performance regression (bug 1056023), so probably good it's backed out until we investigate that.

Comment 23

[Kevin Grandon :kgrandon]

Eric - turns out the CSS transforms likely wont play too well with performance and we need a gif for this. Can you get me the updated asset for the existing gif here: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/shared/progress.gif

I assume that we will likely need @1.5x, @2x, @2.25x versions of this as well. Perhaps we could try initially with a repeating version of a smaller-width one.

Comment 24

[Ed Morley [:emorley]]

(In reply to Kevin Grandon :kgrandon from comment #22)
> (In reply to Ed Morley [:edmorley] from comment #20)
> > Reverted on suspicion of causing B2G debug emulator xpcshell timeouts like:
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=46342534&tree=B2g-Inbound
> 
> I think we have confirmed that this did not cause the xpc crashes, but that
> was caused by bug 1049240. None the less, this seems to cause a performance
> regression (bug 1056023), so probably good it's backed out until we
> investigate that.

I don't think this is correct, bug 1049240 was falsely blamed.

The revert of this bug made the xpcshell job pass again:
https://tbpl.mozilla.org/?tree=B2g-Inbound&jobname=b2g_emulator_vm%20b2g-inbound%20debug%20test%20xpcshell&fromchange=7624f45e8b79&tochange=19c7d0acb8c6

ie this cset:
https://hg.mozilla.org/integration/b2g-inbound/rev/2cdd333337e9

Comment 25

[Eric Pang [:epang] UX]

(In reply to Kevin Grandon :kgrandon from comment #23)
> Eric - turns out the CSS transforms likely wont play too well with
> performance and we need a gif for this. Can you get me the updated asset for
> the existing gif here:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/shared/
> progress.gif
> 
> I assume that we will likely need @1.5x, @2x, @2.25x versions of this as
> well. Perhaps we could try initially with a repeating version of a
> smaller-width one.

Hey Kevin

I've created gif version of the progress indicators.  They can be found on box here:
https://mozilla.box.com/s/o9y1vczu97ehfwtpgnhz

Let me know if they work better. Thx

Comment 26

[Kevin Grandon :kgrandon]

Attachment: Pull request - reland loading indicator

This is a rebased pull request to re-use the web component. Carrying the R+ as it's the same patch, and we're just changing the web component.

Comment 27

[Kevin Grandon :kgrandon]

Attachment: Pull Request - Update gaia-progress to use steps() for animation

Comment 28

[Kevin Grandon :kgrandon]

(In reply to Ed Morley [:edmorley] from comment #20)
> Reverted on suspicion of causing B2G debug emulator xpcshell timeouts like:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=46342534&tree=B2g-Inbound
> 
> https://tbpl.mozilla.org/?tree=B2g-Inbound&jobname=b2g_emulator_vm%20b2g-
> inbound%20debug%20test%20xpcshell&tochange=19e59c14b0aa&fromchange=de62aa662b
> e4
> 
> https://github.com/mozilla-b2g/gaia/commit/
> 71247126001aee77424f753afdfb0c3832118d9e

Testing on try to verify timeouts: https://tbpl.mozilla.org/?tree=Try&rev=684521361f9b

Comment 29

[Kevin Grandon :kgrandon]

Comment on attachment 8479949 [details] [review]
Pull Request - Update gaia-progress to use steps() for animation

Tiny css fix for using steps instead, so R=me on this part.

Landed in master: https://github.com/mozilla-b2g/gaia/commit/7f2100f5aa4fc2fd2dc4682ac722f29337f0d54e

Comment 30

[Kevin Grandon :kgrandon]

Again on try because tests seem to be timing out for some reason: https://tbpl.mozilla.org/?tree=Try&rev=02fc3d28c162

Comment 31

[Kevin Grandon :kgrandon]

TBPL run on XPC shell test times now look normal. The new reduced framerate should make us good here, and we can tweak it in the future. Re-landing and will monitor arewefastyet:

https://github.com/mozilla-b2g/gaia/commit/c366b0e21c4919a95555dfc37fe57abe41feddf2