Closed Bug 1441154 Opened 2 years ago Closed 2 years ago

Start the PanelMultiview sliding transition together with the height transition

Categories

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

59 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file)

This was an existing issue, likely surfaced by bug 1434376.
Comment on attachment 8953995 [details]
Bug 1441154 - Start the PanelMultiview sliding transition together with the height transition.

https://reviewboard.mozilla.org/r/223166/#review229096
Attachment #8953995 - Flags: review?(gijskruitbosch+bugs) → review+
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.)
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.
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f2192eb120
Start the PanelMultiView sliding transition together with the height transition. r=Gijs
FWIW, we can use getComputedStyle(this._viewStack).transform instead.
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>.
As far as I can tell getComputedStyle flushes *styles* not layout.  Also getComputedStyle() has been optimized since bug 1363805.
https://hg.mozilla.org/mozilla-central/rev/d4f2192eb120
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
(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).
You need to log in before you can comment on or make changes to this bug.