1796735 - The History Menu in toolbar flickers when opening the "Recently closed tabs/windows"

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[tgn-ff]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:108.0) Gecko/20100101 Firefox/108.0

Steps to reproduce:

1. Put the History button to the toolbar using the "Customise toolbar" menu.
2. Open a few sites in separate tabs, then close them.
3. Click on the History icon, then on "Recently closed tabs".

Actual results:

The menu flickers.

Expected results:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4ff88d45f59c4357dcc71ddf4ad5344a38f9944d&tochange=aa325161aae5e555cab4d44b6662bbe029d26a5d

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Firefox::Menus' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

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

:emilio, since you are the author of the regressor, bug 1790616, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I only see the flickering iff there's a long title, do you see the same? Otherwise, could you post a screencast of the issue so I'm on the same page? Thank you!

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: What Emilio sees if there's a long title (amazon's in this case)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1796735 - Remove useless flex attributes from panelviews. r=mconley,Gijs,dao

These are probably copy-pasta. All panelviews flex because of:

https://searchfox.org/mozilla-central/rev/c5c002f81f08a73e04868e0c2bf0eb113f200b03/browser/themes/shared/customizableui/panelUI-shared.css#135

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1796735 - Don't flex panelviews. r=mconley,Gijs,dao

With modern flexbox, this can shrink panels down to their minimum size while
transitioning (where you have two panelview siblings).

This can cause apparent jumping like in the screencast in the bug. I don't
think we need (or want) to flex. This comes to the initial commit from
bug 1009116.

Depends on D159933

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1796735 - Don't use width/height attributes for panel sizing. r=Gijs,dao,mconley

While these work now because I haven't ported popups to modern flexbox,
let's remove usage of these attributes so I don't have to later. This
doesn't change behavior.

Depends on D159934

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Flickering I see on release due to subpixel positioning changes.

Comment 9

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

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1796735 - Use getBoundsWithoutFlushing rather than getOuterScreenRect to lock the size of the panel. r=mconley,Gijs,dao

getOuterScreenRect doesn't flush layout, so not flushing here should be
fine.

We only care about the CSS width / height, not about the screen
positioning, so this is more accurate.

The flickering in comment 8 (the hover effect in the "restore previous
session" button) happens because the following set of events:

• We try to lock the size of the panel.
• The size returned by getOuterScreenRect() is fractionally different
  to the actual size.
• We get to 1, realize we've changed size (even though fractionally),
  set the origin at 0, 0, and then call SetPopupPosition to fix that
  up.
• But we've disabled autopositioning1, so the layout position of the
  panel remains at 0, 0.

This prevents the flickering by using the actual layout size
(getBoundsWithoutFlushing / getBoundingClientRect) rather than the
potentially-rounded getOuterScreenRect, to prevent the resize.

We could fix autoPosition = false, but I don't think it's needed
anymore, so I'm going to try removing it instead. However this change
should still avoid work, so seems worth landing regardless.

Depends on D159935

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1796735 - Remove XULPopupElement.autoPosition. r=Gijs,mconley,dao

This was used to prevent reflows due to popuppositioned events during
view transitions.

The previous patch should've prevented the popuppositioned events to
begin with, plus we no longer use arrows that need positioning etc,
which means we shouldn't be triggering the reflows anyways.

Since this is the only consumer of autoPosition = true/false, we can
remove the code supporting it. It's a bit bogus as per the commit
message of the previous patch and, while fixable, it doesn't seem worth
fixing if we can just get rid of it.

Depends on D159936

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d15b8a3c46e
Remove useless flex attributes from panelviews. r=dao

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/208d0c8c9ed3
Don't flex panelviews. r=dao

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82fec042c530
Don't use width/height attributes for panel sizing. r=dao

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3108a6f4b757
Use getBoundsWithoutFlushing rather than getOuterScreenRect to lock the size of the panel. r=dao

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa1994029eeb
Remove XULPopupElement.autoPosition. r=dao

Comment 17

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/7d15b8a3c46e
https://hg.mozilla.org/mozilla-central/rev/208d0c8c9ed3
https://hg.mozilla.org/mozilla-central/rev/82fec042c530
https://hg.mozilla.org/mozilla-central/rev/3108a6f4b757
https://hg.mozilla.org/mozilla-central/rev/aa1994029eeb

Comment 18

[tgn-ff]

Attachment: history_flickering.webm

Not sure if you still need that info, but long titles are not necessary for me to reproduce it. I'll, appropriately, reopen or verify the bug after the patches land.

Comment 19

[Emil Ghitta, QA [:emilghitta]]

I've reproduced this issue using Firefox 108.0a1 (BuildId:20221020215126) on Windows 10 64bit

This issue is verified fixed using Firefox 108.0b3 (BuildId:20221117185908) on Windows 10 64bit, macOS 11 & Ubuntu 22.04