1352119 - Implement new tab loading/progress indicator animation

Status: VERIFIED FIXED

(Firefox :: General, enhancement, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

UX is working on some variations to change the page loading indicator. This bug will be updated once a variation is selected.

Comment 1

[Yunier J.]

I like the new tabs loading animation publish here (https://bug1352068.bmoattachments.org/attachment.cgi?id=8852908), always I wanted only one loading indicator.

I think that last effect is more appropriate for restoring.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Marking this as 13-points due to the related perf issues that may be encountered (see bug 1352085 which was filed after bug 759252 was fixed).

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Toolbar - Refresh.mp4

Please only pay attention to the tabbar in this spec (ignore the stop/refresh icon) and the painting of the page.

Comment 8

[Will]

Please consider this. I think it would make Firefox feel faster and is overall the better way https://bugzilla.mozilla.org/show_bug.cgi?id=1355978

Comment 9

[Eric Pang [:epang] UX]

Attachment: Loading Indicator.zip

SVG sprites of the loading indicator.  Included a blue and grey version just incase.

Comment 10

[Jim Porter (:squib)]

Attachment: Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review commit: https://reviewboard.mozilla.org/r/138108/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138108/

Comment 11

[Tim Nguyen :ntim]

Attachment: Optimized svg (multiple paths)

Used SVGO to optimize according to the SVG guidelines:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines

Comment 12

[Tim Nguyen :ntim]

Attachment: Optimized svg (single path)

Comment 13

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138108/diff/1-2/

Comment 14

[Eric Pang [:epang] UX]

Hey Jim,

There was an update to the Photon colour palette. 
Can you update the colours of the ball.

D7D7DB - grey
0A84FF - blue

Thanks!

Comment 15

[Eric Pang [:epang] UX]

Also the burst should be the same blue #0A84FF @50% opacity.
Then fade to 0% as it approaches the edges.

Comment 16

[Jim Porter (:squib)]

Just an update on where I'm at with this: I think I have the loading indicator completely working now, but I'm trying out a few things to get the "burst" to work properly. This is turning out to be quite difficult, since the CSS features that I could use to do this easily aren't supported in Firefox yet.

While I'm here though... :epang, how hard would it be for you to export an SVG of the loading indicator at half the size it currently is? I think we discussed this at the work week, but the SVG I have is currently twice the size we want it to be.

Comment 17

[Jim Porter (:squib)]

I had an epiphany after thinking about this for a while, and I think I've come up with a way to make the burst work correctly. The issue was that we want the burst to take the same amount of time to fill up a long tab as a short one. However, you can't use CSS `transform: scale(calc(expr))`, which made it impossible to do this by changing the scale. Instead, I'm doing this now by changing the initial size of the burst using background-size. I'll need a new SVG image that's just a small circle (it can be any size since I'm explicitly scaling it), but maybe I can figure out how to make that on my own.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

<svg viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg">
  <circle cx="100" cy="100" r="100"/>
</svg>

That would give you a 100px by 100px circle inside of a 200px by 200px viewbox. Feel free to change those values as you wish, as well as adding in fill="context-fill" to allow CSS to set the fill color.

Comment 19

[Eric Pang [:epang] UX]

Attachment: SVG + JSON.zip

Updated JSON and SVG file sized.

Comment 20

[Eric Pang [:epang] UX]

Comment on attachment 8874543 [details]
SVG + JSON.zip

I opened a new UX bug to hold the svg and json files as well as the in-context motion. (since that's what we're doing for all the other bugs)

https://bugzilla.mozilla.org/show_bug.cgi?id=1370331

Comment 21

[Jim Porter (:squib)]

Attachment: Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review commit: https://reviewboard.mozilla.org/r/146890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146890/

Comment 22

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138108/diff/2-3/

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Hey squib, epang, I wanted to give you a heads up on something.

I've been testing these patches against the Talos benchmarks over the past few days, and the patches as they are introduce some performance regressions. I've been investigating these pre-emptively, as I know we want to land this things, and I don't want us all to get stuck in a quagmire of Talos goop.

The one thing I've noticed that's costing us a bit is the "fill" transition from grey to #0A84FF on .tab-throbber[busy]::before  to .tab-throbber[progress]::before. Each frame of this transition requires us to paint on the main thread, and that's causing some of our tests to notice.

There are a few ways I can think of to address this:

a) Have no colour transition between the busy and progress states
b) Implement the busy and progress states so that there are two images overlapping - one with fill grey, one with fill #0A84FF. When we reach the progress state, transition the opacity of both images to cross fade between them. See [1] for details.

I'm still trying to determine where the remaining regression lies, but the repainting is a big piece. Just an FYI while we're still implementing here.

[1]: https://ariya.io/2014/02/tricks-for-gpu-composited-css

Comment 24

[Jim Porter (:squib)]

> The one thing I've noticed that's costing us a bit is the "fill" transition from grey to #0A84FF on .tab-throbber[busy]::before  to .tab-throbber[progress]::before. Each frame of this transition requires us to paint on the main thread, and that's causing some of our tests to notice.

Is this a fundamental limitation of transitioning on `fill`, or is it just temporary? In either case, we could probably drop the transition since it's pretty subtle anyway. It'd be nice to have, but if it's slow, I'd rather just remove it instead of adding extra complexity.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jim Porter (:squib) from comment #24)
> > The one thing I've noticed that's costing us a bit is the "fill" transition from grey to #0A84FF on .tab-throbber[busy]::before  to .tab-throbber[progress]::before. Each frame of this transition requires us to paint on the main thread, and that's causing some of our tests to notice.
> 
> Is this a fundamental limitation of transitioning on `fill`, or is it just
> temporary?

It's a limitation until WebRender comes around, I believe, since fill colour is just not something that we can currently accelerate on the GPU. So as soon as we start to transition on it, all paints occur on the main thread.

Comment 26

[Jonathan Watt [:jwatt]]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review152648

