From https://bugzilla.mozilla.org/show_bug.cgi?id=1296070#c9, Zooming in further, I can see a Style flush (#66) that's caused be "adjustArrowPositioning" in popup.xml:400, which is in response to a onpopupshowing event that's handled in popup.xml:468. I believe that's causing us to skip a frame here.

The layout flush from adjustArrowPosition isn't avoidable. We need it to get the alignmentOffset correct for placing the arrow in the right position along the edge of the panel. Instead what I've done is try to coalesce all code that relies on flushing the layout to run consecutively so-as to only require one layout flush instead of multiple. I don't see any visible regressions from this and there is now only one Style flush in my SPS profiler log.

Hey paolo, With the work that you're doing in bug 1009116, will this API still be needed to avoid multiple flushes?

Comment on attachment 8784164 [details] [diff] [review] Patch Cancelling review until I've got more info from paolo regarding other movement going on in the panelmultiview binding in bug 1009116.

The _syncContainerWithMainView function might go away, so let's definitely wait and see if we can find a solution in bug 1009116 first.

Comment on attachment 8872504 [details] Bug 1296442 - Avoid sync layout and style flush when positioning the arrow for arrow panels. https://reviewboard.mozilla.org/r/144048/#review147828 ::: toolkit/content/widgets/popup.xml:451 (Diff revision 1) > - var offset = this.alignmentOffset; > + if (this._destroyed) { > + return; > + } > + > + let position = this.alignmentPosition; > + let offset = this.alignmentOffset; These don't flush layout, do they? So... ::: toolkit/content/widgets/popup.xml:453 (Diff revision 1) > + } > + > + let position = this.alignmentPosition; > + let offset = this.alignmentOffset; > + // The assigned side stays the same regardless of direction. > + let isRTL = window.getComputedStyle(this).direction == "rtl"; How about we cache the direction just like we do in arrowscrollboxes? I don't think we need to support dynamic changes here.

Comment on attachment 8872504 [details] Bug 1296442 - Avoid sync layout and style flush when positioning the arrow for arrow panels. https://reviewboard.mozilla.org/r/144048/#review147838 (In reply to Dão Gottwald [::dao] from comment #6) > Comment on attachment 8872504 [details] > Bug 1296442 - Avoid sync layout and style flush when positioning the arrow > for arrow panels. > > https://reviewboard.mozilla.org/r/144048/#review147828 > > ::: toolkit/content/widgets/popup.xml:451 > (Diff revision 1) > > - var offset = this.alignmentOffset; > > + if (this._destroyed) { > > + return; > > + } > > + > > + let position = this.alignmentPosition; > > + let offset = this.alignmentOffset; > > These don't flush layout, do they? So... Sorry, I hadn't read comment 0. Turns out they do: http://searchfox.org/mozilla-central/rev/31eec6f59eb303adf3318058cb30bd6a883115a5/layout/xul/PopupBoxObject.cpp#301 I wonder if there are ways to avoid this. Enn might know. He's probably a better reviewer here too. For instance, I don't know if adjustArrowPosition is expected to do things synchronously, and whether doing this asynchronously from popupshowing (and painting a frame in the meantime) could race with other things, e.g. popupshown. I imagine this could even regress popup opening performance from the user's perspective.

(In reply to Dão Gottwald [::dao] from comment #7) > I wonder if there are ways to avoid this. Enn might know. He's probably a > better reviewer here too. Unfortunately, I don't think he'll be in touch until early August. The comment above this block: http://searchfox.org/mozilla-central/rev/31eec6f59eb303adf3318058cb30bd6a883115a5/layout/xul/PopupBoxObject.cpp#302 seems to suggest that a full layout flush is required, but nothing in the bug that landed that code seems to say why. However, flipping that GetFrame(true) to GetFrame(false) (which only flushes frames, and not layout), results in test failures in (at least) toolkit/content/tests/widgets/test_popupanchor.xul. It looks like there are cases here where the arrow will be positioned poorly unless we do a full layout flush. That having been said, my original patch fails even more of the tests in test_popupanchor.xul. Hey markh, I know it's been a long time since you worked on this stuff in bug 824963 with Enn, but do you know if there's any good way to avoid the flush here?

(In reply to Mike Conley (:mconley) from comment #8) > Hey markh, I know it's been a long time since you worked on this stuff in > bug 824963 with Enn, but do you know if there's any good way to avoid the > flush here? I don't, sorry :(

At a very high level, I _suspect_ the panel needs to know the direction where it will open before actually painting anything, for example whether a vertical panel would open towards the top or the bottom of the screen, and to determine that it has to layout its contents to know its height. The chosen direction is then directly reflected by the property. That, or it just needs to build building the frame object associated with the panel, that holds the window widget. I just know it doesn't even exist if the panel element has the "hidden" attribute. Unfortunately, I don't know how this information can be of any practical significance here...

(In reply to :Paolo Amadini from comment #10) > At a very high level, I _suspect_ the panel needs to know the direction > where it will open before actually painting anything That makes sense to me - but I figure it can probably get all of that information after the next "natural" style and layout flush, before opening and showing the popup. Hey bytesized, I see you've been bravely poking about in PopupBoxObject.cpp lately... do you have any opinions on this problem?

It's been a little while since I have done anything with panels. I have spent some time trying to re-familiarize myself, but I don't think I have really touched this particular area and I am afraid that my opinion on this subject may be fairly limited. Batching the style flush with another style flush sounds like a good idea, but I have a concern. After a popup goes through the "showing" phase, it goes through a "positioning" phase. As I understand it, the act of laying out the panel sometimes reveals that the position is not right (See Bug 1206133 and the bugs it blocks for more details on this). Based on these handlers [1][2], it looks like this style flush is (currently) expected to happen twice, once during the showing phase and once during the positioning phase. It seems like one of these flushes could be combined with another flush, but I do not know if both of them could be. To be honest though, I know next to nothing about style flushes, so I am just not sure what is possible and what isn't. [1] http://searchfox.org/mozilla-central/rev/972b7a5149bbb2eaab48aafa87678c33d5f2f045/toolkit/content/widgets/popup.xml#475,482 [2] http://searchfox.org/mozilla-central/rev/972b7a5149bbb2eaab48aafa87678c33d5f2f045/toolkit/content/widgets/popup.xml#523-525

The popuppositioned event name implies any positioning has *already happened*. So I was surprised that the alignmentPosition getter causes flushes in this handler. In practice it does seem many of the things you might want to do at this phase either cause uninterruptible reflow, or must take place over more than one tick - at the risk of overlapping with popupshown. In the specific case of adjustArrowPosition, maybe we could design around this by deferring showing of the panel's pointy arrow until popupshown. It could potentially transition in to avoid looking too flickery.