1331932 - Disable UI animations during session restore

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Mike de Boer [:mikedeboer]]

Various animations are active whilst restoring windows and tabs, like window resizing animation, tab-open animation, tab-overflow animation(s), etc. Ideally, these would be disabled during restore.

Comment 1

[(no longer active)]

Don't we want the opposite behavior, aka, not janking such animations *by* the session store code?

Mike, this is an example of the kind of JS code that we periodically run which may jank arbitrary UI operations.  We may want to come up with a systematic way of avoiding this problem.  Needinfoing to bring this to your attention in case you have a discussion with the UX team although I don't need any immediate info from you. :-)

Comment 2

[Mike de Boer [:mikedeboer]]

Well, this is also about perceived performance if that matters at all. UI animations during UI restore are pretty useless and often out-of-place.

Comment 3

[(no longer active)]

That's true.  Overall the entire session restore experience is *miserable*.  If you went fruit picking you could collect baskets worth of low hanging fruits.  One day I'm planning to sit down and file a whole bunch of bugs on it (and I'm sure there are existing bugs on file also).

Comment 4

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

(In reply to :Ehsan Akhgari from comment #1)
> Don't we want the opposite behavior, aka, not janking such animations *by*
> the session store code?

We probably want to approach it from both sides:

1) Animations critical to perceived performance (tab opening and closing, loading throbbers, etc) should all be off main thread so that they're not affected by things like SessionStore.

2) SessionStore should probably avoid kicking off any animations. We need to prioritize getting the user from point A to point B (the restored state) in the shortest amount of time possible. The more things we can remove between point A and point B, animations included, the better.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Just FTR, we already add new tabs without animations when restoring:

http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/browser/components/sessionstore/SessionStore.jsm#3269

A really costly thing still are tabbar reflows and its overflow behavior with many tabs. Having an API to add multiple tabs to a window with a single function would probably yield great results. There's a bug somewhere about this that I can't find right now...

Comment 6

[Bao Quan [:beekill]]

I'm not sure, but it looks like there is no animation when we're restoring.

For tabbar overflow [1]:
+ _handleTabSelect calls ensureElementIsVisible [2] with aSmoothScroll = false. We also make sure that smooth scroll is disable here: [3]

Window resize [4] calls adjustTabstrip and TabsInTitlebar.updateAppearance. Both of them don't have animation.

We probably can force disable animation while changing size mode [5] like bug 1336230. 

Tim, what tabbar reflows and overflow behaviour you talked about?

[1]: http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#5794-5795
[2]: http://searchfox.org/mozilla-central/source/toolkit/content/widgets/scrollbox.xml#232
[3]: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#3262

[4]: http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#6401
[5]: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#3950

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Bao Quan [:beekill] from comment #6)
> Tim, what tabbar reflows and overflow behaviour you talked about?