Why is loading.svg to incredibly complex? In the mockup videos I've seen there is only a circle animating left and right, so it's not clear to me what this file is for (it doesn't display anything when loaded directly in a top level window).

::: browser/themes/shared/tabs.inc.css:107
(Diff revision 3)
> +  -moz-context-properties: fill;
> +}
> +
> +.tab-throbber[busy]::before {
> +  animation: tab-throbber-pulse 1.05s steps(60) infinite;
> +  transition: fill 0.166s;

Every frame will cause imagelib to load a new instance of the image with a different fill color, so animating 'fill' when 'fill' is an image context property is likely to be a performance issue.

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review153006

::: browser/base/content/tabbrowser.css:23
(Diff revision 3)
> -.tab-icon-image:not([src]):not([pinned]):not([crashed])[selected],
> -.tab-icon-image:not([src]):not([pinned]):not([crashed]):not([sharing]),
> +.tab-icon-image:not([src]):not([pinned]):not([crashed]):not([busy])[selected],
> +.tab-icon-image:not([src]):not([pinned]):not([crashed]):not([busy]):not([sharing]),
> -.tab-icon-image[busy],
> -.tab-throbber:not([busy]),
>  .tab-icon-sound:not([soundplaying]):not([muted]):not([blocked]),
>  .tab-icon-sound[pinned],
>  .tab-sharing-icon-overlay,
>  .tab-icon-overlay {
>    display: none;
>  }
>  
> +.tab-icon-image[busy],
> +.tab-throbber:not([busy]) {
> +  opacity: 0;
> +}
> +
> +.tab-icon-image:not([busy]) {
> +  transition: opacity 0.166s;
> +}
> +

Is it at all possible to avoid this change?

My local testing and profiling indicates that this is a source of Talos regression for at least ts_paint, and probably a few other tests as well, for the following reasons:

1. Because tab-throbber is no longer display:none, but opacity: 0 instead, this means that the loading.svg is loaded, parsed and rasterized even when it's not about to be used.
2. Changing the opacity of things like the favicons can definitely be accelerated on the GPU, but once the transition is _done_, our layerization code will determine that the favicon no longer requires its own layer, and will sink it back into its background layer - which includes the text for the tab. And then we re-paint it, which is potentially expensive.

For (2), we can avoid this entirely by not transitioning on opacity, and keeping the original display: none rule, _or_ we might be able to tell Gecko to keep favicons on their own layers, at the expense of keeping that layer around (and potential fallout).

My recommendation at this time is to avoid the opacity transition on the favicons and throbber, and to open a follow-up bug to try to reintroduce them (while working with our Graphics team to try to ensure we don't regress performance).

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review153008

::: browser/themes/shared/tabs.inc.css:107
(Diff revision 3)
> +  -moz-context-properties: fill;
> +}
> +
> +.tab-throbber[busy]::before {
> +  animation: tab-throbber-pulse 1.05s steps(60) infinite;
> +  transition: fill 0.166s;

Stated this earlier, but making it more official now; transitioning the fill can only occur on the main thread, so we should avoid this for now.

Would it be possible to drop this fill transition between the busy and progress states, and file a follow-up to try to re-add it?

Comment 29

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jonathan Watt [:jwatt] from comment #26)
> Why is loading.svg to incredibly complex? In the mockup videos I've seen
> there is only a circle animating left and right, so it's not clear to me
> what this file is for (it doesn't display anything when loaded directly in a
> top level window).
> 

I suspect this is because the bouncing ball is being subtle-y transformed as it bounces (it's being squeezed along the y axis, and growing along the x axis). Pasting the mark-up inside SVGOMG[1] might help more easily visualize what's happening.

[1]: https://jakearchibald.github.io/svgomg/

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Are there other tools or techniques that you can recommend that could help us reduce the complexity of the image?

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

I agree, we should drop the fill transition as well as the opacity transition since it's only supposed to show one frame (.166s) and otherwise will not be so noticeable.

Comment 32

[Jim Porter (:squib)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #31)
> I agree, we should drop the fill transition as well as the opacity
> transition since it's only supposed to show one frame (.166s) and otherwise
> will not be so noticeable.

I'm fine with dropping the transition, but to be precise, .166s is 10 frames (assuming 60 Hz).

Comment 33

[Tim Nguyen :ntim]

(In reply to Mike Conley (:mconley) from comment #29)
> (In reply to Jonathan Watt [:jwatt] from comment #26)
> > Why is loading.svg to incredibly complex? In the mockup videos I've seen
> > there is only a circle animating left and right, so it's not clear to me
> > what this file is for (it doesn't display anything when loaded directly in a
> > top level window).
> > 
> 
> I suspect this is because the bouncing ball is being subtle-y transformed as
> it bounces (it's being squeezed along the y axis, and growing along the x
> axis). Pasting the mark-up inside SVGOMG[1] might help more easily visualize
> what's happening.
> 
> [1]: https://jakearchibald.github.io/svgomg/

The SVG in the patch has tons of unnecessary stuff:
- -moz-user-select: none
- the <g> tags
- preserveAspectRatio attributes
- all the clipPaths are useless
- style="width: 100%; height: 100%;"
- we can unwrap all the svg tags
- the transforms can be flattened onto the paths
- I'm seeing some invisible paths as well (no stroke and no fill):
Example:
stroke-linecap="butt" stroke-linejoin="miter" fill-opacity="0" stroke-miterlimit="4" stroke="rgb(77,77,77)" stroke-opacity="1" stroke-width="0"

stroke-width=0 removes the stroke, which makes all the other stroke attributes useless. fill-opacity=0 removes the fill. So this is an invisible path.


FYI, I've solved all these issues in attachment 8866553 [details] and attachment 8866552 [details] using SVGO and some extra scripting magic. One of them has all the paths combined, one of them keeps each frame in a separate path. Let me know if you have a newer svg you'd like me to optimize.

Comment 34

[Mike Conley (:mconley) (:⚙️)]

(In reply to Tim Nguyen :ntim from comment #33)
> 
> FYI, I've solved all these issues in attachment 8866553 [details] and
> attachment 8866552 [details] using SVGO and some extra scripting magic. One
> of them has all the paths combined, one of them keeps each frame in a
> separate path. Let me know if you have a newer svg you'd like me to optimize.

Wow, that is a _heck_ of a lot simpler. Thanks ntim! I'd be eager to know what scripting you used exactly to clean up the SVG. We should probably put the SVGO and script you used into our asset creation pipeline.

Hey epang, is the SVG in attachment 8866553 [details] up to date with any recent changes you've done with the spritesheet?

Comment 35

[Jim Porter (:squib)]

(In reply to Tim Nguyen :ntim from comment #33)
> FYI, I've solved all these issues in attachment 8866553 [details] and
> attachment 8866552 [details] using SVGO and some extra scripting magic. One
> of them has all the paths combined, one of them keeps each frame in a
> separate path. Let me know if you have a newer svg you'd like me to optimize.

The SVG in the current patch has been updated with the new source image from bug 1370331. (It's a bit smaller and has slightly fewer frames of animation.)

Comment 36

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jim Porter (:squib) from comment #35)
> The SVG in the current patch has been updated with the new source image from
> bug 1370331. (It's a bit smaller and has slightly fewer frames of animation.)

Excellent, thanks.

In that case, ntim, would you be willing to run the new SVG through your simplification steps? Or (even better), provide some instructions on how to perform those steps ourselves?

Comment 37

[Tim Nguyen :ntim]

(In reply to Mike Conley (:mconley) from comment #36)
> (In reply to Jim Porter (:squib) from comment #35)
> > The SVG in the current patch has been updated with the new source image from
> > bug 1370331. (It's a bit smaller and has slightly fewer frames of animation.)
> 
> Excellent, thanks.
> 
> In that case, ntim, would you be willing to run the new SVG through your
> simplification steps? Or (even better), provide some instructions on how to
> perform those steps ourselves?

Here are the steps:
- Run through SVGO (which you've already done)
- Open the SVG in Firefox and the web console on that page
- Run:
[...$$("svg > svg")].forEach(e => {e.removeAttribute("preserveAspectRatio");e.removeAttribute("viewBox");e.removeAttribute("style")});
[...$$("[fill='none'], defs")].forEach(e => e.remove());

- Copy the Outer HTML (Right click on the root node > Copy > Outer HTML)
- Paste into SVGO, save the SVG and Refresh Firefox

- Run this into the console:

[...$$("svg > g > path")].forEach(e => {
  let root = document.rootElement;
  let wrappingSVG = e.parentNode.parentNode;
  e.setAttribute("transform", `translate(${wrappingSVG.getAttribute("x")})`);
  root.appendChild(e);
  wrappingSVG.remove();
});
document.rootElement.setAttribute("fill", "#D7D7DA");[...$$("path")].forEach(e => e.removeAttribute("fill"));

- Remove any remaining junk (there should be a style="-moz-user-select: none" and transform="translate(null)" that prevent SVGO from optimizing properly)
- Copy outer HTML and paste into SVGO
- DONE!

Make sure SVGO has Prettify code on, and uses an indent of 2 spaces.

SVGO is not smart enough to do all this stuff by itself, but I think sfoster is working on some SVGO plugins that do this: https://github.com/sfoster/svgo/tree/photon-plugins

Comment 38

[Tim Nguyen :ntim]

Attachment: Optimized SVG

Comment 39

[Tim Nguyen :ntim]

Note that the steps in comment 37 don't necessarily work for all SVGs, you have to do some manual analysis first to see what's useful and what's not, but the issues mentioned in comment 33 are very common pitfalls that are not caught by SVGO.

Comment 40

[Jonathan Watt [:jwatt]]

(In reply to Mike Conley (:mconley) from comment #30)
> Are there other tools or techniques that you can recommend that could help
> us reduce the complexity of the image?

SVGO + plugins seems like the way to go.

Regarding whether the frames are merged into a single path or individual paths, it comes down to memory vs speed. A single path will require slightly less memory but individual paths may load slightly more quickly. I'm not sure there will be much in it.

Comment 41

[Mozilla Autoland]

hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 3 changesets with 6 changes to 6 files (+1 heads)
remote: autoland push detected
remote: recorded push in pushlog
remote: replication log not available; cannot close transaction
remote: transaction abort!
remote: rolling back pushlog
remote: rollback completed
remote: pretxnclose.vcsreplicator hook failed
abort: push failed on remote

Comment 42

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138108/diff/3-4/

Comment 43

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/1-2/

Comment 44

[Tim Nguyen :ntim]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review155288

::: browser/themes/shared/tabs.inc.css:108
(Diff revisions 3 - 4)
> +  /* XXX: It would be nice to transition between the "connecting" color and the
> +     "loading" color; however, that currently forces main thread painting. When
> +     this is fixed, we can do something like `transition: fill 0.166s;` */
> +
>    /* FIXME: The spec has this as #D7D7D8 but this doesn't show up well with the
>       current theme. */
>    fill: grey;

I wonder if filter: grayscale(100%) (instead of fill: gray) would make the transition cost less.

Comment 45

[Markus Stange [:mstange]]

It would not. Filter animations run on the main thread and require repaints. (bug 869496)

Comment 46

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review155300

::: browser/themes/shared/tabbrowser/loading-burst.svg:1
(Diff revision 2)
> +<svg viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg">
> +  <circle cx="50" cy="50" r="50" fill="context-fill"/>
> +</svg>

Looking at the layers being sent over via LayerScope, this SVG ends up being at the max size (scale(20);) went uploaded to the compositor, which (I think) causes the compositor to do a bit more work.

This is at least partially contributing to the tsvgx regression I'm dealing with in bug 1357093. Instead of scaling up a large circle, would it be possible to translateX a rectangle that is the same size as the tab itself?

Comment 47

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138108/diff/4-5/

Comment 48

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/2-3/

Comment 49

[Jim Porter (:squib)]

(In reply to Mike Conley (:mconley) from comment #46)
> This is at least partially contributing to the tsvgx regression I'm dealing
> with in bug 1357093. Instead of scaling up a large circle, would it be
> possible to translateX a rectangle that is the same size as the tab itself?

That would look pretty weird for pinned tabs and slightly-weird for min-width tabs. Would it help if I reduced the "native" size of the SVG? I picked 100x100 arbitrarily.

Comment 50

[Mike Conley (:mconley) (:⚙️)]

If I understand correctly, our graphics layer will calculate the size of the rasterized layer to send to the compositor based on the final size of something that's being composited. So, yes, reducing the initial size of the circle will help a bit, but that SVG, scaled by 20x, will still be quite a bit larger than the area we're trying to show the effect in. The more we can pare that down, probably the better.

Comment 51

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review155900

::: browser/base/content/tabbrowser.css:32
(Diff revision 5)
> +.tab-icon-image[busy],
> +.tab-throbber:not([busy]) {
> +  visibility: hidden;
> +  opacity: 0;
> +}

I'm pretty sure even with visibility:hidden, this will still cause the SVG to be loaded, parsed, and its document constructed in memory. Is there no way to keep this display:none until we absolutely need it?

Comment 52

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review156578

::: browser/base/content/tabbrowser.css:32
(Diff revision 5)
> +.tab-icon-image[busy],
> +.tab-throbber:not([busy]) {
> +  visibility: hidden;
> +  opacity: 0;
> +}

Yeah, I agree with mconley. These should be moved back to display:none

::: browser/base/content/tabbrowser.css:39
(Diff revision 5)
> +  visibility: hidden;
> +  opacity: 0;
> +}
> +
> +.tab-icon-image:not([busy]) {
> +  transition: visibility 0s, opacity 0.166s;

We don't need to transition on visibility, especially since its duration is 0s.

::: browser/themes/shared/tabs.inc.css:90
(Diff revision 5)
> +  margin-inline-start: 1px;
> +  margin-inline-end: calc(1px - 14px);

Where do these values come from? And why are they needed?

Also, why do we need calc(1px - 14px) here? Wouldn't -13px be the same?

::: browser/themes/shared/tabs.inc.css:97
(Diff revision 5)
> +}
> +
> +.tab-throbber::before {
> +  content: "";
> +  position: absolute;
> +

nit, please remove this blank line.

::: browser/themes/shared/tabs.inc.css:98
(Diff revision 5)
> +
> +.tab-throbber::before {
> +  content: "";
> +  position: absolute;
> +
> +  background: url("chrome://browser/skin/tabbrowser/loading.svg") left center no-repeat;

Please use the longhand version of each background property, as the background shorthand will set other properties that you may not have intended such as background-color.

::: browser/themes/shared/tabs.inc.css:101
(Diff revision 5)
> +  position: absolute;
> +
> +  background: url("chrome://browser/skin/tabbrowser/loading.svg") left center no-repeat;
> +  width: 840px;
> +  height: 100%;
> +

nit, please remove this blank line.

::: browser/themes/shared/tabs.inc.css:112
(Diff revision 5)
> +  /* FIXME: The spec has this as #D7D7D8 but this doesn't show up well with the
> +     current theme. */
> +  fill: grey;

I think we should ask UX for a sprite that includes a 1px stroke on the ball so it is always visible. Themes may allow for a grey tab background and this current approach won't work for that either.

Comment 53

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review156586

::: browser/base/content/tabbrowser.xml:7255
(Diff revision 3)
>                      class="tab-background-middle"/>
>            <xul:hbox xbl:inherits="pinned,selected=visuallyselected"
>                      class="tab-background-end"/>
>          </xul:hbox>
> -        <xul:hbox xbl:inherits="pinned,selected=visuallyselected,titlechanged,attention"
> +        <xul:hbox xbl:inherits="busy" class="tab-burst"/>
> +        <xul:hbox xbl:inherits="pinned,selected=visuallyselected,titlechanged,attention,busy"

Why does .tab-content need to inherit "busy" now? I don't see it used in the rest of this patch.

::: browser/themes/shared/tabs.inc.css:124
(Diff revision 3)
>  }
>  
> +.tab-burst {
> +  position: relative;
> +  overflow: hidden;
> +  margin-top: 2px;

Why is this margin-top needed?

::: browser/themes/shared/tabs.inc.css:130
(Diff revision 3)
> +  width: 10%;
> +  height: 100%;
> +  left: calc(17px - 5%);

How does the width:10% and left:calc(17px - 5%) work together? Can we just have width and height set to 16px and not need left? Or at least set left to 8px (half of 16px)?

::: browser/themes/shared/tabs.inc.css:133
(Diff revision 3)
> +  content: "";
> +  position: absolute;
> +  width: 10%;
> +  height: 100%;
> +  left: calc(17px - 5%);
> +  background: url("chrome://browser/skin/tabbrowser/loading-burst.svg") center center no-repeat;

Please use longhand for each background-* property you want to set.

::: browser/themes/shared/tabs.inc.css:135
(Diff revision 3)
> +  width: 10%;
> +  height: 100%;
> +  left: calc(17px - 5%);
> +  background: url("chrome://browser/skin/tabbrowser/loading-burst.svg") center center no-repeat;
> +  background-size: 100% auto;
> +

nit, please remove this blank line.

::: browser/themes/shared/tabs.inc.css:137
(Diff revision 3)
> +  left: calc(17px - 5%);
> +  background: url("chrome://browser/skin/tabbrowser/loading-burst.svg") center center no-repeat;
> +  background-size: 100% auto;
> +
> +  -moz-context-properties: fill;
> +  fill: #0A84FF;

Since this color is shared between the two patches in this bug, we should move it to a CSS variable so it can be shared and kept in sync.

::: browser/themes/shared/tabs.inc.css:141
(Diff revision 3)
> +  -moz-context-properties: fill;
> +  fill: #0A84FF;
> +}
> +
> +.tab-burst[busy]::before {
> +  visibility: hidden;

Can we set this to display:none instead of visiblity:hidden?

::: browser/themes/shared/tabs.inc.css:145
(Diff revision 3)
> +.tab-burst[busy]::before {
> +  visibility: hidden;
> +}
> +
> +.tab-burst:not([busy])::before {
> +  transition: opacity 5s var(--animation-easing-function), transform 5s var(--animation-easing-function);

This seems like really long transitions for opacity and tranform. Are these the final values?

Comment 54

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138108/diff/5-6/

Comment 55

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/3-4/

Comment 56

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review155300

> Looking at the layers being sent over via LayerScope, this SVG ends up being at the max size (scale(20);) went uploaded to the compositor, which (I think) causes the compositor to do a bit more work.
> 
> This is at least partially contributing to the tsvgx regression I'm dealing with in bug 1357093. Instead of scaling up a large circle, would it be possible to translateX a rectangle that is the same size as the tab itself?

I shrunk the source SVG. Hopefully that'll help.

Comment 57

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review156586

> Why does .tab-content need to inherit "busy" now? I don't see it used in the rest of this patch.

Removed.

> Why is this margin-top needed?

It's necessary so that it doesn't overlap the tab's top border, which looks weird. I could add a comment if you like.

> How does the width:10% and left:calc(17px - 5%) work together? Can we just have width and height set to 16px and not need left? Or at least set left to 8px (half of 16px)?

I added a comment explaining why I do this.

> Please use longhand for each background-* property you want to set.

Fixed.

> nit, please remove this blank line.

Fixed.

> Can we set this to display:none instead of visiblity:hidden?

Fixed.

> This seems like really long transitions for opacity and tranform. Are these the final values?

I shortened them a bit, but anything much shorter seems too fast. I'll let UX see what they think when the try build finishes.

Comment 58

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review153006

> Is it at all possible to avoid this change?
> 
> My local testing and profiling indicates that this is a source of Talos regression for at least ts_paint, and probably a few other tests as well, for the following reasons:
> 
> 1. Because tab-throbber is no longer display:none, but opacity: 0 instead, this means that the loading.svg is loaded, parsed and rasterized even when it's not about to be used.
> 2. Changing the opacity of things like the favicons can definitely be accelerated on the GPU, but once the transition is _done_, our layerization code will determine that the favicon no longer requires its own layer, and will sink it back into its background layer - which includes the text for the tab. And then we re-paint it, which is potentially expensive.
> 
> For (2), we can avoid this entirely by not transitioning on opacity, and keeping the original display: none rule, _or_ we might be able to tell Gecko to keep favicons on their own layers, at the expense of keeping that layer around (and potential fallout).
> 
> My recommendation at this time is to avoid the opacity transition on the favicons and throbber, and to open a follow-up bug to try to reintroduce them (while working with our Graphics team to try to ensure we don't regress performance).

I've removed most of these changes and now things will hopefully be a bit better. We can always remove the opacity transition later too if we want. I don't know how noticeable it would be unless you're staring at it.

Comment 59

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review153008

> Stated this earlier, but making it more official now; transitioning the fill can only occur on the main thread, so we should avoid this for now.
> 
> Would it be possible to drop this fill transition between the busy and progress states, and file a follow-up to try to re-add it?

Removed this and added a comment explaining why we might want to re-add it later.

Comment 60

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review156578

> We don't need to transition on visibility, especially since its duration is 0s.

Fixed. That was some test code that leaked through.

> Where do these values come from? And why are they needed?
> 
> Also, why do we need calc(1px - 14px) here? Wouldn't -13px be the same?

I changed this around a bit. Since the loading throbber is only 14x14 px and favicons are 16x16, I added a left margin of 1px so it's correctly centered.

> I think we should ask UX for a sprite that includes a 1px stroke on the ball so it is always visible. Themes may allow for a grey tab background and this current approach won't work for that either.

I don't know if we'll need to do that once the new tab visuals land. It should look fine then. Adding a stroke might help, but it could look weird in some cases...

Comment 61

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jim Porter (:squib) from comment #60)
> I changed this around a bit. Since the loading throbber is only 14x14 px and
> favicons are 16x16, I added a left margin of 1px so it's correctly centered.

Could we have UX provide a 16x16 loading throbber?

> > I think we should ask UX for a sprite that includes a 1px stroke on the ball so it is always visible. Themes may allow for a grey tab background and this current approach won't work for that either.
> 
> I don't know if we'll need to do that once the new tab visuals land. It
> should look fine then. Adding a stroke might help, but it could look weird
> in some cases...

Okay, that sounds fine to me. We can move forward as-is and if we need to add a stroke later we can make that change then.

Comment 62

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138108/diff/6-7/

Comment 63

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/4-5/

Comment 64

[Jim Porter (:squib)]

Ok, I have a (mostly) finished build being produced here: <https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-c634715831f2899dbb8a49c00fa993bc16c73f96/>. You can try it out and make sure that things look good. In particular, look at how fast the animations move since I just picked some random values that seem good for most of the animations. I can make pretty much anything faster/slower as needed.

Comment 65

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/5-6/

Comment 66

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

This is essentially ready. I have a couple small tweaks I want to make, but they should be simple.

Comment 67

[Dão Gottwald [:dao]]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review159860

::: browser/themes/shared/tabs.inc.css:109
(Diff revision 7)
> +.tab-throbber[busy]::before {
> +  animation: tab-throbber-pulse 1.05s steps(60) infinite;
> +
> +  /* XXX: It would be nice to transition between the "connecting" color and the
> +     "loading" color; however, that currently forces main thread painting. When
> +     this is fixed, we can do something like `transition: fill 0.166s;` */

This isn't going to be fixed, it's a known consequence of using context-fill and animating fill.

::: browser/themes/shared/tabs.inc.css:149
(Diff revision 7)
> +}
> +
> +@keyframes tab-icon-image-fade-in {
> +  0% { opacity: 0; }
> +  100% { opacity: 1; }
> +}

Why is this an animation rather than a transition?

::: browser/themes/shared/tabs.inc.css:161
(Diff revision 7)
> +.tab-label-container {
> +  padding-inline-start: 22px;
> +}
> +
> +.tab-icon-image:not([src]):not([busy]):not([pinned]) ~ .tab-label-container > .tab-label {
> +  transform: translateX(-22px);

This forces grayscale AA. We need to keep sub-pixel AA enabled.

Btw, this patch doesn't apply cleanly. You need to rebase.

Comment 68

[Dão Gottwald [:dao]]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review159864

::: browser/base/content/tabbrowser.xml:7286
(Diff revision 6)
>            <xul:hbox xbl:inherits="pinned,selected=visuallyselected"
>                      class="tab-background-middle"/>
>            <xul:hbox xbl:inherits="pinned,selected=visuallyselected"
>                      class="tab-background-end"/>
>          </xul:hbox>
> +        <xul:hbox anonid="tab-burst" class="tab-burst"/>

Rename to tab-loading-burst?

::: browser/base/content/tabbrowser.xml:7332
(Diff revision 6)
>        <constructor><![CDATA[
>          if (!("_lastAccessed" in this)) {
>            this.updateLastAccessed();
>          }
> +
> +        this.mTabBurst.addEventListener("animationend", (event) => {

Please use a <handler>

::: browser/base/content/tabbrowser.xml:7537
(Diff revision 6)
> +        <body><![CDATA[
> +          if (Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {
> +            // Ensure that we restart the animation even if it's already playing.
> +            if (this.mTabBurst.classList.contains("active")) {
> +              this.mTabBurst.classList.remove("active");
> +              this.mTabBurst.clientWidth;

You probably want a style flush here rather than a full layout flush.

::: browser/base/content/tabbrowser.xml:7539
(Diff revision 6)
> +            // Ensure that we restart the animation even if it's already playing.
> +            if (this.mTabBurst.classList.contains("active")) {
> +              this.mTabBurst.classList.remove("active");
> +              this.mTabBurst.clientWidth;
> +            }
> +            this.mTabBurst.classList.add("active");

Please set an attribute on the tab and inherit it to the tab-burst node using xbl:inherits.

Comment 69

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review159998

Comment 70

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138108/diff/7-8/

Comment 71

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/6-7/

Comment 72

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review160102

Looks good overall. I think these comments should help you get to the final version of the patch.

::: browser/themes/shared/tabbrowser/loading.svg:1
(Diff revision 8)
> +<svg xmlns="http://www.w3.org/2000/svg" width="960" height="16" fill="context-fill">

We've been adding license headers to these. Please add one here.

> <!-- This Source Code Form is subject to the terms of the Mozilla Public
>    - License, v. 2.0. If a copy of the MPL was not distributed with this
>    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

::: browser/themes/shared/tabs.inc.css:112
(Diff revision 8)
> +  /* FIXME: The spec has this as #D7D7D8 but this doesn't show up well with the
> +     current theme. */
> +  fill: grey;

To fix this you should use:
fill: var(--toolbarbutton-icon-fill);
opacity: .7;

Then also add the following rule:

toolbar[brighttext] .tab-throbber[busy]::before {
  fill: var(--toolbarbutton-icon-fill-inverted);
}

I'm not exactly sure what to do for selected tabs, because I see in some themes we show dark text in the selected tab (default theme on Windows 10) but in others we show white text in the selected tab (Space Fantasy). We could probably use:
:root:-moz-lwtheme .tab-throbber[busy]::before {
  fill: var(--lwt-text-color);
}

::: browser/themes/shared/tabs.inc.css:125
(Diff revision 8)
> +  margin-inline-end: -16px;
> +}
> +
> +/* FIXME: I don't understand why these two rules are necessary, but it makes
> +   pinned tabs look ok still. Look into this more... */
> +
> +.tab-icon-image[pinned] {
>    margin-inline-end: 6px;
>  }
>  
> +.tab-throbber[pinned] {
> +  margin-inline-end: 7px;

I think you only want this margin-line-end:-16px for non-pinned tabs.

And then I think you can remove these two .tab-icon-image[pinned] and .tab-throbber[pinned] rules, while also removing the rule at http://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/browser/themes/shared/tabs.inc.css#69

::: browser/themes/shared/tabs.inc.css:144
(Diff revision 8)
> +  animation: tab-icon-image-fade-in 0.333s;
> +}
> +
> +@keyframes tab-icon-image-fade-in {
> +  0% { opacity: 0; }
> +  100% { opacity: 1; }

I agree with Dao, this looks like it can be made to use CSS transitions instead of animation.

::: browser/themes/shared/tabs.inc.css:162
(Diff revision 8)
> +.tab-label-container {
> +  padding-inline-start: 22px;
> +}
> +
> +.tab-icon-image:not([src]):not([busy]):not([pinned]) ~ .tab-label-container > .tab-label {
> +  transform: translateX(-22px);

Can you use margin-inline-start: -22px instead? I don't think that will affect antialiasing.

Comment 73

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review159860

> This isn't going to be fixed, it's a known consequence of using context-fill and animating fill.

According to :mconley, we'll be able to do this once we have WebRender: https://bugzilla.mozilla.org/show_bug.cgi?id=1352119#c25

> Why is this an animation rather than a transition?

Since the icon image has display: none when it's hidden, I'm not able to use a transition to make the fade-in work (unless you know of a way). Using an animation does work, however.

> This forces grayscale AA. We need to keep sub-pixel AA enabled.
> 
> Btw, this patch doesn't apply cleanly. You need to rebase.

Should I drop the slide-in effect for the text when a tab with no favicon finishes loading then? Or is there another way to achieve this?

Comment 74

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review160102

> I agree with Dao, this looks like it can be made to use CSS transitions instead of animation.

It used to use a transition, but in order to have the icon be `display: none` when it's not active, I had to switch to an animation. If you have some trick in mind here, I'd be happy to change it, but I spent a few hours struggling with it and this is the best I got.

> Can you use margin-inline-start: -22px instead? I don't think that will affect antialiasing.

Wouldn't this cause reflows during the transition when the text slides over to the left? Should I just remove the transition, or is there a better way to do this?

Comment 75

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138108/diff/8-9/

Comment 76

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/7-8/

Comment 77

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review159864

> You probably want a style flush here rather than a full layout flush.

I've tried `this.mTabBurst.getComputedStyle();`, but it doesn't seem to work. If there's a better way to do this, I'm all ears...

Comment 78

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Jim Porter (:squib) from comment #77)
> Comment on attachment 8875447 [details]
> Bug 1352119 - Implement new page loading indicator animation, part 2: add a
> "burst" when loading finishes
> 
> https://reviewboard.mozilla.org/r/146890/#review159864
> 
> > You probably want a style flush here rather than a full layout flush.
> 
> I've tried `this.mTabBurst.getComputedStyle();`, but it doesn't seem to
> work. If there's a better way to do this, I'm all ears...

I'm not sure what mTabBurst is here, but normally you'd want to call getComputedStyle on the appropriate window object (e.g. `window.getComputedStyle(tabBurst)`) but then you *also* need to actually read off the appropriate property from the returned CSSStyleDeclaration object in order to trigger a style flush. In this case, you want to ensure that animations have been re-resolved so `animationName` is the property you want.

e.g. window.getComputedStyle(document.getAnonymousElementByAttribute(this, "anonid", "tab-loading-burst")).animationName;

(Assuming 'window' is correct here -- I haven't looked at this code closely enough to be sure.)

The other option is you could do something like:

  const burstAnimation = elem.getAnimations({subtree:true}).find(
    animation => animation.animationName == 'tab-burst-animation');
  if (burstAnimation) {
    burstAnimation.currentTime = 0;
  } else {
    this.setAttribute("bursting", "true");
  }

But getAnimations() there is going to trigger a style flush every time so your current setup is probably better.

(And yet another way of doing this without triggering any style flushes, is just to append 'tab-burst-animation' to your animationName so that the computed value of animationName becomes 'tab-burst-animation, tab-burst-animation'. The other values in the list will be automatically duplicated and the animation will restart on the next style flush.)

Comment 79

[Jim Porter (:squib)]

Ooh, neat! I didn't know about `.getAnimations()`. I reworked my patch to use that (and only flushing style when the animation is already in progress) since I think it's the clearest solution. This is an edge case anyway (it only pops up if you reload a fast-loading page immediately after it finished loading), so I think this should perform fine. If it causes issues, I can tweak it further though.

Comment 80

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/8-9/

Comment 81

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review159864

> I've tried `this.mTabBurst.getComputedStyle();`, but it doesn't seem to work. If there's a better way to do this, I'm all ears...

I think I have the current patch working now with just a style flush.

Comment 82

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138108/diff/9-10/

Comment 83

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/9-10/

Comment 84

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review161392

This is currently failing browser/base/content/test/static/browser_all_files_referenced.js because you deleted the files but didn't remove them from the jar.mn. Note that they shouldn't be deleted though since we need them for v56.

@Dao, can you take a look at the .tab-throbber CSS changes?

::: browser/base/content/tabbrowser.xml:7355
(Diff revision 10)
>                       class="tab-throbber"
>                       role="presentation"
>                       layer="true" />

The indentation here should be fixed

::: browser/themes/shared/jar.inc.mn:204
(Diff revision 10)
> -  skin/classic/browser/tabbrowser/connecting.png               (../shared/tabbrowser/connecting.png)
> +  skin/classic/browser/tabbrowser/loading.svg                  (../shared/tabbrowser/loading.svg)
> -  skin/classic/browser/tabbrowser/[e-mail]            (../shared/tabbrowser/[e-mail])

connecting.png and [e-mail] cannot be deleted until 57. These should be wrapped in
%ifndef MOZ_PHOTON_ANIMATIONS
then the loading.svg should be wrapped in 
%ifdef MOZ_PHOTON_ANIMATIONS (or an else block)

::: browser/themes/shared/tabs.inc.css:82
(Diff revision 10)
>  
> -.tab-throbber,
> +@keyframes tab-throbber-pulse {
> +  100% { transform: translateX(-100%); }
> +}
> +
> +.tab-throbber {

Please change this to .tab-throbber[busy] so it only applies when we want it to (thus not creating another layer via position:relative; all the time).

::: browser/themes/shared/tabs.inc.css:89
(Diff revision 10)
> +  overflow: hidden;
> +  width: 16px;
> +  height: 16px;
> +}
> +
> +.tab-throbber::before {

.tab-throbber[busy]::before

::: browser/themes/shared/tabs.inc.css:100
(Diff revision 10)
> +.tab-throbber[busy]::before {
> +  animation: tab-throbber-pulse 1.05s steps(60) infinite;

Move the animation property to the ruleset above and then change this selector to only apply when not selected.

> .tab-throbber:not([selected])::before {
>   /* XXX: It would be nice to transition between the "connecting" color and the
>      "loading" color; however, that currently forces main thread painting. When
>      this is fixed (after WebRender lands), we can do something like
>      `transition: fill 0.333s;` */
> 
>   fill: var(--toolbarbutton-icon-fill);
>   opacity: .7;
> }

We need to remove [busy] from above so that the rule doesn't have too much specificity, plus this will only be displayed when the element has [busy] anyways.

::: browser/themes/shared/tabs.inc.css:112
(Diff revision 10)
> +
> +  fill: var(--toolbarbutton-icon-fill);
> +  opacity: .7;
> +}
> +
> +toolbar[brighttext] .tab-throbber[busy]::before {

toolbar[brighttext] .tab-throbber:not([selected])::before

::: browser/themes/shared/tabs.inc.css:116
(Diff revision 10)
> +
> +toolbar[brighttext] .tab-throbber[busy]::before {
> +  fill: var(--toolbarbutton-icon-fill-inverted);
> +}
> +
> +:root:-moz-lwtheme .tab-throbber[busy]::before {

:root:-moz-lwtheme .tab-throbber:not([selected])::before

::: browser/themes/shared/tabs.inc.css:120
(Diff revision 10)
> +.tab-throbber[progress]::before {
> +  fill: var(--tab-loading-fill);
> +}

I'm having trouble giving this enough specificity, this is what I've got right now:

.tab-throbber[busy][progress]::before {
  fill: var(--tab-loading-fill);
}

But this is missing just enough specificity to stop it from applying for unselected tabs. Using !important here would fix it but we shouldn't be using it for this.

I'm going to redirect to Dao for this review.

::: browser/themes/shared/tabs.inc.css
(Diff revision 10)
> -.tab-throbber[busy] {
> -  list-style-image: url("chrome://browser/skin/tabbrowser/connecting.png");
> -}
> -
> -.tab-throbber[progress] {
> -  list-style-image: url("chrome://global/skin/icons/loading.png");
> -}

These need to stay in a %ifndef MOZ_PHOTON_ANIMATIONS block for v56.

::: browser/themes/shared/tabs.inc.css
(Diff revision 10)
> -  .tab-throbber[busy] {
> -    list-style-image: url("chrome://browser/skin/tabbrowser/[e-mail]");
> -  }
> -
> -  .tab-throbber[progress] {
> -    list-style-image: url("chrome://global/skin/icons/[e-mail]");
> -  }

Ditto.

Comment 85

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review161346

r=me with the following changes.

::: browser/base/content/tabbrowser.xml:7599
(Diff revision 9)
> +        <body><![CDATA[
> +          if (Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {
> +            if (this.hasAttribute("bursting")) {
> +              // Restart the animation if it's already playing.
> +              let burst = document.getAnonymousElementByAttribute(this, "anonid", "tab-loading-burst");
> +              let animation = burst.getAnimations({subtree:true}).find(

This causes an eslint error, you should have a space after the colon.

 TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/base/content/tabbrowser.xml:7599:59 | Missing space before value for key 'subtree'. (key-spacing)

::: browser/themes/shared/tabs.inc.css:82
(Diff revision 10)
> +  display: none;
> +  position: absolute;
> +  content: "";
> +  /* We set the width to be a percentage of the tab's width so that we can use
> +     the `scale` transform to scale it up to the full size of the tab when the
> +     burst occurs. We also need to set the left offset so that the center of the

s/left offset/margin-inline-start/

::: browser/themes/shared/tabs.inc.css:93
(Diff revision 10)
> +}
> +
> +.tab-loading-burst[bursting]::before {

Why do we need to have two separate rules here? Can we just merge these to one, and then change the selector for the top ruleset to be .tab-loading-burst[bursting]::before ? Then we'll be able to remove the need to set display:none; and changing it to display:initial;

Comment 86

[Jared Wein [:jaws] (please needinfo? me)]

https://developer.mozilla.org/en-US/docs/Web/API/Document/getAnimations says that getAnimations() is not shipping yet past Developer Edition. Did something change? I don't see any bugs linked to bug 1254761 about shipping it to release.

Comment 87

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review161414

Changing to r- after re-reading comment 78. Can we go with just appending the animation name again (the last paragraph)?

Comment 88

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138108/diff/10-11/

Comment 89

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/10-11/

Comment 90

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review161414

I'm not actually sure how to do that. I tried a couple simple variations (e.g. `burst.style.animationName = "tab-burst-animation"`), but it didn't work.

Comment 91

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review161392

> Move the animation property to the ruleset above and then change this selector to only apply when not selected.
> 
> > .tab-throbber:not([selected])::before {
> >   /* XXX: It would be nice to transition between the "connecting" color and the
> >      "loading" color; however, that currently forces main thread painting. When
> >      this is fixed (after WebRender lands), we can do something like
> >      `transition: fill 0.333s;` */
> > 
> >   fill: var(--toolbarbutton-icon-fill);
> >   opacity: .7;
> > }
> 
> We need to remove [busy] from above so that the rule doesn't have too much specificity, plus this will only be displayed when the element has [busy] anyways.

Why the :not([selected])? Doesn't that change the behavior here?

Comment 92

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #86)
> https://developer.mozilla.org/en-US/docs/Web/API/Document/getAnimations says
> that getAnimations() is not shipping yet past Developer Edition. Did
> something change? I don't see any bugs linked to bug 1254761 about shipping
> it to release.

It's enabled for Chrome content too.[1]

[1] http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/dom/base/nsDocument.cpp#3240

Comment 93

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Jim Porter (:squib) from comment #90)
> Comment on attachment 8875447 [details]
> Bug 1352119 - Implement new page loading indicator animation, part 2: add a
> "burst" when loading finishes
> 
> https://reviewboard.mozilla.org/r/146890/#review161414
> 
> I'm not actually sure how to do that. I tried a couple simple variations
> (e.g. `burst.style.animationName = "tab-burst-animation"`), but it didn't
> work.

Yeah, you're right. That technique only works if you want to restart a finished animation due to the way the matching up of animations works (i.e. we match from the end of the list). e.g. https://jsfiddle.net/zat7sxuy/

Comment 94

[Eric Pang [:epang] UX]

Sorry for the slow reply, the ni slipped through the cracks. Motion-wise, I think it looks good.  A little hard to judge without the colours implemented yet though.  I'm also looking forward to seeing the wash once we have rectangular tabs.  It's slower then the design, but still feels good.  Might need to file a follow up bug later on, but for now it feels good to me. Awesome work Jim!

Comment 95

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jim Porter (:squib) from comment #91)
> Comment on attachment 8866480 [details]
> Bug 1352119 - Implement new page loading indicator animation, part 1: update
> the throbber
> 
> https://reviewboard.mozilla.org/r/138108/#review161392
> 
> > Move the animation property to the ruleset above and then change this selector to only apply when not selected.
> > 
> > > .tab-throbber:not([selected])::before {
> > >   /* XXX: It would be nice to transition between the "connecting" color and the
> > >      "loading" color; however, that currently forces main thread painting. When
> > >      this is fixed (after WebRender lands), we can do something like
> > >      `transition: fill 0.333s;` */
> > > 
> > >   fill: var(--toolbarbutton-icon-fill);
> > >   opacity: .7;
> > > }
> > 
> > We need to remove [busy] from above so that the rule doesn't have too much specificity, plus this will only be displayed when the element has [busy] anyways.
> 
> Why the :not([selected])? Doesn't that change the behavior here?

I may have gotten confused here and I should defer to Dao. What I was noticing was that in cases where the toolbar has [brighttext="true"], with your patch as-uploaded I was getting a white loading indicator instead of a blue one. Eric saw something similar though I'm not sure if his experience was due to [brighttext]. I flagged Dao for review so I expect he will be able to provide input on it when he looks at it.

Comment 96

[Eric Pang [:epang] UX]

Took a closer look this morning.  Can you make a few small tweaks to the burst?
1. Update the duration from 4s to 2.5s 
2. Update the opacity to start from 50% to 0% (atm it's from 100% to 0%)

Also, notice the ball is currently dark grey?  Are the able to do the grey for connecting then blue for loading?

Thanks!

Comment 97

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/11-12/

Comment 98

[Jim Porter (:squib)]

Ok, I updated things. The builds should end up here once they're done: <https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-8c3d347e2d193ab5629b84f539d594d251d07e7c/>.

Note: I set the initial opacity of the burst to 0.75 because I changed the initial size it has on the first frame. It was a bit too big and I was starting to notice a weird "jump" in the animation after watching it over and over. I went with 0.75 for opacity because that seemed to be similar to what 0.5 would look like using the old sizing. If it's still too opaque, I can drop it further though.

:jaws, I also updated some of the burst CSS because I was having an issue where the animation would only play once, and then it was broken on subsequent loads. I also went with using getComputedStyle() for the style flush, since that ends up being a little simpler code-wise. From comment 93, it sounds like we're stuck with some kind of explicit style flush...

Comment 99

[Eric Pang [:epang] UX]

(In reply to Jim Porter (:squib) from comment #98)
> Ok, I updated things. The builds should end up here once they're done:
> <https://archive.mozilla.org/pub/firefox/try-builds/
> [e-mail]-8c3d347e2d193ab5629b84f539d594d251d07e7c/>.
> 
> Note: I set the initial opacity of the burst to 0.75 because I changed the
> initial size it has on the first frame. It was a bit too big and I was
> starting to notice a weird "jump" in the animation after watching it over
> and over. I went with 0.75 for opacity because that seemed to be similar to
> what 0.5 would look like using the old sizing. If it's still too opaque, I
> can drop it further though.
> 
> :jaws, I also updated some of the burst CSS because I was having an issue
> where the animation would only play once, and then it was broken on
> subsequent loads. I also went with using getComputedStyle() for the style
> flush, since that ends up being a little simpler code-wise. From comment 93,
> it sounds like we're stuck with some kind of explicit style flush...

Hey Jim, is it possible to get a try build of osx?

Comment 100

[Eric Pang [:epang] UX]

never mind, i got it from Tree Herder.  I'm still not seeing the dot in the designed colours it's dark grey instead of grey then blue (during states).

Also, can you make the burst 1.8s?  It still feels a little bit slow.

Thanks!

Comment 101

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138108/diff/11-12/

Comment 102

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/12-13/

Comment 103

[Jim Porter (:squib)]

Kicked off another try build with the new burst timing. Hopefully I got the dot the right color, but it worked on my machine before this too...

Here's the Treeherder link: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=c791ebfb01c2>.

Comment 104

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review162164

Looks good, thanks for making the changes.

Comment 105

[Eric Pang [:epang] UX]

(In reply to Jim Porter (:squib) from comment #103)
> Kicked off another try build with the new burst timing. Hopefully I got the
> dot the right color, but it worked on my machine before this too...
> 
> Here's the Treeherder link:
> <https://treeherder.mozilla.org/#/jobs?repo=try&revision=c791ebfb01c2>.

I'm still seeing only a white dot :(.  The opacity of the burst is causing a flash effect.  I played around with the css to change some of the settings. Lowering the opacity helps with this. Can we change the starting opacity to 40% (seems low, but it make the motion more subtle) Thanks!

Comment 106

[Jim Porter (:squib)]

Wait... it's a white dot now? That's weird. The dark grey dot was probably some issue with the context-fill behavior. The burst is blue, right? That uses the same context-fill code, so it'd weird if one works for you and the other doesn't. Is it possible there's an issue with the macOS codepath here?

Comment 107

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/13-14/

Comment 108

[Darkspirit]

Attachment: 2017-07-14_01-47-16.mp4

(In reply to Jim Porter (:squib) from comment #106)

In this video you can see the build from comment 103 > Linux x64 opt @ Debian Testing (Linux 4.11.0-1-amd64, Radeon RX480) with a fresh profile + tracking protection (strict list) activated.
When the dot moves first time from left to right, it's grey, then blue afterwards.
But the loading effect on the whole tab is hard to see in Compact Light/Dark. The dot is always black in Compact Light. In Compact Dark, the dot is grey at first, then white.

Comment 109

[Jim Porter (:squib)]

That's correct behavior. Grey = connecting, and blue = loading. There might be some funkiness with the compact themes (esp the specificity on the CSS), but I'll let Eric chime in on what he thinks makes sense there before I go too far on implementing anything.

Eric: Any thoughts on what colors we should use for the compact themes?

Comment 110

[Eric Pang [:epang] UX]

(In reply to Jim Porter (:squib) from comment #109)
> That's correct behavior. Grey = connecting, and blue = loading. There might
> be some funkiness with the compact themes (esp the specificity on the CSS),
> but I'll let Eric chime in on what he thinks makes sense there before I go
> too far on implementing anything.
> 
> Eric: Any thoughts on what colors we should use for the compact themes?

It should be fine once the photon colours for the chrome are implemented.  The compact themes should also use the same colours - right now it's hard to see since the current compact themes haven't been updated.

> Wait... it's a white dot now? That's weird. The dark grey dot was probably some issue with the context-fill behavior. 
yep, i'm still seeing a white dot.

>The burst is blue, right? That uses the same context-fill code, so it'd weird if one works for you and the other doesn't. Is it possible there's an issue with the macOS codepath here?

Yep the burst is blue.

Also, were you able to update the burst to 40% opacity?

Comment 111

[Jim Porter (:squib)]

I updated the burst, but forgot to kick off another Try build. Here's the treeherder link for the latest revision: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f00df7a45a903e0fdfc621437b64a188481a6cd>.

Comment 112

[Jared Wein [:jaws] (please needinfo? me)]

I also still see the white dot, though I'm hopeful Dao will see what is wrong with the selectors/precendence in your CSS.

Is the burst ready for review now? We can do the ui-review and code review simultaneously.

Comment 113

[Jared Wein [:jaws] (please needinfo? me)]

FYI, the white bouncing ball may only be reproducible on Windows and OSX. I suggest you test there to see it for yourself. I see it with the build from comment 111. I have Windows configured to use the accent color in the titlebar on Firefox, and my accent color is a dark green.

Comment 114

[Jared Wein [:jaws] (please needinfo? me)]

@Dao, review ping?

Comment 115

[Eric Pang [:epang] UX]

(In reply to Jim Porter (:squib) from comment #111)
> I updated the burst, but forgot to kick off another Try build. Here's the
> treeherder link for the latest revision:
> <https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0f00df7a45a903e0fdfc621437b64a188481a6cd>.

Thanks Jim! The burst looks good to me now.  The dot is still white though :(

Comment 116

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138108/diff/12-13/

Comment 117

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/14-15/

Comment 118

[Jim Porter (:squib)]

Here's a try build with a speculative fix for the dot color. I can't build it locally though, but that might be my laptop dying... <https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee12b2d932dd74906a1ef5c50eb6275e672eeafa>

Comment 119

[Jim Porter (:squib)]

Oh, and everything here is review-ready, I think.

Comment 120

[Eric Pang [:epang] UX]

(In reply to Jim Porter (:squib) from comment #118)
> Here's a try build with a speculative fix for the dot color. I can't build
> it locally though, but that might be my laptop dying...
> <https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ee12b2d932dd74906a1ef5c50eb6275e672eeafa>

Hey Jim, I see the blue dot while loading now! But still white (instead of grey) for connecting.

Comment 121

[Tim Nguyen :ntim]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review166238

::: browser/themes/shared/tabs.inc.css:103
(Diff revision 13)
> +  fill: var(--toolbarbutton-icon-fill);
> +  opacity: 0.7;
> +}
> +
> +@keyframes tab-throbber-animation {
> +  100% { transform: translateX(-100%); }
> +}
> +
> +toolbar[brighttext] .tab-throbber[busy]:not([progress])::before {
> +  fill: var(--toolbarbutton-icon-fill-inverted);
> +}
> +
> +:root:-moz-lwtheme .tab-throbber[busy]:not([progress])::before {
> +  fill: var(--lwt-text-color);
> +}

Have you tried `currentColor` instead of all these different fills ?

Comment 122

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review166430

::: browser/base/content/tabbrowser.xml:7727
(Diff revision 15)
>          ]]></body>
>        </method>
>  
> +      <method name="animateLoadingBurst">
> +        <body><![CDATA[
> +          if (Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {

Please move this pref check outside of this function and check the pref before calling animateLoadingBurst, especially since this function is only called from one place.

::: browser/base/content/tabbrowser.xml:7732
(Diff revision 15)
> +          if (Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {
> +            if (this.hasAttribute("bursting")) {
> +              // Stop the animation if it's already playing so we can restart it.
> +              this.removeAttribute("bursting");
> +              let burst = document.getAnonymousElementByAttribute(this, "anonid", "tab-loading-burst");
> +              window.getComputedStyle(burst).animationName;

Please remove this style flush and rebase on top of the patches in https://bugzilla.mozilla.org/show_bug.cgi?id=1383367 so you can use await BrowserUtils.promiseLayoutFlushed(document, "style", ...)

::: browser/themes/shared/tabs.inc.css:82
(Diff revision 15)
> +.tab-loading-burst::before {
> +  position: absolute;
> +  content: "";
> +  /* We set the width to be a percentage of the tab's width so that we can use
> +     the `scale` transform to scale it up to the full size of the tab when the
> +     burst occurs. We also need to set the margine-inline-start so that the

s/margine/margin/

::: browser/themes/shared/tabs.inc.css:86
(Diff revision 15)
> +     the `scale` transform to scale it up to the full size of the tab when the
> +     burst occurs. We also need to set the margine-inline-start so that the
> +     center of the burst matches the center of the favicon. */
> +  width: 5%;
> +  height: 100%;
> +  margin-inline-start: calc(17px - 2.5%);

Where does 17px come from? Please describe how that number is arrived at in a comment here.

Comment 123

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138108/diff/13-14/

Comment 124

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/15-16/

Comment 125

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review166870

::: browser/base/content/tabbrowser.xml:7727
(Diff revision 15)
>          ]]></body>
>        </method>
>  
> +      <method name="animateLoadingBurst">
> +        <body><![CDATA[
> +          if (Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {

I don't know if I agree with this... shouldn't we try to encapsulate all the behavior in one spot? I could rename this function so it's more about the semantics of indicating that the main frame finished loading, e.g. `rootFrameFinishedLoading`?

Comment 126

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/16-17/

Comment 127

[Jim Porter (:squib)]

New try build with `currentColor`. Hopefully this works ok for you, Eric.

Comment 128

[Eric Pang [:epang] UX]

Hey Jim, I can see the gray now! But realized a little problem with the colour of the dot and the light theme.  The grey isn't visible enough on lighter backgrounds.  Can we try updating it to #0a84ff %50 when connecting, then full opacity when loading?  Let me know, thanks!

Comment 129

[Eric Pang [:epang] UX]

(In reply to Eric Pang [:epang] UX from comment #128)
> Hey Jim, I can see the gray now! But realized a little problem with the
> colour of the dot and the light theme.  The grey isn't visible enough on
> lighter backgrounds.  Can we try updating it to #0a84ff %50 when connecting,
> then full opacity when loading?  Let me know, thanks!

ignore my last comment - from our msgs it's good!

Comment 130

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review167948

Looks like my r+ got lost in these latest revisions. The changes look fine to me.

Comment 131

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Gijs, can you take over the review for Dao? This has been waiting for review for over 2 weeks now.

Comment 132

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review167962

::: browser/themes/shared/tabs.inc.css:152
(Diff revision 14)
> +.tab-icon-image:not([src]):not([busy]):not([pinned]) ~ .tab-label-container > .tab-label {
> +%ifndef MOZ_PHOTON_ANIMATIONS
> +  padding-inline-start: 0;
> +%else
> +  transform: translateX(-22px);
> +  transition: transform 0.333s var(--animation-easing-function);
> +%endif
> +}

Just a heads up that this rule is, I believe, going to cause a regression with sessionrestore_many_windows, since that Talos test causes a bunch of tabs to load without favicons, so we end up painting all of those .tab-labels to a new layer to translate them - that painting and layerization will occur on the main thread for each on-screen tab, and that will delay the restoration event processing.

Ideally, we'd suppress all animations (not just the window opening animations) like bug 1331932 suggested we'd do. But just a heads up that this will likely regress that Talos test.

Comment 133

[Jared Wein [:jaws] (please needinfo? me)]

Hey Jim, have you done a try run to compare the Talos numbers with and without your patch? You can push with the following try syntax:

`mach try -b o -p linux,linux64,win32,win64,macosx64 -u none -t all --rebuild-talos 8 --artifact`

Push to tryserver with your patch applied, and then with your patch unapplied (so that the only difference between the two pushes is your patch).

You can then use comparechooser to compare the two runs, https://treeherder.mozilla.org/perf.html#/comparechooser

Comment 134

[:Gijs (he/him)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review168258

::: browser/base/content/tabbrowser.xml:7430
(Diff revision 14)
>          </xul:hbox>
>          <xul:hbox xbl:inherits="pinned,selected=visuallyselected,titlechanged,attention"
>                    class="tab-content" align="center">
> -          <xul:image xbl:inherits="fadein,pinned,busy,progress,selected=visuallyselected"
> +          <xul:hbox xbl:inherits="fadein,pinned,busy,progress,selected=visuallyselected"
> -                     class="tab-throbber"
> +                    class="tab-throbber"
> -                     role="presentation"
> +                    role="presentation"

This is now superfluous, I think.

::: browser/themes/shared/tabs.inc.css:138
(Diff revision 14)
> +  0% { opacity: 0; }
> +  100% { opacity: 1; }

Elsewhere you said this is because of display: none, but the display: none has disappeared but this is still an animation rather than a transition. Can you clarify why?

::: browser/themes/shared/tabs.inc.css:156
(Diff revision 14)
> +
> +.tab-icon-image:not([src]):not([busy]):not([pinned]) ~ .tab-label-container > .tab-label {
> +%ifndef MOZ_PHOTON_ANIMATIONS
> +  padding-inline-start: 0;
> +%else
> +  transform: translateX(-22px);

This makes centered titles with no favicon look off-center, e.g. loading `data:text/html,<title>Hi</title>` shows this. So I doubt whether this is the correct way to implement this.

Comment 135

[:Gijs (he/him)]

It seems like it would make sense to implement swapping the favicon/loading animation separately from the sliding effect on the tab titles. Tackling one problem at a time will make patches easier to review and the causes of talos regressions, should there be any, easier to identify.

Comment 137

[Mike Conley (:mconley) (:⚙️)]

Out of curiosity, is this bug currently still blocked on Dao's review, or is there enough information here to do another revision?

Comment 138

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) - Holiday until August 8th from comment #137)
> Out of curiosity, is this bug currently still blocked on Dao's review, or is
> there enough information here to do another revision?

I plan to get back to this soon if the review request is still there by then, but expect that it's not ready based on Gijs' comments from a few days ago.

Comment 139

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review168258

> This is now superfluous, I think.

Just the `role="presentation"`, or are other bits superfluous?

> Elsewhere you said this is because of display: none, but the display: none has disappeared but this is still an animation rather than a transition. Can you clarify why?

`display: none;` is still applied here: <https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/base/content/tabbrowser.css#25-32>. This is intentional, since it helps with rendering perf.

> This makes centered titles with no favicon look off-center, e.g. loading `data:text/html,<title>Hi</title>` shows this. So I doubt whether this is the correct way to implement this.

If the new tab spec is for titles to be centered, it probably makes more sense to just drop this. However, I don't see centered titles on central yet; are they landed and preffed-off? If they're not landed, then it would probably make sense for whoever lands second to fix this up so it looks right...

Comment 140

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review167962

> Just a heads up that this rule is, I believe, going to cause a regression with sessionrestore_many_windows, since that Talos test causes a bunch of tabs to load without favicons, so we end up painting all of those .tab-labels to a new layer to translate them - that painting and layerization will occur on the main thread for each on-screen tab, and that will delay the restoration event processing.
> 
> Ideally, we'd suppress all animations (not just the window opening animations) like bug 1331932 suggested we'd do. But just a heads up that this will likely regress that Talos test.

It sounds like we'll probably be dropping this transition (it doesn't look great in my opinion anyway), so this may not actually matter...

Comment 141

[Jim Porter (:squib)]

Eric: Do you know what the spec is for centered tab titles? Specifically, I'm wondering what (if any) difference there should be between tabs with a favicon and tabs with no favicon. Would the horizontal position of the title change based on that? If so, should I try to retain the sliding animation, or should I drop it? (Dropping it would make it easier to retain good performance here and should have better font rendering per comment 67.)

Comment 142

[:Gijs (he/him)]

(In reply to Jim Porter (:squib) from comment #139)
> Comment on attachment 8866480 [details]
> Bug 1352119 - Implement new page loading indicator animation, part 1: update
> the throbber
> 
> https://reviewboard.mozilla.org/r/138108/#review168258
> 
> > This is now superfluous, I think.
> 
> Just the `role="presentation"`, or are other bits superfluous?

Just the role=presentation. Semantically, XUL <image> is like HTML <img> and means "here is an image that has meaning for the user". role=presentation is used to indicate the element is not semantically useful (which means accessibility tools don't show an image that would need alt(ernative) text in order to present it to non-sighted users). But here we're changing this to be a box, which has no semantic meaning at all, and so we don't need to "turn it off" with a role=presentation attribute.

> > This makes centered titles with no favicon look off-center, e.g. loading `data:text/html,<title>Hi</title>` shows this. So I doubt whether this is the correct way to implement this.
> 
> If the new tab spec is for titles to be centered, it probably makes more
> sense to just drop this. However, I don't see centered titles on central
> yet; are they landed and preffed-off? If they're not landed, then it would
> probably make sense for whoever lands second to fix this up so it looks
> right...

Uh, I tested with central (admittedly a few days old) - long titles are start-aligned, short titles are centered, hence the example I gave. I think this isn't changing - it works like that on release/beta, too.

Comment 143

[Jim Porter (:squib)]

(In reply to :Gijs from comment #142)
> Just the role=presentation. Semantically, XUL <image> is like HTML <img> and
> means "here is an image that has meaning for the user". 

Thanks, I'll update this.

> Uh, I tested with central (admittedly a few days old) - long titles are
> start-aligned, short titles are centered, hence the example I gave. I think
> this isn't changing - it works like that on release/beta, too.

Weird, the Linux version doesn't have centered titles...

Comment 144

[Tim Nguyen :ntim]

(In reply to Jim Porter (:squib) from comment #143)
> > Uh, I tested with central (admittedly a few days old) - long titles are
> > start-aligned, short titles are centered, hence the example I gave. I think
> > this isn't changing - it works like that on release/beta, too.
> 
> Weird, the Linux version doesn't have centered titles...

I think this is only on macOS

Comment 145

[Eric Pang [:epang] UX]

(In reply to Jim Porter (:squib) from comment #141)
> Eric: Do you know what the spec is for centered tab titles? Specifically,
> I'm wondering what (if any) difference there should be between tabs with a
> favicon and tabs with no favicon. Would the horizontal position of the title
> change based on that? If so, should I try to retain the sliding animation,
> or should I drop it? (Dropping it would make it easier to retain good
> performance here and should have better font rendering per comment 67.)

Hi Jim, sorry for the slow reply I was on PTO last week and yesterday was a holiday here.

From my understanding the title is always left aligned with the favicon.
If there's no favicon for the site a generic one should be used.
Stephen, can you help confirm this?

This should mean we don't have to worry about the sliding animation?

Comment 146

[Dão Gottwald [:dao]]

(In reply to Eric Pang [:epang] UX from comment #145)
> From my understanding the title is always left aligned with the favicon.

Yes, filed bug 1389008 on this.

> If there's no favicon for the site a generic one should be used.
> Stephen, can you help confirm this?

I don't think this is true.

> This should mean we don't have to worry about the sliding animation?

The degraded text rendering is still a problem.

Comment 147

[Dão Gottwald [:dao]]

Anyway, I agree with this:

(In reply to :Gijs from comment #135)
> It seems like it would make sense to implement swapping the favicon/loading
> animation separately from the sliding effect on the tab titles. Tackling one
> problem at a time will make patches easier to review and the causes of talos
> regressions, should there be any, easier to identify.

As I understand it, the icon itself and the sliding animation can be treated as two separate features. It doesn't seem to make sense to block the icon change on issues with the sliding animation any longer.

Comment 148

[Eric Pang [:epang] UX]

(In reply to Dão Gottwald [::dao] from comment #147)
> Anyway, I agree with this:
> 
> (In reply to :Gijs from comment #135)
> > It seems like it would make sense to implement swapping the favicon/loading
> > animation separately from the sliding effect on the tab titles. Tackling one
> > problem at a time will make patches easier to review and the causes of talos
> > regressions, should there be any, easier to identify.
> 
> As I understand it, the icon itself and the sliding animation can be treated
> as two separate features. It doesn't seem to make sense to block the icon
> change on issues with the sliding animation any longer.

Agreed, we should land and handle this in a separate bug.

Also, Stephen confirmed that there is no generic favicon.
Jim, are there other options we can explore that are good on performance?  Opacity/fade for example.

Comment 149

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138108/diff/14-15/

Comment 150

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/17-18/

Comment 151

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review172876

::: browser/themes/shared/jar.inc.mn:81
(Diff revisions 17 - 18)
>  #ifndef MOZ_PHOTON_THEME
>    skin/classic/browser/identity-icon-hover.svg                 (../shared/identity-block/identity-icon-hover.svg)
>    skin/classic/browser/identity-icon-notice-hover.svg          (../shared/identity-block/identity-icon-notice-hover.svg)
>  #endif
>    skin/classic/browser/info.svg                                (../shared/info.svg)
> +#ifndef MOZ_PHOTON_THEME

This needs rebasing again since MOZ_PHOTON_ANIMATIONS and MOZ_PHOTON_THEME have been removed from the tree.

Comment 152

[Dão Gottwald [:dao]]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review172878

::: browser/themes/shared/tabs.inc.css:53
(Diff revision 15)
>    position: relative;
>    z-index: 2;
>  }
>  
>  .tab-content {
> +  position: relative;

Why is this needed when .tab-throbber has position: relative too?

::: browser/themes/shared/tabs.inc.css:137
(Diff revision 15)
> +}
> +
> +@keyframes tab-icon-image-fade-in {
> +  0% { opacity: 0; }
> +  100% { opacity: 1; }
> +}

Can this be just a transition rather than an animation?

Comment 153

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review172878

> Can this be just a transition rather than an animation?

It's an animation because a transition doesn't work when going from a display: none element. I've added a comment explaining this. (If there is a way to use a transition, I'd be happy to change this.)

Comment 154

[Jim Porter (:squib)]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138108/diff/15-16/

Comment 155

[Jim Porter (:squib)]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146890/diff/18-19/

Comment 156

[Jim Porter (:squib)]

Ok, this should be fully up to date now. There are still a few minor issues:

* The tab loading burst goes over the tab border; this is really hard to tell though so it might not be worth changing.
* I haven't updated to use `promiseLayoutFlushed` (haven't gotten it to work yet).
* I didn't delete connecting.png and [e-mail]. I'm not sure if we still need those for anything else.
* I still need to run updated Talos tests (I'll kick these off soon though).

I also entirely removed the sliding effect on the tab title. We can do something about that as a followup if we want, though.

Comment 157

[Dão Gottwald [:dao]]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review173018

Comment 158

[Dão Gottwald [:dao]]

Comment on attachment 8875447 [details]
Bug 1352119 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes

https://reviewboard.mozilla.org/r/146890/#review173020

::: browser/themes/shared/tabs.inc.css:79
(Diff revision 19)
> +  margin-inline-start: calc(17px - 2.5%);
> +}
> +
> +.tab-loading-burst[pinned]::before {
> +  /* This is like the margin-inline-start rule above, except that icons for
> +     pinned tabs are 12px rom the edge. */

s/rom/from/

Comment 160

[Dão Gottwald [:dao]]

Comment on attachment 8866480 [details]
Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber

https://reviewboard.mozilla.org/r/138108/#review173046

::: browser/themes/shared/jar.inc.mn:225
(Diff revision 16)
>    skin/classic/browser/privatebrowsing/favicon.svg             (../shared/privatebrowsing/favicon.svg)
>    skin/classic/browser/privatebrowsing/private-browsing.svg    (../shared/privatebrowsing/private-browsing.svg)
>    skin/classic/browser/privatebrowsing/tracking-protection-off.svg (../shared/privatebrowsing/tracking-protection-off.svg)
>    skin/classic/browser/privatebrowsing/tracking-protection.svg (../shared/privatebrowsing/tracking-protection.svg)
>    skin/classic/browser/compacttheme/loading-inverted.png (../shared/compacttheme/loading-inverted.png)
>    skin/classic/browser/compacttheme/[e-mail] (../shared/compacttheme/[e-mail])

Oh, please remove this too:

http://searchfox.org/mozilla-central/rev/33295c6f4d57d16fe76476ac131cadd4cd3d05ad/browser/themes/shared/compacttheme.inc.css#152-161

Comment 161

[Jared Wein [:jaws] (please needinfo? me)]

I'll take this and work on fixing the review comments as well as getting the Talos numbers.

Comment 162

[Jared Wein [:jaws] (please needinfo? me)]

Compare talos URL,
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=71d137a4053931c96c06af2a401a90af3ea59402&newProject=try&newRevision=c3380ec4d0987627897fe4e0e4b8876a7eb6853f&framework=1&showOnlyImportant=0

Try push with test results,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3380ec4d0987627897fe4e0e4b8876a7eb6853f

Comment 163

[Jared Wein [:jaws] (please needinfo? me)]

Compare talos URL with base vs throbber,
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=71d137a4053931c96c06af2a401a90af3ea59402&newProject=try&newRevision=c581c04757b6c83e4354a386e85a399fea450d0c&framework=1&showOnlyImportant=0

Compare talos URL with throbber vs throbber + burst,
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c581c04757b6c83e4354a386e85a399fea450d0c&newProject=try&newRevision=c3380ec4d0987627897fe4e0e4b8876a7eb6853f&framework=1&showOnlyImportant=0

Comment 164

[Jared Wein [:jaws] (please needinfo? me)]

From comment +163,

The patch for the loading throbber gives the following regressions(+) and improvements(-):
bloom_basic_singleton opt e10s
  osx-10-10 +3.22%
sessionrestore_many_windows opt e10s
  linux64                  +5.22%
  linux64-stylo            +4.48%
  linux64-stylo-sequential +4.44%
  osx-10-10                +4.20%
  windows7-32              +3.54%
tart opt e10s
  linux64                  -23.20%
  linux64-stylo            -23.50%
  linux64-stylo-sequential -23.46%
  osx-10-10                -8.62%
  windows10-64             -16.63%
  windows7-32              -16.12%
tp5o_webext % Processor Time opt e10s
  windows7-32              +3.42%
ts_paint_webext opt e10s
linux64-stylo-sequential   +2.02%

Adding the burst patch to the throbber patch gives the following regressions(+) and improvements(-):
damp opt e10s
  linux64-stylo            +2.05%
  linux64-stylo-sequential +2.66%
  windows7-32              +3.30%
tart opt e10s
  osx-10-10                +11.80%
tp5o Main_RSS opt e10s
  linux64                  +3.68%
  linux64-stylo            +3.90%
  linux64-stylo-sequential +4.03%
tp5o Private Bytes opt e10s
  windows7-32              +3.34%
tp5o_scroll opt e10s
  linux64                  -21.52%
  linux64-stylo            -18.30%
  linux64-stylo-sequential -18.30%
  osx-10-10                +41.05%
tp5o_webext Main_RSS opt e10s
  linux64                  +4.40%
  linux64-stylo            +3.50%
  linux64-stylo-sequential +4.60%
tp5o_webext Private Bytes opt e10s
  windows7-32              +3.77%
tp5o_webext responsiveness opt e10s
  linux64-stylo-sequential +7.19%
tp5o_webext opt e10s
  linux64-stylo-sequential +4.08%
tpaint opt e10s
  linux64                  +2.69%
  osx-10-10                +11.74%



The patch for the loading throbber is slow in two parts. The largest part is the fade in of the site's favicon. The following comparison shows the performance change when removing this animation:



Removing the fade in of the site's favicon (regression: +, improvement: -):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26d1ed4ca5d3d60cf78d75c1c25fd50db14a0142
(baseline vs throbber + change) https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=71d137a4053931c96c06af2a401a90af3ea59402&newProject=try&newRevision=26d1ed4ca5d3d60cf78d75c1c25fd50db14a0142&framework=1
(throbber vs throbber + change) https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c3380ec4d0987627897fe4e0e4b8876a7eb6853f&newProject=try&newRevision=26d1ed4ca5d3d60cf78d75c1c25fd50db14a0142&framework=1

When comparing the baseline vs. the throbber + change, we see an improvement in sessionrestore_many_windows which is better than expected. In the expected case we hoped to see the regression fixed, but instead we end up with an improvement across the board ranging from -2.76% on linux64 to -20.55% on windows10-64. We do see a regression across the board with damp which is not that well explained, ranging from +2.14% on linux64-stylo to +2.36% on linux64-stylo-sequential. Other areas of improvement are in tpaint (-4.70% to -4.89%), tresize, and ts_paint.

When comparing the throbber vs. the throbber + change, we see a larger improvement in sessionrestore_many_windows. This test improves further by -6.49% on windows7-32 to -21.93% on windows10-64. Curiously, tart regresses across the board from +19.58% on windows7-32 to +27.40% on linux64. The fact that the tart regression doesn't show up in the baseline vs. the throbber + change comparison is unexpected, showing that this changeset removes the improvement in tart that was seen in the initial comparison at the top of this comment. tp5o Main_RSS, tpaint, and tresize also show further improvements.



We can also improve sessionrestore a small amount by not starting any tab animation until the session restore has completed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c7037c588dc06e55a1e3f84ae8b8635f7fc8123
(baseline vs thobber + change) https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=71d137a4053931c96c06af2a401a90af3ea59402&newProject=try&newRevision=0c7037c588dc06e55a1e3f84ae8b8635f7fc8123&framework=1
(throbber vs throbber + change) https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c3380ec4d0987627897fe4e0e4b8876a7eb6853f&newProject=try&newRevision=0c7037c588dc06e55a1e3f84ae8b8635f7fc8123&framework=1

When comparing the throbber vs. the throbber + change, we see an improvement across the board in damp, sessionrestore_many_windows, tart, tp5o Main_RSS, tpaint, and tresize. We only see regressions in tp5o_scroll on linux (+22.11% to +27.30%).

Test results are not complete yet for combining these these two optimizations together. They should appear here when done:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cc048bbef2001ddfeafab18a4918c7ae3d10f2e
(baseline vs throbber + changes) https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=71d137a4053931c96c06af2a401a90af3ea59402&newProject=try&newRevision=8cc048bbef2001ddfeafab18a4918c7ae3d10f2e&framework=1
(throbber vs throbber + changes) https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c3380ec4d0987627897fe4e0e4b8876a7eb6853f&newProject=try&newRevision=8cc048bbef2001ddfeafab18a4918c7ae3d10f2e&framework=1



I plan on doing a similar "sessionrestored" optimization for the burst animation. Local testing shows that this change will regress sessionrestore performance but I will push it to tryserver to verify. We can also use the `tabAnimationsInProgress` property of tabbrowser to skip showing the burst if any tab animations are in progress. I have not implemented this yet but will do so very soon and push the patch to tryserver to verify the results.

Comment 165

[Jared Wein [:jaws] (please needinfo? me)]

I have implemented two simple patches that should improve the performance of the burst animation.



The first patch delays running the burst until sessionrestore has finished (regression: +, improvement: -):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e96e97afdd20f3ae7ae5aeeaa92092490cb25ecc
(baseline vs throbber + burst + change) https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=71d137a4053931c96c06af2a401a90af3ea59402&newProject=try&newRevision=e96e97afdd20f3ae7ae5aeeaa92092490cb25ecc&framework=1
(throbber + burst vs throbber + burst + change) https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b7d9a8c32937a6547f87ac49f9999974cd51844a&newProject=try&newRevision=e96e97afdd20f3ae7ae5aeeaa92092490cb25ecc&framework=1



The second patch prevents running the burst animation if a tab animation is in progress (regression: +, improvement: -):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc32ca00bb561642b56752046c7a37fc127c75db
(baseline vs throbber + burst + change) https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=71d137a4053931c96c06af2a401a90af3ea59402&newProject=try&newRevision=dc32ca00bb561642b56752046c7a37fc127c75db&framework=1
(throbber + burst vs throbber + burst + change) https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b7d9a8c32937a6547f87ac49f9999974cd51844a&newProject=try&newRevision=dc32ca00bb561642b56752046c7a37fc127c75db&framework=1



This try push combines both patches (regression: +, improvement: -):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49425f4610cb16a8bb5609759527d6718af517f6
(baseline vs throbber + burst + change) https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=71d137a4053931c96c06af2a401a90af3ea59402&newProject=try&newRevision=49425f4610cb16a8bb5609759527d6718af517f6&framework=1
(throbber + burst vs throbber + burst + change) https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b7d9a8c32937a6547f87ac49f9999974cd51844a&newProject=try&newRevision=49425f4610cb16a8bb5609759527d6718af517f6&framework=1



All patches regarding the burst animation are built on top of both optimizations for the loading throbber.

Comment 166

[Jared Wein [:jaws] (please needinfo? me)]

I'm going to split this bug in two since it may take another day or two to get the performance correct for the burst animation. I'll file a new bug for the burst animation and land the throbber animation in this bug since the Talos results show it is now showing an improvement.

Comment 167

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1352119 - Improve the performance of the throbber animation by removing the icon fade as well as only starting the throbber animation after the session has restored.

Review commit: https://reviewboard.mozilla.org/r/170544/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170544/

Comment 168

[Dão Gottwald [:dao]]

Comment on attachment 8899324 [details]
Bug 1352119 - Improve the performance of the throbber animation by removing the icon fade as well as only starting the throbber animation after the session has restored.

https://reviewboard.mozilla.org/r/170544/#review175952

::: browser/base/content/browser.js:1750
(Diff revision 1)
>          Cu.reportError(ex);
>        }
>      }, {timeout: 10000});
> +
> +    scheduleIdleTask(() => {
> +      document.documentElement.setAttribute("sessionrestored", "true");

Any reason for using scheduleIdleTask here rather than e.g. setting the attribute directly here? http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/browser/base/content/browser.js#1682

It's not like you're doing any actual work in the scheduleIdleTask callback.

Comment 169

[Jared Wein [:jaws] (please needinfo? me)]

No reason, I can make that change.

Comment 170

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8899324 [details]
Bug 1352119 - Improve the performance of the throbber animation by removing the icon fade as well as only starting the throbber animation after the session has restored.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170544/diff/1-2/

Comment 171

[Dão Gottwald [:dao]]

Comment on attachment 8899324 [details]
Bug 1352119 - Improve the performance of the throbber animation by removing the icon fade as well as only starting the throbber animation after the session has restored.

https://reviewboard.mozilla.org/r/170544/#review176018

Comment 172

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de253c538644
Implement new page loading indicator animation, part 1: update the throbber. Additional performance improvements implemented by Jared Wein. r=dao

Comment 173

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Just for the record, this caused some conflicts when I merged this to m-c after previously merging bug 1389921 from autoland. I think I resolved it correctly, though.

Comment 174

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/de253c538644

Comment 175

[Md. Majedul islam]

I can see this feature implemented on latest nightly 57.0a1 (2017-08-22) in Ubuntu 16.04, 64 bit 

Build ID 	20170822100529
User Agent      Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 176

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Wes Kocher (:KWierso) from comment #173)
> Just for the record, this caused some conflicts when I merged this to m-c
> after previously merging bug 1389921 from autoland. I think I resolved it
> correctly, though.

I verified that the patch on m-c is correct.

Comment 177

[Salar]

I'm not sure if this bug caused this issue:
https://bugzilla.mozilla.org/show_bug.cgi?id=1393351