1382243 - Reduce nesting and ensure proper cleanup of nodes used in photon panelmultiview transition code

Status: RESOLVED WONTFIX

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[:Gijs (he/him)]

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 1

[:Gijs (he/him)]

Attachment: Bug 1382243 - Refactor panelmultiview code to reduce complexity and length of functions,

While this creates several methods with just 1 callsite, given the previous length of the
showSubView method (230 lines), it seems wise to split this up and to try to reduce the
nesting, as well as making it easier to reason about what happens if we hide the panel
mid-transition.

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

Comment 2

[Mike de Boer [:mikedeboer]]

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?

Comment 3

[:Gijs (he/him)]

(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 4

[Mike de Boer [:mikedeboer]]

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 5

[Mike de Boer [:mikedeboer]]

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()'?

Comment 6

[:Gijs (he/him)]

Comment on attachment 8894961 [details]
Bug 1382243 - Refactor panelmultiview code to reduce complexity and length of functions,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166076/diff/1-2/

Comment 7

[:Gijs (he/him)]

This makes some identity popup tests unhappy. :-\

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3995ce876553&selectedJob=122635663

Comment 8

[:Gijs (he/him)]

Attachment: Bug 1382243 - make all the identity tests async,

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

Comment 9

[:Gijs (he/him)]

Comment on attachment 8894961 [details]
Bug 1382243 - Refactor panelmultiview code to reduce complexity and length of functions,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166076/diff/2-3/

Comment 10

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b6a13810fe7

Comment 11

[Johann Hofmann [:johannh]]

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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Backout patch

Comment 15

[Mike de Boer [:mikedeboer]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d73fee7d113f20918f10e9b07cca40379597aebb

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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