1441154 - Start the PanelMultiview sliding transition together with the height transition

Status: RESOLVED FIXED

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

Description

[:Paolo Amadini]

This was an existing issue, likely surfaced by bug 1434376.

Comment 1

[:Paolo Amadini]

Attachment: Bug 1441154 - Start the PanelMultiview sliding transition together with the height transition.

This fixes an issue where the two transition are not simultaneous.

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

Comment 2

[:Gijs (he/him)]

Comment on attachment 8953995 [details]
Bug 1441154 - Start the PanelMultiview sliding transition together with the height transition.

https://reviewboard.mozilla.org/r/223166/#review229096

Comment 3

[:Gijs (he/him)]

Though looking at this in more detail, why are we waiting for a flush at all? (Can still land this because it fixes an immediate regression, but curious what the reasoning is.)

Comment 4

[:Paolo Amadini]

It provides the starting point for the transition, otherwise the layout code doesn't know what the initial values should be. I can easily add a comment to that effect.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f2192eb120
Start the PanelMultiView sliding transition together with the height transition. r=Gijs

Comment 6

[Hiroyuki Ikezoe (:hiro)]

FWIW, we can use getComputedStyle(this._viewStack).transform instead.

Comment 7

[:Paolo Amadini]

This is what we used to do before Photon, but now promiseDocumentFlushed exists so we can avoid the synchronous reflow that would be caused by getComputedStyle. See <https://developer.mozilla.org/en-US/Firefox/Performance_best_practices_for_Firefox_fe_engineers#Detecting_and_avoiding_synchronous_reflow>.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

As far as I can tell getComputedStyle flushes *styles* not layout.  Also getComputedStyle() has been optimized since bug 1363805.

Comment 9

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/d4f2192eb120

Comment 10

[:Gijs (he/him)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> As far as I can tell getComputedStyle flushes *styles* not layout.  Also
> getComputedStyle() has been optimized since bug 1363805.

Sure, but in the context where this is happening we have literally just dirtied style by setting node.style.foo, so it would force a sync style flush at that point, and then we dirty style some more and another style flush will happen after that... it doesn't seem clever to force a sync style flush in that case (and even worse if it does cause a layout flush).