Closed Bug 1869891 Opened 5 months ago Closed 5 months ago

Mouse cursor sees-through the XUL notifications

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
122 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox120 --- unaffected
firefox121 --- unaffected
firefox122 --- verified
firefox123 --- verified

People

(Reporter: saschanaz, Assigned: emilio, NeedInfo)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files, 4 obsolete files)

image.png
36.86 KB, image/png
Details
Bug 1869891 - Hack around for 122. r=tnikkel
48 bytes, text/x-phabricator-request
Details | Review
Attached image image.png —

Mozregression says it's bug 1865494. Clicking the notification clicks the window below the notification, and the notification window gets the extra border around it. The only culprit I can think of from that bug is the PresShell change as others are all PIP and cocoa specific, so the component.

Repro steps:

  1. Set alerts.useSystemBackend to false
  2. Open https://www.bennish.net/web-notifications.html
  3. Click Authorize and then Show.

Expected: No border around the notification window and you should be able to click the X button
Actual: A thick border around it and trying to click X button clicks irrelevant things under the notification.

This matters more for Thunderbird than Firefox because it still uses XUL notifications on Windows. (I haven't tested whether this affects Linux.)

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

:sev, since you are the author of the regressor, bug 1865494, could you take a look?

For more information, please visit BugBot documentation.

status-firefox120: --- → unaffected
status-firefox121: --- → unaffected
status-firefox122: --- → affected
status-firefox-esr115: --- → unaffected
Flags: needinfo?(sev)

(In reply to Kagami [:saschanaz] (they/them) from comment #0)

The only culprit I can think of from that bug is the PresShell change as others are all PIP and cocoa specific, so the component.

Hi Kagami. Have you tried rolling back the PresShell change?

It was supposed to be a non-functional change on the Windows platform, since on there viewWidget == windowWidget.

I can try myself, but I need to do a non-artifact build on Windows which I can probably only get to during the weekend.

Flags: needinfo?(sev)

Hmm, there could be some confusion between the menu popup widget and the root widget in that code. Hopefully I can take a look soon.

(In reply to Vsevolod Buzinov from comment #2)

(In reply to Kagami [:saschanaz] (they/them) from comment #0)

The only culprit I can think of from that bug is the PresShell change as others are all PIP and cocoa specific, so the component.

Hi Kagami. Have you tried rolling back the PresShell change?

It was supposed to be a non-functional change on the Windows platform, since on there viewWidget == windowWidget.

I can try myself, but I need to do a non-artifact build on Windows which I can probably only get to during the weekend.

Just tried it and rolling back only the PresShell change does fix the issue. 👀

diff --git a/layout/base/PresShell.cpp b/layout/base/PresShell.cpp
index 42351ad15ad20..d0509774fad5d 100644
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -11604,7 +11604,8 @@ void PresShell::SyncWindowProperties(bool aSync) {
     widget::TransparencyMode mode = nsLayoutUtils::GetFrameTransparency(
         canvas ? canvas : rootFrame, rootFrame);
     StyleWindowShadow shadow = rootFrame->StyleUIReset()->mWindowShadow;
-    windowWidget->SetTransparencyMode(mode);
+    nsCOMPtr<nsIWidget> viewWidget = view->GetWidget();
+    viewWidget->SetTransparencyMode(mode);
     windowWidget->SetWindowShadowStyle(shadow);

     // For macOS, apply color scheme overrides to the top level window widget.

Hi Emilio,

Seems that it is possible for viewWidget to be different from windowWidget, should this code be changed to something along these lines?

nsCOMPtr<nsIWidget> viewWidget = view->GetWidget();
viewWidget->SetTransparencyMode(mode);
if (windowWidget != viewWidget) {
  windowWidget->SetTransparencyMode(mode);
}
Flags: needinfo?(emilio)

I looked into this a bit on Windows. It seems that these notifications popups are full top level windows (in gecko terms, ie they are no menupopup frames). And they don't seem to be using the "attach to parent" arrangement for widgets (which is used for the main top level Firefox window). So there is a top level widget that is outside of the frame/view/layout system, and then the widget underneath that that corresponds to the root view/frame.

So I haven't been able to look at this on Windows yet, but the window is created here, and I'm a bit surprised about comment 6.

So I guess questions are:

  • Why does this happen only on Windows? (Can't repro on macOS or Linux)
  • Why are there two widgets there?

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

  • Why are there two widgets there?

That is (or was) the default for every top level window on Windows. Until we added the "attached to parent widget" business, which made the frame/view/layout attach to the top widget instead of creating a child widget.

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

  • Why does this happen only on Windows? (Can't repro on macOS or Linux)

A guess: either the widget setup is different there, or the widgets get set to transparent by default or set to transparent somewhere else?

The reason it doesn't attach is this code

https://searchfox.org/mozilla-central/rev/8b5d93d099b5b501cf5bd2b606c59afcf88bef1c/layout/base/nsDocumentViewer.cpp#3272

The window type is Popup, which seems that it comes from the "popup=yes" in the code you linked. (The second widget that we create, after deciding to not attach, has type Child.)

Seems like we would probably want to attach in this case, but Popup widget type is also likely used for MenuPopupFrames for which we would not want to attach, but I suspect we can't have mParentWidget being the widget for a MenuPopupFrame here.

nsMenupopupFrame doesn't create a doc viewer or such. So I don't think those reach that codepath.

Yeah but maybe if an iframe got created inside the nsMenupopupFrame we could find it's widget as a parent widget there? How does it find parent widget there? Or is parent widget null? We shouldn't try to create a widget for that iframe, so probably won't end up there.

The code in comment 5 seems like maybe a good, less risky regression fix.

If we wanted to attach to the parent widget (and thereby get rid of the second widget here), we could allow attaching to Popup type widgets as long as the widget is not associated to a view.

Yeah something like comment 5 for 122, and trying something like that for 123 sounds like the right way to go imo.

the reason this doesnt happen on linux on Wayland at least is probably because of https://searchfox.org/mozilla-central/rev/07937a5d00e0f65611d8b3bd2992c7603aeaa70d/widget/gtk/nsWindow.cpp#5975

Assignee: nobody → emilio
Flags: needinfo?(emilio)

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.

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

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

Depends on D196665

Markus, Stephen, do you have a good sense of why we need to expose the relevant nsChildView to the view tree in macOS?

If we didn't need to do that, we could unify a bit the handling of widgets (see comment 17).

My understanding is that, for popups, we already have this behavior (the popup content view is not exposed to the view tree and instead handled internally in nsCocoaWindow via mPopupContentView).

So handling it the same way everywhere might be nice anyways. Do you have more context on the current set-up, where top-level windows get an nsChildView attached to the view (as in, nsView) tree, but menupopups get an nsCocoaWindow directly?

Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(mstange.moz)
Blocks: 1870512

Comment on attachment 9369068 [details]
Bug 1869891 - Simplify ShouldAttachToTopLevel(). r=tnikkel

Revision D196664 was moved to bug 1870512. Setting attachment 9369068 [details] to obsolete.

Attachment #9369068 - Attachment is obsolete: true

Comment on attachment 9369069 [details]
Bug 1869891 - Remove top-level windows with WindowType::Popup. r=tnikkel

Revision D196665 was moved to bug 1870512. Setting attachment 9369069 [details] to obsolete.

Attachment #9369069 - Attachment is obsolete: true

Comment on attachment 9369070 [details]
Bug 1869891 - Remove accessibility hack that should not longer be needed. r=Jamie

Revision D196666 was moved to bug 1870512. Setting attachment 9369070 [details] to obsolete.

Attachment #9369070 - Attachment is obsolete: true

Comment on attachment 9369071 [details]
Bug 1869891 - Remove no longer needed wayland panel emulation. r=stransky

Revision D196667 was moved to bug 1870512. Setting attachment 9369071 [details] to obsolete.

Attachment #9369071 - Attachment is obsolete: true

https://hg.mozilla.org/mozilla-central/rev/8bc8704c3d40

Status: NEW → RESOLVED
Closed: 5 months ago
status-firefox122: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 122 Branch
Flags: qe-verify+
Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(mstange.moz)

The question in comment 22 is still relevant.

Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(mstange.moz)

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

The question in comment 22 is still relevant.

Ah right. No, I do not have context around the current setup.

Flags: needinfo?(spohl.mozilla.bugs)

Reproducible on a 2023-12-13 Nightly build on Windows 10.
Verified as fixed on Firefox Nightly 123.0a1 and Firefox 122.0b4 on Windows 10, Ubuntu 22, macOS 12.

Status: RESOLVED → VERIFIED
status-firefox122: fixed → verified
status-firefox123: --- → verified
Flags: qe-verify+
See Also: → 1894328
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: