1870512 - Remove top-level windows with popup=true

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

See bug 1869891 for context. The popup=true variants are super weird and already not supported effectively on Wayland.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1870512 - Simplify ShouldAttachToTopLevel(), and revert patch for bug 1869891. r=tnikkel

Definitely not for 122, but I think we should try this. Note that this
doesn't affect nsMenuPopupFrames, which create the widget on their own
here:

https://searchfox.org/mozilla-central/rev/07937a5d00e0f65611d8b3bd2992c7603aeaa70d/layout/xul/nsMenuPopupFrame.cpp#266

It only affects windows with popup=yes, which right now are only the
nsXULAlerts windows.

I'll look into trying to unify the macOS set-up, which right now
unconditionally uses this (presumably to get an nsChildView for each
top-level nsCocoaWindow). Maybe that can be handled internally by the
widget layer, we'll see.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1870512 - Remove top-level windows with WindowType::Popup. r=tnikkel

These are only used right now for XUL notifications. They are extremely
weird (I bet there's tons of code that assume that WindowType::Popup
corresponds to nsMenuPopupFrame), and afaict there's no good reason why
these have to be this way.

They no longer are on Linux (because Wayland doesn't support such
thing for example).

XUL alerts work fine with this patch on all platforms after some basic
testing.

Depends on D196664

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1870512 - Remove accessibility hack that should not longer be needed. r=Jamie

There's no windows with popup=true anymore as of the previous patch.

Depends on D196665

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1870512 - Remove no longer needed wayland panel emulation. r=stransky

Depends on D196666

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39cc9d155c12
Simplify ShouldAttachToTopLevel(), and revert patch for bug 1869891. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/cf0bda3b4253
Remove top-level windows with WindowType::Popup. r=tnikkel,sessionstore-reviewers,dao
https://hg.mozilla.org/integration/autoland/rev/846e436ca1c2
Remove accessibility hack that should not longer be needed. r=Jamie
https://hg.mozilla.org/integration/autoland/rev/c909d207f316
Remove no longer needed wayland panel emulation. r=stransky

Comment 6

[Norisz Fay [:noriszfay]]

Backed out changeset 846e436ca1c2 for causing automation.py & accessibility related Windows failures

Backout link

Push with failures

Failure log 1 // Failure log 2

This mda failure might be related too.

P.S: The other 3 changesets have been backed out too since all the issues were still present after the first backout. Backout link here

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f3e10bdd2af
Simplify ShouldAttachToTopLevel(), and revert patch for bug 1869891. r=tnikkel

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fd01e6f31ce
Remove top-level windows with WindowType::Popup. r=tnikkel,sessionstore-reviewers,dao

Comment 9

[Emilio Cobos Álvarez (:emilio)]

FYI I just noticed https://searchfox.org/comm-central/rev/fc51b5b1da584c4099d8e94d45b1ed3af521da1b/mailnews/base/src/MailNotificationManager.jsm#413. It specifies dialog=yes and popup=yes, which makes no sense. Likely just removing popup=yes there is the right way to go.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8453c280dd2
Remove accessibility hack that should not longer be needed. r=Jamie
https://hg.mozilla.org/integration/autoland/rev/aa4d4fecaf96
Remove no longer needed wayland panel emulation. r=stransky

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/904c9133f749
Allow another Msg_ProcessUnhandledEvent on win32. CLOSED TREE

Comment 12

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/7f3e10bdd2af
https://hg.mozilla.org/mozilla-central/rev/7fd01e6f31ce
https://hg.mozilla.org/mozilla-central/rev/e8453c280dd2
https://hg.mozilla.org/mozilla-central/rev/aa4d4fecaf96
https://hg.mozilla.org/mozilla-central/rev/904c9133f749

Comment 13

[Magnus Melin [:mkmelin]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)
Filed bug 1871389.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9369115 [details]
Bug 1870512 - Simplify ShouldAttachToTopLevel(), and revert patch for bug 1869891. r=tnikkel

Beta/Release Uplift Approval Request

• User impact if declined: This should be enough to fix the rendering issue bug 1869796 originally reported.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: none
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Trivial-ish patch, but with non-trivial implications.
• String changes made/needed: none
• Is Android affected?: Yes

Comment 15

[Donal Meehan [:dmeehan]]

Comment on attachment 9369115 [details]
Bug 1870512 - Simplify ShouldAttachToTopLevel(), and revert patch for bug 1869891. r=tnikkel

We are at the end of the Beta cycle.
Rejecting uplift request, given this has a medium risk and comment 43 on 186979
We can revisit if it's deemed a further conversation is needed.