I just meant that when restoring a session we add one tab after the other. We have to reflow the whole tabbar after each and every gBrowser.addTab() call. It would be a lot cheaper to create all those tabs in a DocFragment and then insert that, but that would mean you'd have to rework quite a few things in <tabbrowser>. Might be feasible now with non-webext-addons being phase out soon.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #7)
> (In reply to Bao Quan [:beekill] from comment #6)
> > Tim, what tabbar reflows and overflow behaviour you talked about?
> 
> I just meant that when restoring a session we add one tab after the other.
> We have to reflow the whole tabbar after each and every gBrowser.addTab()
> call.

Only if addTab or code running between the addTab calls flushes layout. We've been working on eliminating these layout flushes, see browser/base/content/test/performance/browser_tabopen_reflows.js.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Ah, thanks for the correction. Great to see only a single reflow left in there!

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Bao Quan [:beekill] from comment #6)
> Window resize [4] calls adjustTabstrip and TabsInTitlebar.updateAppearance.
> Both of them don't have animation.

Try restoring a window with many, many tabs. Since the new tabs are not scrolling into view, they disappear from view and the tab scroll button starts highlighting. This is done here: http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#6475

> We probably can force disable animation while changing size mode [5] like
> bug 1336230. 

That sounds very useful to me! Somehow on OSX, the window resize animation _always_ plays, so perhaps it's different on that platform for some reason.

Comment 11

[Bao Quan [:beekill]]

(In reply to Mike de Boer [:mikedeboer] from comment #10)
> (In reply to Bao Quan [:beekill] from comment #6)
> > Window resize [4] calls adjustTabstrip and TabsInTitlebar.updateAppearance.
> > Both of them don't have animation.
> 
> Try restoring a window with many, many tabs. Since the new tabs are not
> scrolling into view, they disappear from view and the tab scroll button
> starts highlighting. This is done here:
> http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.
> xml#6475

I think the bug you mentioned was solved here [1].

> > We probably can force disable animation while changing size mode [5] like
> > bug 1336230. 
> That sounds very useful to me! Somehow on OSX, the window resize animation
> _always_ plays, so perhaps it's different on that platform for some reason.

I'll start with this.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1342849

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to Bao Quan [:beekill] from comment #11)
> I think the bug you mentioned was solved here [1].

DOH! The world is moving so fast I can't keep up, it seems! I even reviewed that patch not too long ago... LOL.

> I'll start with this.

Cool.

Comment 13

[Bao Quan [:beekill]]

Attachment: suppress-animation.v1.patch

This patch disables animations while we're restoring window dimensions. Currently, it only disables animations on Windows and macOS but I'm not sure about macOS (I don't have a mac).

Jim, Markus, Mike suggested that you guys can review on this. Can you have a look at this?

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 8877513 [details] [diff] [review]
suppress-animation.v1.patch

Review of attachment 8877513 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on the nsIWidget + Mac changes.
I don't know if using DOMWindowUtils is the best API here, but I also can't think of a better API. On Mac you only have an animation when the window opens, not when it closes, so you don't really need to restore the animation behavior after session restore, so you could just override the window's macanimationtype attribute. But that would be a Mac-only solution and the solution that you have here is a bit more generic, so I prefer it.

::: widget/cocoa/nsCocoaWindow.mm
@@ +1284,5 @@
>    NS_OBJC_END_TRY_ABORT_BLOCK;
>  }
>  
> +void
> +nsCocoaWindow::SuppressAnimation(bool aEnable)

Let's call this aSuppress instead of aEnable.

::: widget/nsIWidget.h
@@ +831,5 @@
>      virtual void SetSizeMode(nsSizeMode aMode) = 0;
> +    
> +    /**
> +     * Suppress animations that are applied to a window by OS.
> +     * This is overrided in windows/nsWindow and cocoa/nsCocoaWindow

I don't think this line is necessary. nsIWidget methods are expected to be overridden by the subclasses that want to.

@@ +833,5 @@
> +    /**
> +     * Suppress animations that are applied to a window by OS.
> +     * This is overrided in windows/nsWindow and cocoa/nsCocoaWindow
> +     */
> +    virtual void SuppressAnimation(bool aEnable) {}

aSuppress here as well

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8877513 [details] [diff] [review]
suppress-animation.v1.patch

Review of attachment 8877513 [details] [diff] [review]:
-----------------------------------------------------------------

Code looks good, and when testing it out it works perfectly on OSX!

::: widget/nsIWidget.h
@@ +828,5 @@
>       * Minimize, maximize or normalize the window size.
>       * Takes a value from nsSizeMode (see nsIWidgetListener.h)
>       */
>      virtual void SetSizeMode(nsSizeMode aMode) = 0;
> +    

nit: superfluous whitespace.

::: widget/windows/nsWindow.h
@@ +132,5 @@
>                                            int32_t aHorizontal,
>                                            int32_t aVertical) override;
>    virtual void            PlaceBehind(nsTopLevelWidgetZPlacement aPlacement, nsIWidget *aWidget, bool aActivate) override;
>    virtual void            SetSizeMode(nsSizeMode aMode) override;
> +  virtual void            SuppressAnimation(bool aEnable) override;

I guess we would need `aSuppress` here as well?

Comment 16

[Jim Mathies [:jimm]]

Comment on attachment 8877513 [details] [diff] [review]
suppress-animation.v1.patch

Review of attachment 8877513 [details] [diff] [review]:
-----------------------------------------------------------------

f+ with suggestions.

::: browser/components/sessionstore/SessionStore.jsm
@@ +3936,5 @@
>        aHeight = bottom - aTop;
>      }
>  
> +    // Suppress animations
> +    domWindowUtils.suppressAnimation(true);

What if this script throws in here, can we revert the setting in a catch statement if something goes wrong?

::: widget/windows/nsWindow.cpp
@@ +2134,5 @@
>    }
>  }
>  
> +void
> +nsWindow::SuppressAnimation(bool aEnable)

