Please update panel animation for the door hanger sub menus as part of the Photon motion design refresh. See attached video for reference.

Updated the Menu Spec here: https://mozilla.invisionapp.com/share/24BS7S8ZH#/229252106_Menus.

Comment on attachment 8888825 [details] Bug 1374749 - Animate the panelviews differently to make it look as if the view to show is pushing the previous view out of the panel. https://reviewboard.mozilla.org/r/159856/#review165190 Haven't tested but here's some initial feedback. ::: browser/base/content/browser.css:124 (Diff revision 1) > } > > +%ifndef MOZ_PHOTON_THEME > panelview:not([mainview]):not([current]) { > +%else > +panelview:not([current]) { How does this work with the not-from/to-mainview transitions (so 3rd level or so?) ::: browser/components/customizableui/PanelMultiView.jsm:568 (Diff revision 1) > + let movementX = previousRect.width; > + let moveX = (moveToLeft ? "" : "-") + movementX; argh, my brain... "moveX" vs "movementX" ? You use one in some places, the other in the other places... it's confusing. Can you clarify these names? ::: browser/components/customizableui/PanelMultiView.jsm:570 (Diff revision 1) > > - if (!reverse) { > + // The 'magic' part: build up the amount of pixels to move right or left. > + let moveToLeft = (this._dir == "rtl" && !reverse) || (this._dir == "ltr" && reverse); > + let movementX = previousRect.width; > + let moveX = (moveToLeft ? "" : "-") + movementX; > + let nodeAnimatingOut = moveToLeft ? previousViewNode : viewNode; To me, "out" means "out of the viewport", but I think from this code that you think it means the node that is soon to become the current node... can we think of a name that's less confusing? ::: browser/components/customizableui/PanelMultiView.jsm:588 (Diff revision 1) > // Somehow, putting these properties in PanelUI.css doesn't work for > // newly shown nodes in a XUL parent node. > - nodeToAnimate.style.transition = "transform ease-" + (reverse ? "in" : "out") + > + this._viewStack.style.transition = "transform ease-" + (reverse ? "in" : "out") + > " var(--panelui-subview-transition-duration)"; > - nodeToAnimate.style.willChange = "transform"; > - nodeToAnimate.style.borderInlineStart = "1px solid var(--panel-separator-color)"; > + this._viewStack.style.willChange = "transform"; > + nodeAnimatingOut.style.borderInlineStart = "1px solid var(--panel-separator-color)"; This border will be different on rtl, but the node depends only on the direction of movement. Logically, I think that means this migth not be the right border. :-) ::: browser/components/customizableui/PanelMultiView.jsm:592 (Diff revision 1) > - nodeToAnimate.style.willChange = "transform"; > - nodeToAnimate.style.borderInlineStart = "1px solid var(--panel-separator-color)"; > + this._viewStack.style.willChange = "transform"; > + nodeAnimatingOut.style.borderInlineStart = "1px solid var(--panel-separator-color)"; > > // Wait until after the first paint to ensure setting 'current=true' > // has taken full effect; once both views are visible, we want to > // correctly measure rects using `dwu.getBoundsWithoutFlushing`. I'm sure it's just me, but I don't think we do any bounds checks anymore here? So should we just omit this extra mozafterpaint wait?

If I press ESC very rapidly while the panel is transitioning, it is not displayed correctly the next time it is opened. Since the panel recovers, this is not a blocker for landing, but it's something you may want to look into and it has to be fixed.

OK, sounds like something that'd be an issue with the current animation as well. With not blocking, you mean that this issue can be fixed in a follow-up bug right after this patch has landed, right?

