Closed Bug 1331932 Opened 3 years ago Closed 2 years ago

Disable UI animations during session restore

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: mikedeboer, Assigned: beekill, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

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.
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. :-)
Flags: needinfo?(mconley)
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.
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).
(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.
Flags: needinfo?(mconley)
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...
See Also: → 1336230
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
Flags: needinfo?(ttaubert)
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(ttaubert)
(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.
Ah, thanks for the correction. Great to see only a single reflow left in there!
(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.
Flags: needinfo?(mdeboer)
(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
(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.
Attached patch suppress-animation.v1.patch (obsolete) — Splinter Review
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?
Attachment #8877513 - Flags: feedback?(mstange)
Attachment #8877513 - Flags: feedback?(mdeboer)
Attachment #8877513 - Flags: feedback?(jmathies)
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
Attachment #8877513 - Flags: feedback?(mstange) → feedback+
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?
Attachment #8877513 - Flags: feedback?(mdeboer) → feedback+
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=
Attachment #8877513 - Flags: feedback?(jmathies) → feedback+
Attached patch suppress-animation.v2.patch (obsolete) — Splinter Review
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
Attachment #8877513 - Attachment is obsolete: true
Attachment #8878325 - Flags: feedback?(mstange)
Attachment #8878325 - Flags: feedback?(mdeboer)
Attachment #8878325 - Flags: feedback?(jmathies)
(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 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.
Attachment #8878325 - Flags: feedback?(mstange)
Attachment #8878325 - Flags: feedback?(mdeboer)
Attachment #8878325 - Flags: feedback+
Attached patch suppress-animation.v3.patch (obsolete) — Splinter Review
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.
Attachment #8878325 - Attachment is obsolete: true
Attachment #8878325 - Flags: feedback?(jmathies)
Attachment #8880265 - Flags: review?(mdeboer)
Attachment #8880265 - Flags: review?(jmathies)
Assignee: nobody → nnn_bikiu0707
Status: NEW → ASSIGNED
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.
Attachment #8880265 - Flags: review?(mdeboer)
Attachment #8880265 - Flags: review?(jmathies)
Attachment #8880265 - Flags: review+
Attachment #8880265 - Flags: review+ → review?(jmathies)
Attachment #8880265 - Flags: review?(jmathies) → review+
Attached patch suppress-animation.v4.patch (obsolete) — Splinter Review
Fixed nits.
Attachment #8880265 - Attachment is obsolete: true
Attachment #8881211 - Flags: review?(mdeboer)
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'
Attachment #8881211 - Flags: review?(mdeboer) → review-
Changed commit message. Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99538f5000df8eddbaae316ee0e5dd910f1b7f88
Attachment #8881211 - Attachment is obsolete: true
Attachment #8882779 - Flags: review?(mdeboer)
Comment on attachment 8882779 [details] [diff] [review]
suppress-animation.v4.patch

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

Nice! I'll request checkin.
Attachment #8882779 - Flags: review?(mdeboer) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad0560789e1e
Suppress window animations while we're restoring window dimension. r=mikedeboer,jimm,mstange
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/afad368bc897
Suppress window animations while we're restoring window dimension. r=mikedeboer,jimm,mstange
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/afad368bc897
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.