Closed Bug 1391704 Opened 4 years ago Closed 3 years ago

Avoid flickering while moving tabs across windows


(Firefox :: Tabbed Browser, enhancement, P1)




Firefox 57
57.3 - Sep 19
Tracking Status
firefox57 --- verified


(Reporter: florian, Assigned: florian)


(Depends on 2 open bugs, Blocks 2 open bugs)


(Whiteboard: [photon-performance])


(1 file, 1 obsolete file)

The experience of dragging a tab from a window to another could be a lot smoother.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8898903 - Flags: review?(mconley)
Flags: qe-verify+
Whiteboard: [photon-performance]
Whiteboard: [photon-performance] → [photon-performance] [triage]
Attached patch Patch v2Splinter Review
Updated patch to fix issues noticed on my previous try run, should hopefully be greener this time:
Attachment #8899792 - Flags: review?(mconley)
Attachment #8898903 - Attachment is obsolete: true
Attachment #8898903 - Flags: review?(mconley)
QA Contact: adrian.florinescu
Iteration: --- → 57.2 - Aug 29
Priority: -- → P1
Whiteboard: [photon-performance] [triage] → [photon-performance]
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.
Attachment #8899792 - Flags: review?(mconley) → review+
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
Pushed by
Avoid flickering while moving tabs across windows, r=mconley.
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(In reply to Pulsebot from comment #4)
> Pushed by
> 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 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 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.
Depends on: 1400209
No longer depends on: 1400209
See Also: → 1400209
Depends on: 1400242
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;
Flags: qe-verify+
Depends on: 1467206
Depends on: 1469970
Depends on: 1475589
You need to log in before you can comment on or make changes to this bug.