I noticed in bug 1354086 that we don't reset certain styles we set on various panelview and container elements when the transition is aborted or never happens. This and making sure that code isn't as deeply nested (and potentially split up into separate methods) will make it easier to adjust the transition in bug 1374749 without breaking more things.

Comment on attachment 8894961 [details] Bug 1382243 - Refactor panelmultiview code to reduce complexity and length of functions, https://reviewboard.mozilla.org/r/166076/#review171678 ::: browser/components/customizableui/PanelMultiView.jsm:617 (Diff revision 1) > - // to transition, so we make sure to do all the work on the transform > + // to transition, so we make sure to do all the work on the transform > - // transition-end, because that is guaranteed to happen. > + // transition-end, because that is guaranteed to happen. > - if (ev.target != nodeToAnimate || ev.propertyName != "transform") > - return; > - > - this._viewContainer.removeEventListener("transitionend", this._transitionEndListener); > + return ev.target != this._viewStack || ev.propertyName != "transform"; > + } > + let didTransition = await this._promiseEventOnceOrHidden(this._viewContainer, "transitionend", > + transitionChecker); What I don't really like about using a promisified solution is that - while it simplifies the code in question quite nicely due to composable pattern - it also promises to resolve after another tick of the event loop, which has been a source of frustration to me before when working on code that requires absolutely fluent transition animations. At least, this is why I did this implementation using callbacks and not promises. What do you think? Is adding the promises control flow worth it? Is there a way you can do this refactor using callbacks for the 'transistionend' and 'MozAfterPaint' cases, instead of promises?

(In reply to Mike de Boer [:mikedeboer] from comment #2) > Comment on attachment 8894961 [details] > Bug 1382243 - Refactor panelmultiview code to reduce complexity and length > of functions, > > https://reviewboard.mozilla.org/r/166076/#review171678 > > ::: browser/components/customizableui/PanelMultiView.jsm:617 > (Diff revision 1) > > - // to transition, so we make sure to do all the work on the transform > > + // to transition, so we make sure to do all the work on the transform > > - // transition-end, because that is guaranteed to happen. > > + // transition-end, because that is guaranteed to happen. > > - if (ev.target != nodeToAnimate || ev.propertyName != "transform") > > - return; > > - > > - this._viewContainer.removeEventListener("transitionend", this._transitionEndListener); > > + return ev.target != this._viewStack || ev.propertyName != "transform"; > > + } > > + let didTransition = await this._promiseEventOnceOrHidden(this._viewContainer, "transitionend", > > + transitionChecker); > > What I don't really like about using a promisified solution is that - while > it simplifies the code in question quite nicely due to composable pattern - > it also promises to resolve after another tick of the event loop, which has > been a source of frustration to me before when working on code that requires > absolutely fluent transition animations. Well, the events mean that there's at least 1 tick even without promises, right, and maybe more than 1 (certainly for transitionend)? Note also that my understanding of promises is that they don't tick the event loop, but that they run after all the other JS on the stack has completed, once resolve() (or reject()) is called. Specifically, if I read https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model, the promise resolution code is guaranteed to be called after the transitionend event fires and before we render anything (at least, up until the next time we wait for a promise resolution, of course!). So, I don't think this should be a concern here. > At least, this is why I did this implementation using callbacks and not > promises. > What do you think? Is adding the promises control flow worth it? Is there a > way you can do this refactor using callbacks for the 'transistionend' and > 'MozAfterPaint' cases, instead of promises? Not without callback hell, because you need to pass along all the state to the callback somehow. I think the control flow is worth it because it makes the code more linear and easier to reason about what things happen in what order (and when we should revert some aspects of it). What do you think?

Comment on attachment 8894961 [details] Bug 1382243 - Refactor panelmultiview code to reduce complexity and length of functions, (In reply to :Gijs from comment #3) > Not without callback hell, because you need to pass along all the state to > the callback somehow. I think the control flow is worth it because it makes > the code more linear and easier to reason about what things happen in what > order (and when we should revert some aspects of it). > > What do you think? Even though I think 'callback hell' is overstating it a little, I do think that the change to using a promise-based control flow make the code more readable. I'll review the patch as-is soon.

Comment on attachment 8894961 [details] Bug 1382243 - Refactor panelmultiview code to reduce complexity and length of functions, https://reviewboard.mozilla.org/r/166076/#review172298 Let's go with this! General comment: please add proper JSDocs before the new methods or convert existing/ moved ones. Thanks! ::: browser/components/customizableui/PanelMultiView.jsm:453 (Diff revision 1) > - this._placeSubView(viewNode); > + this._placeSubView(viewNode); > - } > + } > + return viewNode; > + } > > - let reverse = !!aPreviousView; > + _getPreviousRectAndPanelBounds(viewNode, previousViewNode) { nit: perhaps '_cachePreviousRectAndConstrainBounds()'?

This makes some identity popup tests unhappy. :-\ https://treeherder.mozilla.org/#/jobs?repo=try&revision=3995ce876553&selectedJob=122635663

Comment on attachment 8897014 [details] Bug 1382243 - make all the identity tests async, https://reviewboard.mozilla.org/r/168298/#review173628 Looks good. We should clean up those tests some day.

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a528b4400362 Refactor panelmultiview code to reduce complexity and length of functions, r=mikedeboer https://hg.mozilla.org/integration/autoland/rev/ab84efbc7489 make all the identity tests async, r=johannh

https://hg.mozilla.org/mozilla-central/rev/a528b4400362 https://hg.mozilla.org/mozilla-central/rev/ab84efbc7489

Requesting checkin of the Backout patch as linked in comment 14 - this should apply cleanly on m-c and autoland. Backing this patch out because it causes animation glitches. The refactor will be re-applied in bug 1374749 to the extent that it'll not re-introduce these glitches.

Merged backout: https://hg.mozilla.org/mozilla-central/rev/147023f5aad0