1390579 - Switching tabs from a loading page to a non-loading page plays the stop->reload animation

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

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

STR:
Open Firefox to mozilla.org
Wait for mozilla.org to finish loading
Open a second tab to deelay.me
While deelay.me is loading, switch tabs to the mozilla.org tab

ER:
No animation when switching stop/reload states

AR:
The stop->reload animation plays

Comment 1

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

http://deelay.me/3000/http://deelay.me/img/10000ms.gif might be a better URL to test since it completes loading in only 3 seconds vs the long delay for deelay.me.

Comment 2

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

Attachment: Bug 1390579 - Check if aRequest is an instanceof nsIRequest instead of just a simple null check since when switching tabs aRequest is an Object that maintains some of the request details.

I also changed the code to update the timeWhenSwitchedToStop so if pages continue to load long after the tab switch we will still show the animation when the page has finished loading.

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

Comment 3

[:Gijs (he/him)]

Comment on attachment 8897547 [details]
Bug 1390579 - Check if aRequest is an instanceof nsIRequest instead of just a simple null check since when switching tabs aRequest is an Object that maintains some of the request details.

https://reviewboard.mozilla.org/r/168812/#review174208

There might be an issue with initial requests (see below), but more generally, this code feels like it should really be using per-tab data instead of having a single `timeWhenSwitchedToStop` value, which then affects all tabs. So it feels like, if I load tab A and immediately switch to tab B that also has a load pending, whether tab B shows an animation when the stop button switches to a reload one depends on how close the start of the load in A and the end of the load in B are together (time-wise), rather than only on tab B. That doesn't seem right. Or am I missing something? (in which case, what? :-) )

::: browser/base/content/browser.js:5073
(Diff revision 1)
> -    return !this.timeWhenSwitchedToStop ||
> +    return this.timeWhenSwitchedToStop &&
>             window.performance.now() - this.timeWhenSwitchedToStop > 150;

Naively, I would expect this would have prevented the animation from showing for initial loads in the window, becuase presumably timeWhenSwitchedToStop would be 0/undefined. After your change, that's no longer the case. Is that intentional, or is there something else that's preventing the animation for initial loads?

Comment 4

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

Comment on attachment 8897547 [details]
Bug 1390579 - Check if aRequest is an instanceof nsIRequest instead of just a simple null check since when switching tabs aRequest is an Object that maintains some of the request details.

(In reply to :Gijs from comment #3)
> There might be an issue with initial requests (see below), but more
> generally, this code feels like it should really be using per-tab data
> instead of having a single `timeWhenSwitchedToStop` value, which then
> affects all tabs. So it feels like, if I load tab A and immediately switch
> to tab B that also has a load pending, whether tab B shows an animation when
> the stop button switches to a reload one depends on how close the start of
> the load in A and the end of the load in B are together (time-wise), rather
> than only on tab B. That doesn't seem right. Or am I missing something? (in
> which case, what? :-) )

This is actually a case where per-tab data doesn't make as much sense. Switching to a tab that immediately plays the animation would be distracting in the same way that the initial bug was trying to fix (bug 1384180). In that case, we would be switching from reload->stop->reload all within 150ms which is something that UX has asked us not to animate.
 
> ::: browser/base/content/browser.js:5073
> (Diff revision 1)
> > -    return !this.timeWhenSwitchedToStop ||
> > +    return this.timeWhenSwitchedToStop &&
> >             window.performance.now() - this.timeWhenSwitchedToStop > 150;
> 
> Naively, I would expect this would have prevented the animation from showing
> for initial loads in the window, becuase presumably timeWhenSwitchedToStop
> would be 0/undefined. After your change, that's no longer the case. Is that
> intentional, or is there something else that's preventing the animation for
> initial loads?

The old code here would have returned true if timeWhenSwitchedStop was 0/undefined, which would have played the animation when we didn't want it to. So this in itself was wrong.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8897547 [details]
Bug 1390579 - Check if aRequest is an instanceof nsIRequest instead of just a simple null check since when switching tabs aRequest is an Object that maintains some of the request details.

https://reviewboard.mozilla.org/r/168812/#review174388

Alright, sorry for the confusion!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa46b8eed093
Check if aRequest is an instanceof nsIRequest instead of just a simple null check since when switching tabs aRequest is an Object that maintains some of the request details. r=Gijs

Comment 7

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

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

Comment 8

[Stefan [:StefanG_QA]]

User Agent    Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build   ID    20170830100230

This bug fix is Verified with latest Nightly 57.0a1.