1750487 - When about:preferences has page zoom other than 100%, search engine dropdown is unusable

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Asif Youssuff]

Steps to reproduce:

1. Navigate to about:preferences#search
2. Increase zoom level to 120%
3. Click the Default Search engine dropdown

What happens:

The popup doesn't appear, and it is impossible to visually select a different search engine. In some of the builds since this regression, the popup also flickers like described in bug 1726939. It has since gotten worse, however.

11:14.23 INFO: No more integration revisions, bisection finished.
11:14.23 INFO: Last good revision: 9f0fbb1431721c9eae68a3c94ae49a4d33fdb1f8
11:14.23 INFO: First bad revision: 7b82d177a6b979036f180329be6b029d690d9e0c
11:14.23 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9f0fbb1431721c9eae68a3c94ae49a4d33fdb1f8&tochange=7b82d177a6b979036f180329be6b029d690d9e0c

Comment 1

[Asif Youssuff]

Attachment: about:support

Comment 2

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

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

This should be easy-ish to figure out, I'd think.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1750487 - Clean up menu popup rect handling. r=stransky

Store preferred rect in dev pixels, which simplifies a bit other calculations,
and ensures that the nsMenuPopupFrame code accounts for zoom etc.

Use existing conversion methods for GDK <-> Device <-> CSS conversions.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1750487 - Don't update popup hierarchy if position and size didn't change in nsWindow::NativeMoveResizeWaylandPopup. r=stransky

Depends on D139623

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1750487 - Fix early-out in nsMenuPopupFrame::MoveTo to account for margins. r=stransky

This was also broken.

Depends on D139624

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1750487 - Fix weird full-zoom handling for unanchored popups in nsMenuPopupFrame positioning. r=mstange

I don't see why ever mScreenRect would deal with zoom-less coordinates.
I suspect this might have been accounting for screenX/Y not dealing
with zoom in the past, but that was fixed (not long ago, actually) in
bug 1753836.

Also, browser.xhtml never has zoom. I haven't seen this causing problems
in practice in non-Wayland platforms, but I think it's the right thing
to do and the code as is is extremely confusing, since other places
applying screen rects and context menu offsets treat them as real CSS
pixels:

https://searchfox.org/mozilla-central/rev/73a8000b0c0eb527faef01ea17c6d2398622a386/layout/xul/nsMenuPopupFrame.cpp#2407-2411

So I don't think this changes behavior in practice in non-wayland
platforms given we never have full zoom in browser.xhtml and we mostly
hit this code path for context menus, but given the above I think it's
simpler and more correct.

Depends on D139670

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2eceb0d288a
Fix weird full-zoom handling for unanchored popups in nsMenuPopupFrame positioning. r=mstange

Comment 10

[Phabricator Automation]

Comment on attachment 9265383 [details]
Bug 1750487 - Clean up menu popup rect handling. r=stransky

Revision D139623 was moved to bug 1757106. Setting attachment 9265383 [details] to obsolete.

Comment 11

[Phabricator Automation]

Comment on attachment 9265384 [details]
Bug 1750487 - Don't update popup hierarchy if position and size didn't change in nsWindow::NativeMoveResizeWaylandPopup. r=stransky

Revision D139624 was moved to bug 1757106. Setting attachment 9265384 [details] to obsolete.

Comment 12

[Phabricator Automation]

Comment on attachment 9265426 [details]
Bug 1750487 - Fix early-out in nsMenuPopupFrame::MoveTo to account for margins. r=stransky

Revision D139670 was moved to bug 1757106. Setting attachment 9265426 [details] to obsolete.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09583277ae5e
Fix test_popup_scaled to account for intentional behavior change.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2543c933025b
Avoid unneeded zoom mess-up calculation now that screen coordinates are correct. rpending=nchevobbe

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(In reply to Pulsebot from comment #14)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2543c933025b
Avoid unneeded zoom mess-up calculation now that screen coordinates are
correct. rpending=nchevobbe

Nicolas, I had to push this to avoid backout because there was a devtools test failure (see commit message).

I decided to push it because I'm fairly sure it's the right fix (plus it's simpler which is always nice). Assuming this sticks, can you take a quick look?

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4540f58e953c
Tentatively make test_popup_scaled's last test work on Windows on automation. CLOSED TREE

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #15)

(In reply to Pulsebot from comment #14)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2543c933025b
Avoid unneeded zoom mess-up calculation now that screen coordinates are
correct. rpending=nchevobbe

Nicolas, I had to push this to avoid backout because there was a devtools test failure (see commit message).

I decided to push it because I'm fairly sure it's the right fix (plus it's simpler which is always nice). Assuming this sticks, can you take a quick look?

Thanks Emilio, the change makes sense to me

Comment 18

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/a2eceb0d288a
https://hg.mozilla.org/mozilla-central/rev/09583277ae5e
https://hg.mozilla.org/mozilla-central/rev/2543c933025b
https://hg.mozilla.org/mozilla-central/rev/4540f58e953c

Comment 19

[Emilio Cobos Álvarez (:emilio)]

For the record, this made the menus usable, but bug 1757106 will actually fix their position in presence of zoom.

Comment 20

[Ardelean Oana, Desktop QA [:oardelean]]

I tried to reproduce this issue on Firefox 98, on Linux x86_64(Ubuntu 20.04) with no success. Are there any additional steps/configuration details i should follow?
If not, would you be kind to verify this fix on the latest beta? https://archive.mozilla.org/pub/firefox/candidates/99.0b4-candidates/build1/win64/en-US/
Thank you.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

You need MOZ_ENABLE_WAYLAND=1, probably.

Comment 22

[Ardelean Oana, Desktop QA [:oardelean]]

Reproducible with Firefox 98.0(build ID: 20220304153049) on Ubuntu 22(Wayland Session). Verified as fixed on Firefox 99.0(build ID: 20220330194208), Firefox 107.0b4(build ID: 20221023190001) and Nightly 108.0a1(build ID: 20221024212806) on Ubuntu 22.