1391704 - Avoid flickering while moving tabs across windows

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, enhancement, P1)

Description

[Florian Quèze [:florian]]

The experience of dragging a tab from a window to another could be a lot smoother.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

Changes made by the patch:

When detaching a tab to a new window:
- skip the window opening animation (only if the tab has been detached by dragging, detaching from the "Move to New Window" context menu item will still play the window opening animation)
- swap the docshells during the new window's onload event instead of during _delayedStartup, so that the new window is painted with the right content the first time.
- play the tab closing animation in the old window, to give immediate feedback to the user and 'hide' the time it takes for the new window to open.

When moving the last tab of a window to another window:
- instead of the current slow tab close code path that replaces the last tab with a blank one (which is actually painted on the old window that's still on the foreground), hide the window and close it. (if I close the window with window animations disabled, I still get one frame with the content are painted blank, whereas hiding the window happens without any repaint before it disappears; at least on Mac).

When adopting a tab:
- skip the tab open animation: the web page is painted immediately in the content area, so the tab open animation is useless (the user doesn't need to wait) and distracting.

Comment 2

[Florian Quèze [:florian]]

Attachment: Patch v2

Updated patch to fix issues noticed on my previous try run, should hopefully be greener this time:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=976245e321c763e69f3c91a29d33e5ccab64b006

Comment 3

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

Comment on attachment 8899792 [details] [diff] [review]
Patch v2

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

This feels really nice when I use it. Thanks for your work here, florian.

It'd be really great if there was a way of ensuring this level of smoothness going forward with automated tests, but I'm not sure how we do that in the short term.

::: browser/base/content/browser.js
@@ +1407,5 @@
> +
> +      try {
> +        // Make sure selectedBrowser has the same remote settings as the one
> +        // we are swapping in.
> +        gBrowser.updateBrowserRemoteness(gBrowser.selectedBrowser,

It looks like we're already doing some similar work in onDOMContentLoaded... is it possible to combine efforts there? At the very least, I think that'll ensure that we don't accidentally get into a case where we can updateBrowserRemoteness once in onDOMContentLoaded and again here in onLoad.

::: browser/base/content/tabbrowser.xml
@@ +2996,5 @@
>              }
> +            aTab._endRemoveArgs = [closeWindow, newTab];
> +
> +            // swapBrowsersAndCloseOther will take care of closing the window without animation.
> +            if (closeWindow && aAdoptedByTab) {

Nice optimization here!

::: browser/components/customizableui/content/panelUI.js
@@ +319,5 @@
>     * @return a Promise that resolves once the panel is ready to roll.
>     */
> +  async ensureReady() {
> +    if (this._isReady)
> +      return;

Nit: From a glance, it looks like this file does _mostly_ one-liner bracing, so we should probably keep doing that.

@@ +321,5 @@
> +  async ensureReady() {
> +    if (this._isReady)
> +      return;
> +
> +    await delayedStartupPromise;

Another nit: I know that this default to walking up to the window scope and getting window.delayedStartupPromise, but my brain first assumed that this was some locally scoped variable. Can we make this explicit, like:

await window.delayedStartupPromise;

instead? Also, I seem to recall hearing that it's (maybe insignificantly) better to call out window scoping variables like this because it means that the JS engine doesn't have to go up the scopes looking for the first match.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d64ad536c158
Avoid flickering while moving tabs across windows, r=mconley.

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 6

[Florian Quèze [:florian]]

(In reply to Pulsebot from comment #4)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d64ad536c158
> Avoid flickering while moving tabs across windows, r=mconley.

In addition to addressing comment 3, I had to add the this._tabFilters.delete call at https://hg.mozilla.org/mozilla-central/rev/d64ad536c158#l2.16 to plug leaks that were visible on Windows debug with e10s disabled on try.

I also had to do a non-trivial unbitrotting (that ended up simplifying the patch) due to the conflicts with bug 1054740 and bug 1393802 that landed while I was debugging the leak.

When testing on Windows, nsIBaseWindow.visibility = false wasn't enough to hide the window without animation, I had to add a nsIDOMWindowUtils.suppressAnimation call before changing the visibility.

Given that the changes were a bit more than I was comfortable carrying the existing r+ forward for, I got felipe to look at this over IRC. He requested that I add a comment in _delayedStartup to explain why we no longer check for a XULElement uri there (which I did at https://hg.mozilla.org/mozilla-central/rev/d64ad536c158#l1.48) and said "with that, you can call r=me for the changes".

Just before landing, I noticed on my macbook that I still randomly had a white about:blank flash on the closing window. I fixed that by moving the nsIDOMWindowUtils.suppressAnimation call and nsIBaseWindow.visibility = false before the docshell swap.

Comment 7

[Adrian Florinescu [:aflorinescu]]

Environment:
OSx10.12, Windows 10(Qf machine), Ubuntu 16.04x64, Windows 8.1x64, Windows 7x64;
FF 57 09.15.2017 

This issue has been verified by exploratory testing and also covered by smoke testing during the pre-Beta smoke test run(09.15.2017):
-detaching tab behavior - drag and drop && "move to new window";
-re-attaching tab behavior;
-closing tab behavior;
-new window first paint;