Avoid flickering while moving tabs across windows

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Tabbed Browser
P1
normal
VERIFIED FIXED
11 months ago
6 days ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Depends on: 4 bugs, Blocks: 2 bugs)

Trunk
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-performance])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

11 months ago
The experience of dragging a tab from a window to another could be a lot smoother.
(Assignee)

Comment 1

11 months ago
Created attachment 8898903 [details] [diff] [review]
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.
Attachment #8898903 - Flags: review?(mconley)
(Assignee)

Updated

11 months ago
Flags: qe-verify+
Whiteboard: [photon-performance]

Updated

11 months ago
Whiteboard: [photon-performance] → [photon-performance] [triage]
(Assignee)

Comment 2

11 months ago
Created attachment 8899792 [details] [diff] [review]
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
Attachment #8899792 - Flags: review?(mconley)
(Assignee)

Updated

11 months ago
Attachment #8898903 - Attachment is obsolete: true
Attachment #8898903 - Flags: review?(mconley)
QA Contact: adrian.florinescu

Updated

11 months ago
Status: NEW → ASSIGNED
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+

Updated

11 months ago
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19

Comment 4

11 months ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d64ad536c158
Avoid flickering while moving tabs across windows, r=mconley.
https://hg.mozilla.org/mozilla-central/rev/d64ad536c158
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Comment 6

11 months ago
(In reply to Pulsebot from comment #4)
> Pushed by florian@queze.net:
> 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.
Depends on: 1400209
No longer depends on: 1400209
See Also: → bug 1400209

Updated

10 months ago
Depends on: 1400242
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;
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
Depends on: 1459327

Updated

a month ago
Depends on: 1467206

Updated

29 days ago
Depends on: 1469970

Updated

6 days ago
Depends on: 1475589
You need to log in before you can comment on or make changes to this bug.