Enter/exit transitions for door hanger running at a very low frame-rate (see attached video). Unacceptable for a primary piece of user interface IMO. It's these kind of paper-cuts that makes Firefox feel like an unpolished product. Especially on a platform like OSX.

Comment on attachment 8724895 [details] janky-door-hanger.mp4 I should mention that this video is slowed down to 10% speed so we can see each frame of the animation.

Kats/dholbert: so let's say we want to do something about this without changing the appearance/contents of the doorhanger itself... where would we even start?

Based on my (limited) understanding of this, the doorhanger is animating both size and opacity. Opacity animation can be done in the compositor, but size cannot. So this animation has to run on the main thread, and we would have to basically make sure that each step of the animation sticks to the 16ms budget. The first step there is probably looking at the browser code that drives the animation (not sure if it's in CSS or JS, or both) and making sure it's not doing anything stupid. Following that a profile (using the Gecko profiler) would be useful to see where it's taking up time. This animation should be running in the parent process so at least it shouldn't be getting janked by content, we should be able to control everything that's competing with it.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3) > Based on my (limited) understanding of this, the doorhanger is animating > both size and opacity. Opacity animation can be done in the compositor, but > size cannot. Size as in transform? Because I'm fairly sure that's what it is, and it isn't obvious to me why that couldn't run in the compositor for panels, whose size changes do not impact the size of anything outside of them, if that makes sense. > So this animation has to run on the main thread, and we would > have to basically make sure that each step of the animation sticks to the > 16ms budget. The first step there is probably looking at the browser code > that drives the animation (not sure if it's in CSS or JS, or both) CSS only, I'm fairly sure. We use these panel animations everywhere. Specifically: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#454-460