Comment on attachment 8888825 [details] Bug 1374749 - Animate the panelviews differently to make it look as if the view to show is pushing the previous view out of the panel. https://reviewboard.mozilla.org/r/159856/#review181278 ::: browser/components/customizableui/PanelMultiView.jsm:537 (Diff revision 2) > // must grow to meet that new height. Otherwise, it must shrink. > // > // All three of these actions make use of CSS transformations, so they > // should all occur simultaneously. > if (this.panelViews && playTransition) { > - // Sliding the next subview in means that the previous panelview stays > + this._transitionViews(previousViewNode, viewNode, aAnchor, reverse, previousRect, () => { It seems better to set the "open" attribute before calling the function and resetting it at the beginning of the callback. This way, you can remove the aAnchor parameter. (No need to check whether the panel is open or not.) ::: browser/components/customizableui/PanelMultiView.jsm:613 (Diff revision 2) > - let nodeToAnimate = reverse ? previousViewNode : viewNode; > > - if (!reverse) { > - // We set the margin here to make sure the view is positioned next > - // to the view that is currently visible. The animation is taken > - // care of by transitioning the `transform: translateX()` property > + // The 'magic' part: build up the amount of pixels to move right or left. > + let moveToLeft = (this._dir == "rtl" && !reverse) || (this._dir == "ltr" && reverse); > + let deltaX = previousRect.width; > + let nodeAnimatingOut = moveToLeft ? previousViewNode : viewNode; I agree with Gijs that the border should be on the same logical element when the views slide in or out. Use "reverse" to choose the deepest node and set borderInlineStart. (At some point we may be able to move these back to be CSS properties that depend on a "transitioning" attribute or something.) ::: browser/components/customizableui/PanelMultiView.jsm:630 (Diff revision 2) > - // Wait until after the first paint to ensure setting 'current=true' > - // has taken full effect; once both views are visible, we want to > + // Wait until after the first paint to ensure setting 'current=true' has > + // taken full effect. > - // correctly measure rects using `dwu.getBoundsWithoutFlushing`. > - window.addEventListener("MozAfterPaint", () => { > + window.addEventListener("MozAfterPaint", () => { Shouldn't this use a varioation of BrowserUtils.promiseLayoutFlushed too? ::: browser/components/customizableui/PanelMultiView.jsm:664 (Diff revision 2) > > + // We force 'display: none' on the previous view node to make sure that > + // it doesn't cause an annoying flicker whilst resetting the styles below. > + previousViewNode.style.display = "none"; > + // We need a layout flush to get that done. > + BrowserUtils.promiseLayoutFlushed(document, "layout", () => { All the uses of promiseLayoutFlushed are incorrect, because the callback should never modify the DOM. You should wait on the returned Promise instead. ::: browser/components/customizableui/content/panelUI.css:32 (Diff revision 2) > transition: transform var(--panelui-subview-transition-duration); > } > > .panel-viewcontainer[panelopen]:-moz-any(:not([viewtype="main"]),[transitioning]) { > transition-property: height; > - transition-timing-function: ease-in; > + transition-timing-function: var(--animation-easing-function); If I slow down the animation to take 3 seconds, I can see that it spends most of the time near the end, compared to the original animation, that is mostly uniform when slowed down in the same way. Which of these is the correct easing function? ::: browser/components/customizableui/content/panelUI.xml:57 (Diff revision 2) > </binding> > > <binding id="photonpanelmultiview" extends="chrome://browser/content/customizableui/panelUI.xml#panelmultiview"> > <content> > <xul:box anonid="viewContainer" class="panel-viewcontainer" xbl:inherits="panelopen,transitioning"> > - <xul:stack anonid="viewStack" xbl:inherits="transitioning" class="panel-viewstack"> > + <xul:box anonid="viewStack" xbl:inherits="transitioning" class="panel-viewstack"> I have tested the Identity Block and it seems to work correctly. Are there any -moz-stack-sizing leftovers to clean up?

