883136 - popup.alignmentOffset flushes and other comments from bug 812943

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

Although I didn't test directly, the changes from bug 812943 (popup slide the arrow feature) looks to add extra layout flushes. This can cause performance problems.

More specifically:

402         arrowbox.style.removeProperty("margin");
 This added line can change layout.

404         var position = this.alignmentPosition;

  The layout change (plus others) above can cause a flush. I recall however, that a flush is needed here when I added this, but the margin change should be moved after this line.

405         if (position.indexOf("start_") == 0 || position.indexOf("end_") == 0) {
406           container.orient = "";
407           arrowbox.orient = "vertical";

Both of these lines change layout.

408           if (position.indexOf("_after") > 0) {
409             arrowbox.pack = "end";
410             arrowbox.style.marginBottom = this.alignmentOffset + "px";

Retrieving the alignmentOffset causes another flush. This is because nsPopupBoxObject::GetAlignmentOffset calls GetFrame with 'true'.

Ideally 'false' should be passed instead. You might as well consider moving the retrieval of the alignmentOffset to just after the alignmentPosition.


+    bool slideHorizontal = mSlide && mPosition <= POPUPPOSITION_AFTEREND;
+    bool slideVertical = mSlide && mPosition >= POPUPPOSITION_STARTBEFORE;

This doesn't handle POPUPPOSITION_OVERLAP or POPUPPOSITION_AFTERPOINTER.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Test which uses reflow observer to count panel reflows

This patch adds a test which counts the reflows caused by a simple openPopup.  If only this patch is applied, the test fails as it sees 2 reflows instead of 1 - but the test passes when the following patch is applied.

This introduces a new test file as it might make sense to enhance the test later to cover additional scenarios (eg, to test that moving an open popup doesn't cause unnecessary reflows), but the current single test should meet the requirements of this bug...

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: Make the requested changes

This patch make the suggested changes in comment 0, and causes the previously attached test to pass (ie, causes a simple openPopup to cause 1 reflow rather than the 2 it currently does)

Comment 3

[Neil Deakin]

Comment on attachment 768356 [details] [diff] [review]
Make the requested changes

>     bool slideHorizontal = mSlide && mPosition <= POPUPPOSITION_AFTEREND;
>-    bool slideVertical = mSlide && mPosition >= POPUPPOSITION_STARTBEFORE;
>+    bool slideVertical = mSlide && mPosition >= POPUPPOSITION_STARTBEFORE
>+                         && mPosition <= POPUPPOSITION_ENDAFTER;

I'd rather we checked only the valid values for slideHorizontal as well, in case mPosition is POPUPPOSITION_UNKNOWN.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85ad86ba3208
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a26a0cb1be0

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/85ad86ba3208
https://hg.mozilla.org/mozilla-central/rev/6a26a0cb1be0