If it's a transform then yeah it should be ok on the compositor. One thing to look for is a "Performance warning" output to stderr, there are bunch of them in nsDisplayList.cpp for various conditions where we can't run the animation on the compositor (e.g. [1]). [1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=12dfe2b87a79#5645

A gecko profile might help show where we're spinning our wheels here, too: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Reporting_a_Performance_Problem (FWIW, on Linux -- the platform that I'm on -- there's no animation here at all. The menu just pops open.)

(In reply to Daniel Holbert [:dholbert] from comment #6) > A gecko profile might help show where we're spinning our wheels here, too: > https://developer.mozilla.org/en-US/docs/Mozilla/Performance/ > Reporting_a_Performance_Problem > > (FWIW, on Linux -- the platform that I'm on -- there's no animation here at > all. The menu just pops open.) Yes, the animations are disabled on Linux because of "issues". Neil Deakin knows more about that.

(In reply to Wilson Page [:wilsonpage] from comment #0) > Created attachment 8724895 [details] > janky-door-hanger.mp4 > > Enter/exit transitions for door hanger running at a very low frame-rate (see > attached video). I would call this the hamburger or main menu. We tend to call the stuff that pops down from the icons in the location bar (NB: not the autocomplete popup, all the arrow panels) doorhangers. Those are also animated. Would you agree that those animations are substantially better and/or totally fine?

If you're talking about this doorhanger, it's better. Perhaps because it has less content that needs to reflow/transition. I would argue though that there's still a lack of polish. The drop shadow seems to fade-out independently of the white box, meaning the last frame of the animation is a black shadow. IMO, overall, the transition looks overly complex.

Comment on attachment 8725764 [details] janky-small-doorhanger.mp4 Video is slowed to 10% original speed. Although better, it looks as though it's dropping frames.

(In reply to Wilson Page [:wilsonpage] from comment #9) > I would argue though that there's still a lack of polish. The drop shadow > seems to fade-out independently of the white box, meaning the last frame of > the animation is a black shadow. You mean the "out" animation" rather than the "in" animation? > IMO, overall, the transition looks overly complex. Can you clarify what you mean? Concretely, what would you expect to be changed?

Is this only on OS X? I think this might be a dupe of https://bugzilla.mozilla.org/show_bug.cgi?id=1036929

(In reply to :Gijs Kruitbosch from comment #11) > You mean the "out" animation" rather than the "in" animation? Correct. > Can you clarify what you mean? Concretely, what would you expect to be > changed? I could live with the current transition if it didn't drop frames and the box-shadow didn't transition independently of the white content panel. But IMO the 'grow' transition is dated and reminds me of a stock jquery effect. A simple slide down (say 10px) and fade would look simpler :)

(In reply to Wilson Page [:wilsonpage] from comment #13) > (In reply to :Gijs Kruitbosch from comment #11) > > Can you clarify what you mean? Concretely, what would you expect to be > > changed? > > I could live with the current transition if it didn't drop frames and the > box-shadow didn't transition independently of the white content panel. But > IMO the 'grow' transition is dated and reminds me of a stock jquery effect. > A simple slide down (say 10px) and fade would look simpler :) It's pretty close to the native OS X effect, which is also fade + grow...

I don't really have a problem with the animation. But I agree that it's super janky. The shadow artifact in particular is bad. I think bug 1036929 has some conversation about that.

(In reply to Stephen Horlander [:shorlander] from comment #15) > I don't really have a problem with the animation. But I agree that it's > super janky. The shadow artifact in particular is bad. I think bug 1036929 > has some conversation about that. This bug is just tracking fixing the jank and glitches. Changes to the animation design is out of scope :)

(In reply to Wilson Page [:wilsonpage] from comment #16) > (In reply to Stephen Horlander [:shorlander] from comment #15) > > I don't really have a problem with the animation. But I agree that it's > > super janky. The shadow artifact in particular is bad. I think bug 1036929 > > has some conversation about that. > > This bug is just tracking fixing the jank and glitches. Changes to the > animation design is out of scope :) Except of course that simpler animations are simpler to speed up...

Could/Should we add this and/or the see also bug to the qx backlog?

(In reply to :Gijs Kruitbosch from comment #18) > Could/Should we add this and/or the see also bug to the qx backlog? I've just gone and added it per feedback from Jared. Let me know if you disagree! :-)

Using the Gecko Profiler, I ran /browser/components/customizableui/test/browser_panel_toggle.js 20 times. Here's the profiler output, https://cleopatra.io/#report=46ae772d6fcf4ca73f3ceaa30d0fd53c69f0269c

The previous profile restarts the test after 2 iterations each time, and there is some forced GC between tests. I modified the test and duplicated the add_task so the test would run with 249 openings/closings without having to restart the test. This is the associated profile for the 249 runs, https://cleopatra.io/#report=fad9206d14f8ba75463e08cec8f339b6c11862ee

On the JS side, we're spending 3.8% of the time in a flush that originates from panelUI._adjustContainerHeight() which is called from panelUI._syncContainerWithMainView().

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22) > On the JS side, we're spending 3.8% of the time in a flush that originates > from panelUI._adjustContainerHeight() which is called from > panelUI._syncContainerWithMainView(). But that's done onpopupshowing, which AIUI gets called right before or right after we start showing the popup - what's happening the other 150ms of the transition? Looking at the profiles in tight detail, it looks like we just spend a significant time painting each frame - longer than we really have. The presshell flush that results from just calling openPopup takes 10ms, then popup.xml adjusts the arrow position for the doorhanger and that causes another presshell flush, which then takes another 3 or so ms, and then we spend time painting. It would be useful if there were markers in the profile that indicated when the transitions in question started and ended though... right now it's quite hard to isolate a specific transition, and I'm not sure I'm getting it right. Can you add those to the test runs you're using?

_syncContainerWithMainView() isn't only called in onpopupshowing. There is also a MutationObserver that starts observing during onpopupshowing, and an overflow event listener that calls _syncContainerWithMainView(). I placed a breakpoint in _adjustContainerHeight() and it gets hit once during onpopupshowing, and then it got hit 4 more times afterwards, all from the MutationObserver (determined via commenting out both the overflow event listener and the MutationObserver). We are still building the panel after onpopupshowing, setting things like the zoomResetButton label, etc. We should move that and other similar work to onpopupshowing. [Leaving needinfo on for adding extra markers in the next profile]

These are the mutation observer records that are hit when opening the panel after starting up the browser: Observer event #1: attributes mutation on #zoom-reset-button with class of panel-combined-button[label] attributes mutation on #zoom-reset-button with class of toolbarbutton-icon[label] attributes mutation on #zoom-reset-button with class of toolbarbutton-text[value] attributes mutation on #zoom-reset-button with class of toolbarbutton-multiline-text[text] childList mutation on #zoom-reset-button with class of toolbarbutton-multiline-text Observer event #2: attributes mutation on #PanelUI-mainView with class of [style]

Observer event #1 occurs because we don't set the zoomResetButton label until a separate "popupshowing" event listener is reached. In my local patch, I have added a call to updateZoomResetButton() during onBuild so the value gets initialized earlier. I also had to modify updateZoomResetButton() to only set the label if the new value was different than the old value. Setting the identical value still triggers the MutationObserver. Observer event #2 comes from setting the height on the #PanelUI-mainView, and it is set on transitionend, http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.xml?rev=8cb42e7a16b4#232. I think it is safe to disregard this event. Fixing observer event #1 is not a difficult fix, but it would be nice to have a test that verifies no new mutation observer events happen while the popup is animating. I need to play around with this a bit more to see if I can figure out a way to do this with the right timing window.

I'm going to pick this back up.

Review commit: https://reviewboard.mozilla.org/r/68436/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68436/

To test this I took two approaches, first an automated test that is included in the patch. Secondly, I used the scratchpad to start the profiler, open and close the Firefox menu 10 times, then stop the profiler. This produced the following results: Without patch: min 3.98fps, avg 25.30fps, max 56.15fps With patch: min 10.05fps, avg 27.41fps, max 59.99fps The average difference isn't huge here, but going from 4fps in the worst case to 10fps is a sizable difference. Specifically though, we no longer stutter when opening the menu. We had this stutter because the transitionend event listener would trigger when the opacity transition finished, even though we were still in the middle of the transform transition. Part of this patch delays the sizing code to run until after the trasform transition has finished.

Comment on attachment 8776769 [details] Bug 1252224 - Remove synchronous layout flushes and style invalidations during the opening of multiview panels. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68436/diff/1-2/

https://reviewboard.mozilla.org/r/68436/#review65898 ::: browser/components/customizableui/content/panelUI.xml (Diff revision 2) > - // Ignore the mutation that'll fire when we set the height of > - // the main view. > - this.ignoreMutations = true; > - this._mainView.style.height = > - this.getBoundingClientRect().height + "px"; > - this.ignoreMutations = false; As a drive-by comment, aren't mutation observers currently called in a JS Microtask after the current script has finished executing? If that is correct, then this.ignoreMutations will never be true while the mutation observer is called. But I see this code is being removed anyways...

Comment on attachment 8776769 [details] Bug 1252224 - Remove synchronous layout flushes and style invalidations during the opening of multiview panels. https://reviewboard.mozilla.org/r/68436/#review65986 Thanks for looking at this, jaws. Man, it's been a while since I've looked at this stuff. This seems sensical - but I'm r-'ing because it appears to break the subview transition. For example, if I click on the Developer Tools button, the subview snaps in instead of sliding in. ::: browser/components/customizableui/test/browser_no_mutationrecords_during_panel_opening.js:7 (Diff revision 2) > +/** > + * Test opening and closing the menu panel UI. > + */ I think we can be more specific here - it tests that we don't get unexpected mutations during the panel opening and closing. ::: browser/components/customizableui/test/browser_no_mutationrecords_during_panel_opening.js:40 (Diff revision 2) > + let newValue = mutation.type == "attributes" ? > + mutation.target.getAttribute(mutation.attributeName) : > + mutation.type == "characterData" ? > + mutation.target.textContent : > + null; This nested ternary should probably be unbundled. It might be less terse, but it'll be wayyyy easier to read. ::: browser/components/customizableui/test/browser_no_mutationrecords_during_panel_opening.js:70 (Diff revision 2) > + let shownPromise = promisePanelShown(window); > + PanelUI.show(); > + yield shownPromise; > + observer.disconnect(); > + > + is(failures, 0, "There should be no mutation events during opening of the panel"); "no mutation events" -> "no unexpected mutation events", since there should be _some_ mutation events. ::: browser/components/customizableui/test/browser_no_mutationrecords_during_panel_opening.js:74 (Diff revision 2) > + let hiddenPromise = promisePanelHidden(window); > + PanelUI.hide(); > + yield hiddenPromise; Why is this a separate task? Should we not just put this into the end of the previous task?

Comment on attachment 8776769 [details] Bug 1252224 - Remove synchronous layout flushes and style invalidations during the opening of multiview panels. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68436/diff/2-3/

https://reviewboard.mozilla.org/r/68436/#review65986 > Why is this a separate task? Should we not just put this into the end of the previous task? I have it as a separate task just to make sure that if there is an exception the test will clean up after itself.

(In reply to :Paolo Amadini from comment #33) > https://reviewboard.mozilla.org/r/68436/#review65898 > > ::: browser/components/customizableui/content/panelUI.xml > (Diff revision 2) > > - // Ignore the mutation that'll fire when we set the height of > > - // the main view. > > - this.ignoreMutations = true; > > - this._mainView.style.height = > > - this.getBoundingClientRect().height + "px"; > > - this.ignoreMutations = false; > > As a drive-by comment, aren't mutation observers currently called in a JS > Microtask after the current script has finished executing? If that is > correct, then this.ignoreMutations will never be true while the mutation > observer is called. > > But I see this code is being removed anyways... Yeah, there is a lot wrong with this function. Not much can be salvaged, but the control center panel is depending on the height to be set here so I added the getBoundingClientRect() line to the control center to keep it functioning. (In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #34) > This seems sensical - but I'm r-'ing because it appears to break the subview > transition. For example, if I click on the Developer Tools button, the > subview snaps in instead of sliding in. This was broken because I was setting the panelopen attribute directly on the panel, instead of on the binding so the XBL attribute inheritance wasn't propagating down to the descendants. This is now fixed.

Comment on attachment 8776769 [details] Bug 1252224 - Remove synchronous layout flushes and style invalidations during the opening of multiview panels. https://reviewboard.mozilla.org/r/68436/#review66770 This looks okay to me and tests fine. I trust that we're removing some layout jank, but I'll admit that I didn't feel much of a performance win when comparing to a build without this patch (on OS X). Perhaps this has a bigger impact on Windows. Thanks Jared! ::: browser/base/content/browser.js:7231 (Diff revision 3) > + this._identityPopupMultiView._mainView.style.height = > + this._identityPopup.getBoundingClientRect().height + "px"; Out of curiosity, why does the identity panel need this special treatment?

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #38) > ::: browser/base/content/browser.js:7231 > (Diff revision 3) > > + this._identityPopupMultiView._mainView.style.height = > > + this._identityPopup.getBoundingClientRect().height + "px"; > > Out of curiosity, why does the identity panel need this special treatment? https://bugzilla.mozilla.org/show_bug.cgi?id=1206233#c28 is dealing with flex and a set height. Without this line, the panel clips the multiline text. This work is being tracked in bug 1206233.

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d1923f89225c Remove synchronous layout flushes and style invalidations during the opening of multiview panels. r=mconley

Backed out for failures in e.g. browser_no_mutationrecords_during_panel_opening.js: https://hg.mozilla.org/integration/autoland/rev/99ed3a3aa3e82bd75ff3077ff0d1b97c637fa6a1 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d1923f89225c95667e7f573a03d9ace23e05b6a9 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=1496841&repo=autoland

https://hg.mozilla.org/integration/fx-team/rev/04fec252a1c609885454ed49bc36d69aee0206e1 Bug 1252224 - Remove synchronous layout flushes and style invalidations during the opening of multiview panels. r=mconley

https://hg.mozilla.org/integration/fx-team/rev/8deed228b6924da2aab7485d1d5779d8293fce49 Backout 04fec252a1c6 (bug 1252224) due to wrong patch being pushed. r=me https://hg.mozilla.org/integration/fx-team/rev/429d054ed34d2474d5521d89ece0926987068103 Bug 1252224 - Remove synchronous layout flushes and style invalidations during the opening of multiview panels. r=mconley

Backed out again in https://hg.mozilla.org/integration/fx-team/rev/e0e5d6619a32 for failures: https://treeherder.mozilla.org/logviewer.html#?job_id=11045132&repo=fx-team

Wasn't just Windows failures, apparently: https://treeherder.mozilla.org/logviewer.html#?job_id=11045346&repo=fx-team

Kris, do you know why these changes would be breaking your test?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #47) > Kris, do you know why these changes would be breaking your test? I'm not entirely sure what causes the difference in height, but it looks like we're no longer calculating the main view height at popupshown (which happens just before that test checks the initial size of the popup). So something must be happening between the showSubView call and the first mutation event that makes a sub-pixel difference in the layout size. It might be enough to just add a timeout before checking the initial size in the test, or to force a mutation just to make sure everything is in sync.

https://hg.mozilla.org/integration/fx-team/rev/0c4f81bc2b52a7690436569452e6388c54f9ec96 Bug 1252224 - Remove synchronous layout flushes and style invalidations during the opening of multiview panels. r=mconley

Backed this out because for permafailing in browser_ext_browserAction_popup_resize.js on Linux after merge: https://hg.mozilla.org/mozilla-central/rev/6e191a55c3d23e83e6a2e72e4e80c1dc21516493 Merge with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=2ed7e61b988d2466a61528f66050596ef272ebda Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=4642803&repo=mozilla-central 16:16:55 INFO - 153 INFO TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Document should be vertically scrollable (1518 > 0) - 16:16:55 INFO - 154 INFO TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Panel arrow is positioned as expected - 16:16:55 INFO - 155 INFO TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | 41 should be greater than or within one pixel of 41: Panel has not moved upwards - 16:16:55 INFO - 156 INFO TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | -499 should be greater than or within one pixel of -884: Panel has not shrunk from original size - 16:16:55 INFO - 157 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Top of popup should be on-screen. (15 >= 24) - 16:16:55 INFO - Stack trace: 16:16:55 INFO - chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:testPopupSize/checkPanelPosition:197 16:16:55 INFO - chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:testPopupSize:259

I've noticed that the password manager and control centre doorhangers are really janky during transitions when pressing the "ESC" key [1]. Pressing the "X" rather than using the "ESC" key closes the doorhanger without any of the jank/sluggish transitions/animations. I'm only seeing this happen under macOS 10.11.6 and doesn't look like it's happening under the Win 10/Ubuntu VM's. Is what I'm seeing related to this bug? Or does it warrant a separate bug? [1] https://youtu.be/OZVMX05tRQk

Can you file a separate bug for that? Most of the bugs I've seen filed are dealing with the opening of the panel.

I'm gonna convert this to a meta bug because there appear to be numerous things that should be investigated and each one is somewhat disjoint.