1703405 - browser/base/content/test/performance/browser_appmenu.js fails under proton

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

[task 2021-04-06T21:15:14.972Z] 21:15:14     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_appmenu.js | unexpected reflow at collectItems@resource:///modules/PanelMultiView.jsm hit 1 times
[task 2021-04-06T21:15:14.972Z] 21:15:14     INFO - Stack:
[task 2021-04-06T21:15:14.972Z] 21:15:14     INFO -   collectItems@resource:///modules/PanelMultiView.jsm:1509:30
[task 2021-04-06T21:15:14.973Z] 21:15:14     INFO -   descriptionHeightWorkaround@resource:///modules/PanelMultiView.jsm:1527:7
[task 2021-04-06T21:15:14.973Z] 21:15:14     INFO -   handleEvent@resource:///modules/PanelMultiView.jsm:1258:23

I'm 99% sure this is because we added wrapping to the sync item in the app menu in bug 1698062. Wrapping items cause us to hit descriptionheightworkaround code, which causes sync layout flushes.

I'm not sure what we want to do here. Mike/Emma, thoughts? We can add an exception but that is kind of sad because adding sync layout flushes is bad...

Comment 1

[Emma Malysz]

Hm, I initially thought the cleanup work in Bug 1703417 and avoiding this call https://searchfox.org/mozilla-central/rev/ee9dab6aa95f167a34cb178960f7375210a0bba4/browser/base/content/browser-sync.js#514-516 may fix it, but we're still hitting a sync layout flush.

Is there a preferred way to wrap items? afaik the app menu has previously never dealt with this issue before and all items were kept to the same line

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I suggest we add it to the allowlist for now, and file a cleanup bug to remove the flush.

Comment 3

[:Gijs (he/him)]

(In reply to Emma Malysz from comment #1)

Is there a preferred way to wrap items? afaik the app menu has previously never dealt with this issue before and all items were kept to the same line

I am afraid the second part here is correct. As for a preferred way, there are two other things that may be worth trying:

1. if we know the width available for the text beforehand (which I think we do, because the width of the panel is fixed?), ensure it is set directly on the element to ensure the XUL code for the preferred height of the element gets it right first time.
2. switch the element in question to an HTML equivalent that wraps (possibly combined with (1)) and ensure we don't hit it in the descriptionHeightWorkAround code.
3. switch everything in the main view to non-XUL flexbox and see if that helps.

But...

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos) from comment #2)

I suggest we add it to the allowlist for now, and file a cleanup bug to remove the flush.

I think this is probably the realistic option here given timelines.

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1703405 - add expected reflow for hamburger menu opening under proton, r?mconley,emalysz

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1698062

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc8b64f3831b
add expected reflow for hamburger menu opening under proton, r=mconley

Comment 7

[Atila Butkovits]

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