Closed Bug 1390579 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.2 - Aug 29
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

(Keywords: regression, Whiteboard: [reserve-photon-animation])

Attachments

(1 file)

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
Blocks: 1384180
Whiteboard: [photon-animation][triage]
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.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 57.2 - Aug 29
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon-animation][triage] → [reserve-photon-animation]
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?
Attachment #8897547 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8897547 - Flags: review?(gijskruitbosch+bugs)
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!
Attachment #8897547 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by jwein@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/aa46b8eed093
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Flags: qe-verify? → qe-verify+
QA Contact: stefan.georgiev
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.