1926624 - Primary password dialog has the wrong size on macOS

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Simon Friedberger (:simonf)]

My Nightly on MacOs is displaying the primary password dialog at what seems to be minimum window size. It cannot be resized. This has been happening for maybe a week.

Comment 1

[Simon Friedberger (:simonf)]

Attachment: screenshot.png

Comment 2

[Simon Friedberger (:simonf)]

Attachment: smallfont

Look like this is related to high-dpi issues. This is what happens when I drag the window to my other screen. The contents now fit in the window but they're too small.

Comment 3

[Simon Friedberger (:simonf)]

:emilio, after about 172 tries the first plausible patch that mozregression has led me to is https://bugzilla.mozilla.org/show_bug.cgi?id=1924240 could you take a look, please?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

[Tracking Requested - why for this release]: Reproducible regression, it seems, would be good to get to the bottom of it.

Sure, that sounds not-totally-unlikely. At a glance I can't repro on a single monitor, and I won't have access to multiple monitors until monday at best. Just to confirm, your setup is:

• macOS (inferred from screenshots)
• External monitor, with different DPI to native display (inferred from comment 2).

Right?

Also, which push did mozregression point to? I would kinda expect https://hg.mozilla.org/mozilla-central/rev/3613fc672c50, but maybe it's the later ones? (I landed that bug in separate pushes to ease bisection among other things).

Thanks!

Comment 5

[Simon Friedberger (:simonf)]

:emilio, here you go: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9f82cc33da0b3c376fdb2d5192bc4ff9b3167076&tochange=3613fc672c501bd6280abfe58e1f53e72f2cd5de

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: WIP: Bug 1926624 - Handle hidden parent backing scale.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Can't test macOS until the evening, but if you can test comment 5 it'd be great.

Comment 8

[Simon Friedberger (:simonf)]

This doesn't seem to fix it.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Ok, I can reproduce the issue. Reduced STR:

• Go to about:preferences.
• Open DevTools (Cmd+Opt+I)
• Paste Services.prompt.alert(window, "Some title", "some message")

This is only an issue if I'm on the low-res monitor, not the high-res (laptop).

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1926624 - Unify top level widget behavior across platforms, standardizing on previous macOS behavior. r=#mac-reviewers!,#win-reviewers!,stransky!

So the issue here is that my patch made GetTopLevelWidget() return the
top level window across the widget chain, including this one in
particular:

https://searchfox.org/mozilla-central/rev/387f3edbef37d31b2e91fb0812c74b54729e86ff/layout/base/nsDocumentViewer.cpp#2339

We have two AppWindows parented:

• Top level browser -> Alert window.

It seems it's know that cocoa windows that aren't shown yet may have a
HiDPI scale factor different from their actual screen's scale factor:

• https://searchfox.org/mozilla-central/rev/387f3edbef37d31b2e91fb0812c74b54729e86ff/widget/cocoa/nsCocoaWindow.mm#1895-1935

This causes the alert window have a devicePixelRatio of 2 but the top
level have a devicePixelRatio of 1.

Which means that nsDeviceContext thinks we have a DPI of 1, but
AppWindow and co have a DPI of 2, causing this mismatch.

Fix this by making GetTopLevelWidget actually return the nsCocoaWindow
widget for the alert, not the other.

This restores the behavior for cocoa, but changes it for Windows and
Linux (Android can't have nested top level windows, afaict).

Given windows and Linux both had the concept of "top level widget" in a
similar fashion, I think we can try this. I'll add a smaller fix for
uplift purposes.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1926624 - Simplify cocoa backing scale factor changes. r=#mac-reviewers!

No intended behavior change. BackingScaleFactorChanged bails out if it didn't
in fact change. Make it check BackingScaleFactor() to account for the lazy
initialization.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1926624 - Remove DEBUG-only test-only hidpi code. r=#mac-reviewers!

It's not all that useful, and it's rather confusing.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1926624 - [beta] Don't use GetTopLevelWidget on nsDocumentViewer::CreateDeviceContext on macOS. r=#mac-reviewers!

This is an uplift friendly fix which just restores macOS behavior on
this problematic callsite, preventing issues like the one from comment
0.

What nsDeviceContext does with the widget is just calling GetDPI, and
nsChildView has a reasonable implementation, so this should be safe.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9435224 [details]
Bug 1926624 - [beta] Don't use GetTopLevelWidget on nsDocumentViewer::CreateDeviceContext on macOS. r=#mac-reviewers!

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Minimal fix for regression (more proper fix is in still-to-be reviewed patches).
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Minimal fix that restores behavior only on macOS.
• String changes made/needed: none
• Is Android affected?: No

Comment 15

[Simon Friedberger (:simonf)]

I think this is affecting Thunderbird. Do we need a cross-lift?

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/252b49944c8e
Unify top level widget behavior across platforms, standardizing on previous macOS behavior. r=stransky,mac-reviewers,win-reviewers,spohl,nrishel

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3018242e1b5
Simplify cocoa backing scale factor changes. r=mac-reviewers,spohl

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Uplifting to beta would also fix it in tb afaiui

Comment 19

[Jonathan Almeida [:jonalmeida]]

Attachment: similar screenshot with merge account dialog.png

FYI FxA/Sync engineers have observed a similar effect to the "merge account" dialog as well. Noting that here if that helps with verifying/testing this case too.

Comment 20

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/252b49944c8e
https://hg.mozilla.org/mozilla-central/rev/a3018242e1b5

Comment 21

[Donal Meehan [:dmeehan]]

Comment on attachment 9435224 [details]
Bug 1926624 - [beta] Don't use GetTopLevelWidget on nsDocumentViewer::CreateDeviceContext on macOS. r=#mac-reviewers!

Approved for 133.0b7

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/349c675d4289

Comment 23

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Reproduced the issue with Firefox 133.0b6 on macOS 12 with an external monitor with Primary password and Merge Warning dialogs and steps from comment 9. The dialogs are cropped.
The issue is verified fixed with Firefox 134.0a1 (2024-11-10) and 133.0b7 from comment 22 on Windows 10x64, macOS 12 and Ubuntu 24. The Primary password, Merge Warning and test dialogs from comment 9 are correctly displayed.