(In reply to :Paolo Amadini from comment #7) > If I slow down the animation to take 3 seconds, I can see that it spends > most of the time near the end, compared to the original animation, that is > mostly uniform when slowed down in the same way. Which of these is the > correct easing function? Basically, UX/ Amy Lee asked me to use the Photon (this one) everywhere.

(In reply to Mike de Boer [:mikedeboer] from comment #6) > OK, sounds like something that'd be an issue with the current animation as > well. With not blocking, you mean that this issue can be fixed in a > follow-up bug right after this patch has landed, right? I couldn't reproduce on the current version, but yes, if this is not trivial to figure out then filing a follow-up bug is fine.

Comment on attachment 8888825 [details] Bug 1374749 - Animate the panelviews differently to make it look as if the view to show is pushing the previous view out of the panel. https://reviewboard.mozilla.org/r/159856/#review182248 ::: browser/components/customizableui/PanelMultiView.jsm:19 (Diff revision 3) > +const TRANSITION_PHASES = { > + START: 1, > + PREPARE: 2, > + TRANSITION: 3, > + END: 4, > + CLEANUP: 5 > +}; > + I think a better approach here would be to use a non-reentrant async function and check for a "fast forward" parameter during the execution. this.transitionPromise = Promise.resolve(); async function transition() { this.fastForward = false; await doFirstThing(); if (!this.fastForward) { doNextThing(); if (!this.fastForward) { await doLastThing(); } reverseNextThing(); } await reverseFirstThing(); } this.fastForward = true; this.transitionPromise = this.transitionPromise.then(transition); ::: browser/components/customizableui/PanelMultiView.jsm:580 (Diff revision 3) > + * Sliding the next subview in means that the previous panelview stays where it > + * is and the active panelview slides in from the left in LTR mode, right in > + * RTL mode. This seems to describe the old animation? ::: browser/components/customizableui/PanelMultiView.jsm:726 (Diff revision 3) > - this._autoResizeWorkaroundTimer = window.setTimeout(() => { > + this._autoResizeWorkaroundTimer = this.window.setTimeout(() => { > + if (!this._viewContainer) > + return; > - this._viewContainer.style.removeProperty("height"); > + this._viewContainer.style.removeProperty("height"); > - this._viewContainer.style.removeProperty("width"); > + this._viewContainer.style.removeProperty("width"); > - }, 500); > + }, 500); We may be able to replace this timeout with another wait for layout now that we have this function available, which we didn't at the time. ::: browser/components/customizableui/PanelMultiView.jsm:815 (Diff revision 3) > - BrowserUtils.promiseLayoutFlushed(this.document, "layout", () => { > - return this._dwu.getBoundsWithoutFlushing(viewNode); > - }).then(viewRect => { > + BrowserUtils.whenLayoutFlushed(this.document, "layout", () => { > + let viewRect = this._dwu.getBoundsWithoutFlushing(viewNode); > + The original code is the correct way of using promiseLayoutFlushed. Basically, if something else is using promiseLayoutFlushed at the same time, all the layout reads are done together, and then all the DOM writes are done together later, when the returned Promises are resolved. Doing any DOM write inside the callback may result in difficult to diagnose intermittent sync reflows if some other caller is reading the state directly instead of using getBoundsWithoutFlushing.

Paolo, I can't in any way shape or form possible get smooth transitions that _never_ glitches without doing it the way I'm doing it here. I'm out of ideas and it feels like you have a strong grasp on the problem space and can async/await-ify this code better than I can with my current capabilities. As to avoid the risk to waste too much time on this further, I'm unassigning myself and hope you will be able to find the time to move things forward.

I think most of the issues you observed were related to the fact that the "current" attribute, which controls the visibility of the view, was set too early. In general, the way I approached the problem was by replacing the calls to promiseLayoutFlushed with "await new Promise(r => setTimeout(r, 5000))" and see if the painted state was consistent. You need layout flushes after you are ready to the start a new animation and before you start it, but at the same time you have to ensure that all other DOM changes are atomic, for instance there should be no await statement between showing a view and moving its DOM node to the off-screen area. To guarantee this, most of the code had to be moved around. With the attached preliminary patch the result looks quite stable and free of glitches to me. Mike, as I am going away soon, can you take this from here and work with Gijs to land it? The issue with the arrow panel resizing unexpectedly at the end still exists, so I had to keep the 500ms timer for now. Note that for the case where the subview is off-screen we can build a new asynchronous version of the description height workaround, but I suggest doing this in a separate bug or changeset.

Also, while I implemented the fast-forward approach for the only case that needed it, which is the wait for the animation, there is still no strong protection against unexpected re-entrancy during any of the "await" statements, you may want to add it as I suggested in the previous review.

Paolo proved an important point, which makes me able to move forward and finish this up for realz!

Comment on attachment 8888825 [details] Bug 1374749 - Animate the panelviews differently to make it look as if the view to show is pushing the previous view out of the panel. https://reviewboard.mozilla.org/r/159856/#review182812 Looks good, tested it out and it works nice. r=me ::: browser/components/customizableui/PanelMultiView.jsm:19 (Diff revision 6) > XPCOMUtils.defineLazyModuleGetter(this, "BrowserUtils", > "resource://gre/modules/BrowserUtils.jsm"); > XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI", > "resource:///modules/CustomizableUI.jsm"); > > +const TRANSITION_PHASES = { const TRANSITION_PHASES = Object.freeze({ ... }); ??? Just a suggestion ;) ::: browser/components/customizableui/PanelMultiView.jsm:351 (Diff revision 6) > - this._subViews = this._viewStack = this.__dwu = this._panelViewCache = null; > + this._subViews = this._viewStack = this.__dwu = this._panelViewCache = > + this._transitionDetails = null; Well, this is quite the chain! Can you indent the `this._transitionDetails` line two more spaces so it is clear that this statement is continuing across the line break? ::: browser/components/customizableui/PanelMultiView.jsm:351 (Diff revision 6) > this._panel.removeEventListener("popupshowing", this); > this._panel.removeEventListener("popupshown", this); > this._panel.removeEventListener("popuphidden", this); > this.window.removeEventListener("keydown", this); > this.node = this._clickCapturer = this._viewContainer = this._mainViewContainer = > - this._subViews = this._viewStack = this.__dwu = this._panelViewCache = null; > + this._subViews = this._viewStack = this.__dwu = this._panelViewCache = `this._dwu`, not `this.__dwu`. I don't see any other instance of `this.__dwu` in this patch. ::: browser/components/customizableui/PanelMultiView.jsm:731 (Diff revision 6) > - // Now that the subview is visible, we can check the height of the > - // description elements it contains. > + if (phase >= TRANSITION_PHASES.PREPARE) { > + this._transitioning = false; Curious, if the phase is >= PREPARE, why are we setting `this._transitioning = false;` ?

Comment on attachment 8888825 [details] Bug 1374749 - Animate the panelviews differently to make it look as if the view to show is pushing the previous view out of the panel. https://reviewboard.mozilla.org/r/159856/#review182828 Thanks for the review, Jared! \o/ I'll push an update soon. ::: browser/components/customizableui/PanelMultiView.jsm:351 (Diff revision 6) > this._panel.removeEventListener("popupshowing", this); > this._panel.removeEventListener("popupshown", this); > this._panel.removeEventListener("popuphidden", this); > this.window.removeEventListener("keydown", this); > this.node = this._clickCapturer = this._viewContainer = this._mainViewContainer = > - this._subViews = this._viewStack = this.__dwu = this._panelViewCache = null; > + this._subViews = this._viewStack = this.__dwu = this._panelViewCache = That's because there's only a `_dwu` getter, not a setter. Inside the getter we cache the object in `__dwu`, which we clear here to avoid memory leaks. ::: browser/components/customizableui/PanelMultiView.jsm:731 (Diff revision 6) > - // Now that the subview is visible, we can check the height of the > - // description elements it contains. > + if (phase >= TRANSITION_PHASES.PREPARE) { > + this._transitioning = false; Well, that's because in `_transitionViews` we set `this._transitioning = true` when this phase is entered; the `_transitioning` setter also sets the attribute on the panelmultiview node and activated the appropriate styles. Therefore we need to clean this up as well.

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/20f6207757f3 Animate the panelviews differently to make it look as if the view to show is pushing the previous view out of the panel. r=jaws

Backed out for failing browser-chrome's browser/base/content/test/urlbar/browser_page_action_menu.js: https://hg.mozilla.org/integration/autoland/rev/24c7ddf09f52daff961ba4b21c2cb49f7bd06626 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=20f6207757f3f493b5613ac3e862850cff4fcb97&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=129619324&repo=autoland 09:50:48 INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | "Account Not Verified" == "Account Not Verified" - 09:50:48 INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | "toolbarseparator" == "toolbarseparator" - 09:50:48 INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | "-moz-box" == "-moz-box" - 09:50:48 INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | false == false - 09:50:48 INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | true == true - 09:50:48 INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | "Verify Your Account..." == "Verify Your Account..." - 09:50:48 INFO - Leaving test bound sendToDevice_syncNotReady_other_states 09:50:48 INFO - Entering test bound sendToDevice_syncNotReady_configured 09:50:48 INFO - Waiting for main page action button to have non-0 size 09:50:48 INFO - Buffered messages logged at 09:50:14 09:50:48 INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | true == true - 09:50:48 INFO - Buffered messages finished 09:50:48 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/urlbar/browser_page_action_menu.js | Test timed out -

This also causes reflow failures: https://treeherder.mozilla.org/logviewer.html#?job_id=129620031&repo=autoland [task 2017-09-08T17:25:18.885795Z] 17:25:18 INFO - Click appMenu-library-remotetabs-button [task 2017-09-08T17:25:18.888499Z] 17:25:18 INFO - Buffered messages finished [task 2017-09-08T17:25:18.889596Z] 17:25:18 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_appmenu_reflows.js | unexpected uninterruptible reflow [task 2017-09-08T17:25:18.890458Z] 17:25:18 INFO - [ [task 2017-09-08T17:25:18.891631Z] 17:25:18 INFO - "descriptionHeightWorkaround@resource:///modules/PanelMultiView.jsm:1277:20", [task 2017-09-08T17:25:18.892765Z] 17:25:18 INFO - "_transitionViews@resource:///modules/PanelMultiView.jsm:627:7", [task 2017-09-08T17:25:18.893845Z] 17:25:18 INFO - "async*showSubView/<@resource:///modules/PanelMultiView.jsm:537:15", [task 2017-09-08T17:25:18.894932Z] 17:25:18 INFO - "async*showSubView@resource:///modules/PanelMultiView.jsm:444:13", [task 2017-09-08T17:25:18.896061Z] 17:25:18 INFO - "value@resource:///modules/PanelMultiView.jsm:316:42", [task 2017-09-08T17:25:18.897145Z] 17:25:18 INFO - "showSubView@chrome://browser/content/customizableui/panelUI.js:386:7", [task 2017-09-08T17:25:18.898466Z] 17:25:18 INFO - "async*oncommand@chrome://browser/content/browser.xul:1:1", [task 2017-09-08T17:25:18.900474Z] 17:25:18 INFO - "openSubViewsRecursively@chrome://mochitests/content/browser/browser/base/content/test/performance/browser_appmenu_reflows.js:109:9", [task 2017-09-08T17:25:18.901459Z] 17:25:18 INFO - "async*openSubViewsRecursively@chrome://mochitests/content/browser/browser/base/content/test/performance/browser_appmenu_reflows.js:121:15", [task 2017-09-08T17:25:18.903727Z] 17:25:18 INFO - "async*@chrome://mochitests/content/browser/browser/base/content/test/performance/browser_appmenu_reflows.js:132:11", [task 2017-09-08T17:25:18.904637Z] 17:25:18 INFO - "async*withReflowObserver@chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:139:11", [task 2017-09-08T17:25:18.905651Z] 17:25:18 INFO - "async*@chrome://mochitests/content/browser/browser/base/content/test/performance/browser_appmenu_reflows.js:95:9", [task 2017-09-08T17:25:18.906734Z] 17:25:18 INFO - "Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:803:21", [task 2017-09-08T17:25:18.909228Z] 17:25:18 INFO - "TaskImpl_run@resource://gre/modules/Task.jsm:331:42", [task 2017-09-08T17:25:18.910017Z] 17:25:18 INFO - "TaskImpl@resource://gre/modules/Task.jsm:280:3", [task 2017-09-08T17:25:18.910822Z] 17:25:18 INFO - "asyncFunction@resource://gre/modules/Task.jsm:252:14", [task 2017-09-08T17:25:18.911859Z] 17:25:18 INFO - "Task_spawn@resource://gre/modules/Task.jsm:166:12", [task 2017-09-08T17:25:18.912951Z] 17:25:18 INFO - "Tester_execTest@chrome://mochikit/content/browser-test.js:794:9", [task 2017-09-08T17:25:18.915108Z] 17:25:18 INFO - "Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:694:7", [task 2017-09-08T17:25:18.916096Z] 17:25:18 INFO - "SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59", [task 2017-09-08T17:25:18.918495Z] 17:25:18 INFO - "" [task 2017-09-08T17:25:18.919320Z] 17:25:18 INFO - ]

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f09b2b387751 Animate the panelviews differently to make it look as if the view to show is pushing the previous view out of the panel. r=jaws

Backed out for frequently failing browser-chrome's browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize.js on macOS: https://hg.mozilla.org/integration/autoland/rev/307b5682dec342ab187af9e13ee006c36bd09da8 Push with failure (only opt here, for later pushes opt + debug): https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f09b2b387751bb86a34b5cfb2bd1eacd31ab1fad&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=130089658&repo=autoland 08:00:42 INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize.js | Document has the expected compat mode - 08:00:42 INFO - Increase body children's width. Expect them to wrap, and the frame to grow vertically rather than widen. 08:00:42 INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize.js | Browser height should increase (454 > 176) - 08:00:42 INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize.js | Window width should not change - 08:00:42 INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize.js | Window height should increase (454 >= 176) - 08:00:42 INFO - Buffered messages finished 08:00:42 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize.js | Document should not be vertically scrollable - 15 <= 1 - JS frame :: chrome://mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize.js :: testPopupSize :: line 225 08:00:42 INFO - Stack trace: 08:00:42 INFO - chrome://mochitests/content/browser/browse

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5b700335fc30 Animate the panelviews differently to make it look as if the view to show is pushing the previous view out of the panel. r=jaws

I have reproduce this issue using an affected Nightly build from 2017-06-20. Verified fixed on 57.0b7 (20171009192146) across platforms: Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.