Closed
Bug 1391704
Opened 7 years ago
Closed 7 years ago
Avoid flickering while moving tabs across windows
Categories
(Firefox :: Tabbed Browser, enhancement, P1)
Firefox
Tabbed Browser
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | verified |
People
(Reporter: florian, Assigned: florian)
References
(Depends on 1 open bug, Blocks 2 open bugs)
Details
(Whiteboard: [photon-performance])
Attachments
(1 file, 1 obsolete file)
14.95 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
The experience of dragging a tab from a window to another could be a lot smoother.
Assignee | ||
Comment 1•7 years ago
|
||
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•7 years ago
|
Flags: qe-verify+
Whiteboard: [photon-performance]
Updated•7 years ago
|
Whiteboard: [photon-performance] → [photon-performance] [triage]
Assignee | ||
Comment 2•7 years ago
|
||
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•7 years ago
|
Attachment #8898903 -
Attachment is obsolete: true
Attachment #8898903 -
Flags: review?(mconley)
Updated•7 years ago
|
QA Contact: adrian.florinescu
Updated•7 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 57.2 - Aug 29
Priority: -- → P1
Whiteboard: [photon-performance] [triage] → [photon-performance]
Comment 3•7 years ago
|
||
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•7 years ago
|
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
Pushed by florian@queze.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/d64ad536c158 Avoid flickering while moving tabs across windows, r=mconley.
![]() |
||
Comment 5•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d64ad536c158
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Assignee | ||
Comment 6•7 years 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.
Comment 7•7 years ago
|
||
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;
Blocks: 1316126
You need to log in
before you can comment on or make changes to this bug.
Description
•