Closed Bug 1052503 Opened 5 years ago Closed 5 years ago

[Rocketbar] Update loading progress bar

Categories

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

x86
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
2.1 S3 (29aug)
tracking-b2g backlog

People

(Reporter: epang, Assigned: kgrandon)

References

Details

(Keywords: polish, Whiteboard: [systemsfe])

Attachments

(2 files, 1 obsolete file)

Update both indeterminate and determinant progress bars across OS.

Visual design/specs to follow.
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!
Flags: needinfo?(epang)
Let's fix this first to close the user story.
Blocks: 941169
No longer blocks: browser-chrome-mvp
Please be very careful when creating this GIF. High framerate progress bar animations have been a big source of performance problems in the past.
(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
Flags: needinfo?(epang) → needinfo?(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.
Flags: needinfo?(gmarty)
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.
Flags: needinfo?(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?
Flags: needinfo?(gmarty)
(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
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?
Flags: needinfo?(gmarty)
Flags: needinfo?(epang)
Depends on: gaia-progress
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?
Flags: needinfo?(gmarty) → needinfo?(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 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 :)
Flags: needinfo?(epang)
(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.
Flags: needinfo?(kgrandon)
blocking-b2g: --- → backlog
Attached file Github pull request (obsolete) —
Taking this since I'm working on it and the web component.
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
Target Milestone: --- → 2.1 S3 (29aug)
I think this just needs to block the progress bar user story.
No longer blocks: rocketbar-search-mvp
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.
Summary: [System] Update Progress bars across the OS → [Rocketbar] Update loading progress bar
Comment on attachment 8474796 [details] [review]
Github pull request

Anyone have time for a review for this simple one? Thanks!
Attachment #8474796 - Flags: review?(dale)
Attachment #8474796 - Flags: review?(bfrancis)
Comment on attachment 8474796 [details] [review]
Github pull request

Nice!
Attachment #8474796 - Flags: review?(dale)
Attachment #8474796 - Flags: review?(bfrancis)
Attachment #8474796 - Flags: review+
Thanks for the quick review. This is now in master: https://github.com/mozilla-b2g/gaia/commit/546eb9172f7e75603da8b94264d80a44d85e1e5c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1056023
The push prior has just completed its retrigger and is green fwiw:
https://tbpl.mozilla.org/php/getParsedLog.php?id=46358101&tree=B2g-Inbound
(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.
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.
Flags: needinfo?(epang)
(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
(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
Flags: needinfo?(epang)
Depends on: 1058323
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.
Blocks: browser-chrome-mvp
No longer blocks: 941169
Keywords: polish
(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 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
Attachment #8479949 - Flags: review+
Again on try because tests seem to be timing out for some reason: https://tbpl.mozilla.org/?tree=Try&rev=02fc3d28c162
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
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.