Closed Bug 1394985 Opened 2 years ago Closed 2 years ago

Tab loading indicators should be synced to have the same startTime


(Firefox :: Tabbed Browser, defect, P1)




Firefox 57
57.3 - Sep 19
Tracking Status
firefox57 --- verified


(Reporter: jaws, Assigned: birtles)



(Whiteboard: [reserve-photon-animation])


(2 files, 4 obsolete files)

If two tabs begin loading at separate times, their tab loading indicator animation will have started at separate times and thus the two "ping pong balls" will not be in sync with each other.

When we set the "busy" attribute on a tab [1], we should either:
A) set the start time of the current tab's loading indicator animation to equal that of any in progress
B) set the start time of all tab loading indicator animations to 0

We should also not force a synchronous layout flush when doing this. We can use BrowserUtils.promiseLayoutFlushed [2] to wait for the style flush.

We may also have to do this at [3] for session restore.



Attached patch WIP patch (obsolete) — Splinter Review
Here's a very rudimentary proof of concept.

Unfortunately getAnimations({subtree:true}) doesn't descend into anonymous content. I expect our AnimationObserver implementation doesn't either.

I haven't looked at the session restore part yet either.
Flags: qe-verify?
Priority: -- → P4
Whiteboard: [photon-animation][triage] → [reserve-photon-animation]
Comment on attachment 8902558 [details] [diff] [review]
WIP patch

Review of attachment 8902558 [details] [diff] [review]:

This patch works for me when testing with

::: browser/base/content/tabbrowser.xml
@@ +637,5 @@
> +                     animation.animationName === "tab-throbber-animation-rtl") &&
> +                    (animation.playState === "running" || animation.playState === "pending")
> +                  )
> +                  // Sort by startTime such that null start times sort last
> +                  .sort((a, b) => String(a.startTime) > String(b.startTime));

Instead of trying to sort these placing placing null at the end (and in the interim converting each Number to a String), how about we instead map null -> Infinity, then just use Math.max on the array.

Math.min(...[1.23,1.032,3,1.0000001,null,2].map(n => n === null ? Infinity : n))
Attachment #8902558 - Flags: feedback+
Assignee: nobody → bbirtles
Priority: P4 → P1
I discovered that MutationObservers observe animations in XBL anonymous content so I tried using that approach. However, I gave up on it because at the point where the observer is called, the added animations are still pending so in order to get the startTime of the oldest running animation we'd either need to:

* Observer changes to animations too (lots of bookkeeping) or,
* Use `getAnimations` (at which point the previous patch is simpler).

Furthermore, this approach would store a one time 'first start time' so in order to get the behavior where, if there are currently no running animations we start from the beginning, we'd need to watch for removed animations too. So, basically, lots of bookkeeping.
Attached patch WIP patch v2 (obsolete) — Splinter Review
Updated WIP patch incorporating feedback from comment 2 and adding other simplifications.
Attachment #8902558 - Attachment is obsolete: true
Jared, how do I test the session restore code path?

Also, assuming I do need to call this from SessionRestore.jsm, where is the best place to share this code? Adding a method to tabbrowser-tab?
Flags: needinfo?(jaws)
Attached patch WIP patch v3 (obsolete) — Splinter Review
Fix a few lint errors.
Attachment #8903036 - Attachment is obsolete: true
Attached patch WIP patch v4 (obsolete) — Splinter Review
Fix a bug
Attachment #8903038 - Attachment is obsolete: true
Comment on attachment 8903050 [details] [diff] [review]
WIP patch v4

Review of attachment 8903050 [details] [diff] [review]:

I talked with mikedeboer and he said that he doesn't think anything special will need to be done for sessionrestore.

> <mikedeboer> ok, that's in the tab constructor... so sessionstore doesn't do anything special there methinks. so this'll just work

::: browser/base/content/tabbrowser.xml
@@ +643,5 @@
> +         => anim.startTime === null ? Infinity : anim.startTime)
> +                );
> +                if (firstStartTime !== Infinity) {
> +                  for (let animation of animations) {
> +                    animation.startTime = firstStartTime;

Since this will cause another style flush, please put this loop inside of a requestAnimationFrame callback.
Attachment #8903050 - Attachment is obsolete: true
Please let me know if there are any talos numbers I ought to check for this sort of change.
Comment on attachment 8903392 [details]
Bug 1394985 - Synchronize throbber animations;

To check Talos numbers for this you can do the following:

With this changeset applied and without it applied, push to tryserver using the following syntax:

try: -b o -p linux64,macosx64,win32,win64 -u none -t other-e10s,svgr-e10s --rebuild-talos 6 --artifact 

Then you can use perfherder to compare the two pushes.
Attachment #8903392 - Flags: review?(jaws) → review+
Thanks Jared!

I just remembered that we have an override for the 'playState' getter for CSSAnimation objects that flushes style[1] so I'll revise the patch to not check playState in the rAF callback (and instead just pass in animations that are not already synced into the closure).

Pushed by
Synchronize throbber animations; r=jaws
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Iteration: --- → 57.3 - Sep 19
Flags: qe-verify? → qe-verify+
QA Contact: stefan.georgiev
I was able to reproduce this issue with the following Nightly build (20170901150353).

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 (20170915100121)

This issue is verified as fixed with the latest Nightly build on 9/15/2017.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.