We have two existing calls to this DWM api setting DWMWA_TRANSITIONS_FORCEDISABLED. Lets see if we can reuse this helper in those calls.

http://searchfox.org/mozilla-central/search?q=DWMWA_TRANSITIONS_FORCEDISABLED&path=

Comment 17

[Bao Quan [:beekill]]

Attachment: suppress-animation.v2.patch

I updated the patch with your suggestions. 

I'm not sure how to disable window animations on Linux. We probably could just ignore Linux platform [1]. Mike, do you know anyone who is familiar with Linux?

Do we need testcases to test this behavior on each platform?

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1336230

Comment 18

[Bao Quan [:beekill]]

(In reply to Jim Mathies [:jimm] from comment #16)
> Comment on attachment 8877513 [details] [diff] [review]
> suppress-animation.v1.patch
> 
> Review of attachment 8877513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f+ with suggestions.
> 
> ::: browser/components/sessionstore/SessionStore.jsm
> @@ +3936,5 @@
> >        aHeight = bottom - aTop;
> >      }
> >  
> > +    // Suppress animations
> > +    domWindowUtils.suppressAnimation(true);
> 
> What if this script throws in here, can we revert the setting in a catch
> statement if something goes wrong?

I don't understand why this could throw an exception?

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8878325 [details] [diff] [review]
suppress-animation.v2.patch

Review of attachment 8878325 [details] [diff] [review]:
-----------------------------------------------------------------

I think I already f+'d this ;-)

I also think we all agreed that you can get this patch ready for review! Markus already even r+'d the OSX bits for you! (Which is awesome.)

Let's leave Linux out of this for now. Meanwhile I'll look for a GTK expert.

Comment 20

[Bao Quan [:beekill]]

Attachment: suppress-animation.v3.patch

Ok, I understand what Jim meant. So if there is an exception is thrown during the restoration of window dimension, users will be left with a window without any animations. I put the code inside try ... finally ... block to ensure animations is back after restoration completes.

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880265 [details] [diff] [review]
suppress-animation.v3.patch

Review of attachment 8880265 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

::: browser/components/sessionstore/SessionStore.jsm
@@ +3887,5 @@
>      var _this = this;
>      function win_(aName) { return _this._getWindowDimension(win, aName); }
>  
> +    let domWindowUtils = win.QueryInterface(Ci.nsIInterfaceRequestor)
> +                         .getInterface(Ci.nsIDOMWindowUtils);

nit: please align the `.getInterface` with the dot in `.QueryInterface`. You may also shorten this to `const dwu` if you like, which is what most of us fondly abbreviate it to.

@@ +3937,5 @@
>      }
>  
> +    // Suppress animations.
> +    domWindowUtils.suppressAnimation(true);
> +  

nit: trailing whitespace.

Comment 22

[Bao Quan [:beekill]]

Attachment: suppress-animation.v4.patch

Fixed nits.

Comment 23

[Mike de Boer [:mikedeboer]]

Comment on attachment 8881211 [details] [diff] [review]
suppress-animation.v4.patch

Review of attachment 8881211 [details] [diff] [review]:
-----------------------------------------------------------------

Please update the patch with a proper commit message. Format: 'Bug XXX - Correct, explanatory message that allows other people to understand what this patch is about from a glance. r=reviewer1,reviewer2,reviewerX'

Comment 24

[Bao Quan [:beekill]]

Attachment: suppress-animation.v4.patch

Changed commit message. Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99538f5000df8eddbaae316ee0e5dd910f1b7f88

Comment 25

[Mike de Boer [:mikedeboer]]

Comment on attachment 8882779 [details] [diff] [review]
suppress-animation.v4.patch

Review of attachment 8882779 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! I'll request checkin.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad0560789e1e
Suppress window animations while we're restoring window dimension. r=mikedeboer,jimm,mstange

Comment 27

[Carsten Book [:Tomcat]]

had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8c574e38c77d7a2f1616e2b67fdb854e361a91ef&filter-searchStr=Linux+x64+asan+Executed+by+TaskCluster+build-linux64-asan-fuzzing%2Fopt+tc(Bof) for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=111634023&repo=mozilla-inbound from this or the other bug

Comment 28

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/283946b9d871
Backed out changeset ad0560789e1e

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/afad368bc897
Suppress window animations while we're restoring window dimension. r=mikedeboer,jimm,mstange

Comment 30

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

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