Closed Bug 1211635 Opened 6 years ago Closed 6 years ago

XUL notifications lack a transparent background (large opaque background surrounding the notification)

Categories

(Core :: Widget: Win32, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(1 file, 3 obsolete files)

Right now popups and dialogs are prevents from being layered windows, which means we can't show them with a transparent background.

We need a transparent window for the notification popup. This has been a longstanding issue but is now much more visible with the work that is being done in bug 1201397.

I'm not sure what ramifications bug 450322 may have for this bug but it may be good to double-check.
Jim, I looked in to this and it appears that having dialog=yes will stop us from setting an inner translucency of the window to eTransparencyTransparent. We need dialog=yes to get us a topmost window, or else the notification can fall behind a window that is foregrounded over it.

The patch in bug 1201397 should make it easy to test with this, though it is not a reduced test case and is not necessary (it just makes the issue much easier to see).
Flags: needinfo?(jmathies)
Not sure what this needinfo is about, yes it sounds like we need to add support for this.
Flags: needinfo?(jmathies)
Looking at our widget code, this looks non-trivial to do. I'll need to do some more digging. I'll try to poke at this more this weekend.
Flags: needinfo?(jmathies)
any updates here Jim? Push team would love for this to get landed by Fx44 uplift to aurora
Duplicate of this bug: 1215840
Blocks: 1215840
Blocks: 1216585
(In reply to Edwin Wong [:edwong] from comment #4)
> any updates here Jim? Push team would love for this to get landed by Fx44
> uplift to aurora

Sorry I haven't had a chance to look at this yet. If it's pressing we should try to find a different owner.
No longer blocks: 1216585
To put a finer point on what Jim said, e10s is Jim's #1 priority right now, so I can't imagine him getting to this bug in for a couple months.
Flags: needinfo?(jmathies)
Maybe we could wswitch to a <panel>/<popup> as they can be top-most and have transparency?
I'll investigate doing comment #8 when I finish my current round of bugs.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Summary: Provide a CSS property or window styling that allows a dialog=yes window to have eTransparencyTransparent → XUL notifications lack a transparent background (large opaque background surrounding the notification)
Duplicate of this bug: 1218092
Duplicate of this bug: 1216692
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Duplicate of this bug: 995351
Assignee: nobody → jaws
Status: NEW → ASSIGNED
This patch is very close to the behavior that we would like but it has one bug that needs to be fixed still.

After showing a notification, the owner window gets its window position changed to HWND_TOPMOST. I've tried using the SWP_NOOWNERZORDER flag to prevent this but I think there is another spot in the code that isn't accounting for this (I'm guessing nsWindow::OnWindowPosChanged but I will need to debug more).

We will probably want to rename mIsTaskbarSuppressed, but I would like to continue to use one flag for both taskbar suppression and topmost windows (CHROME_SUPPRESS_TASKBAR) as we are running out of space in the bitfield of the nsIWebBrowserChrome flags.

Jim and Matt, what do you think about this patch so-far?
Attachment #8687516 - Flags: feedback?(jmathies)
Attachment #8687516 - Flags: feedback?(MattN+bmo)
The HWND_TOPMOST window position only gets propagated to the other window if the other window is focused while the alert window is alive. I stepped through nsWindow::Create and the dialog is being created with a NULL owner so I don't think SWP_NOOWNERZORDER is necessary. Also confirmed via WinSpy++ that the dialog has no owner window.

Perhaps we have some chunk of code that when trying to move the browser window to the foreground in the process of activating it, is trying to move it in front of the dialog window which is causing it to get the HWND_TOPMOST position?
I think I've narrowed it down to this callstack, though setting a breakpoint at the `else if (!(GetWindowLongPtrW(wndAfter ...)` line in nsWindow::PlaceBehind isn't getting hit when the bug reproduces:
http://screencast.com/t/Ndlwi110

What's happening is that the notification window has HWND_TOPMOST, and when the browser window is activated we want to bring it to the foreground but it is constrained to remain behind the notification window. The browser window's z-order gets changed to be as high as possible but still behind the notification window.

Neil, you wrote some of this original code long ago (bug 347513). Do you have an idea of what is going wrong here?
Flags: needinfo?(neil)
Comment on attachment 8687516 [details] [diff] [review]
WIP, top-level window with transparancy and not appearing in Windows taskbar

Review of attachment 8687516 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/alerts/nsXULAlerts.cpp
@@ -150,5 @@
>    rv = argsArray->AppendElement(scriptableAlertSource);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    nsCOMPtr<nsIDOMWindow> newWindow;
> -  nsAutoCString features("chrome,dialog=yes,titlebar=no,popup=yes");

Why not popup=yes? popup=yes should always imply taskbarSuppressed=yes making a new option not necessary, right?

::: toolkit/themes/windows/global/alerts/alert.css
@@ +10,5 @@
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  
> +#alertNotification {
> +  background: transparent;

OS X already has this (with `-moz-appearance: none;`) so I think we should put this in shared (move the OS X-specific CSS too) so Linux gets transparency too immediately when the platform supports it. The transparency doesn't currently seem to work on my Fedora machine (without your patch) but setting background to transparent and moz-appearance: none doesn't seem to make things any worse.
Attachment #8687516 - Flags: feedback?(MattN+bmo)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> Created attachment 8687516 [details] [diff] [review]
> WIP, top-level window with transparancy and not appearing in Windows taskbar
> 
> This patch is very close to the behavior that we would like but it has one
> bug that needs to be fixed still.
> 
> After showing a notification, the owner window gets its window position
> changed to HWND_TOPMOST. I've tried using the SWP_NOOWNERZORDER flag to
> prevent this but I think there is another spot in the code that isn't
> accounting for this (I'm guessing nsWindow::OnWindowPosChanged but I will
> need to debug more).

So what happens here? The browser window gains HWND_TOPMOST status or is the owner some other window?
(In reply to Matthew N. [:MattN] from comment #17)
> Comment on attachment 8687516 [details] [diff] [review]
> WIP, top-level window with transparancy and not appearing in Windows taskbar
> 
> Review of attachment 8687516 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/alerts/nsXULAlerts.cpp
> @@ -150,5 @@
> >    rv = argsArray->AppendElement(scriptableAlertSource);
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> >    nsCOMPtr<nsIDOMWindow> newWindow;
> > -  nsAutoCString features("chrome,dialog=yes,titlebar=no,popup=yes");
> 
> Why not popup=yes? popup=yes should always imply taskbarSuppressed=yes
> making a new option not necessary, right?

This seems like it might be a good idea, looking at the nsWindow code popups get positioned similarly, and they have the WS_EX_TOOLWINDOW extended style.
(In reply to Jim Mathies [:jimm] from comment #18)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> > Created attachment 8687516 [details] [diff] [review]
> > WIP, top-level window with transparancy and not appearing in Windows taskbar
> > 
> > This patch is very close to the behavior that we would like but it has one
> > bug that needs to be fixed still.
> > 
> > After showing a notification, the owner window gets its window position
> > changed to HWND_TOPMOST. I've tried using the SWP_NOOWNERZORDER flag to
> > prevent this but I think there is another spot in the code that isn't
> > accounting for this (I'm guessing nsWindow::OnWindowPosChanged but I will
> > need to debug more).
> 
> So what happens here? The browser window gains HWND_TOPMOST status or is the
> owner some other window?

The browser window gains HWND_TOPMOST status when it is activated. The notification doesn't have an owner.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15)
> The HWND_TOPMOST window position only gets propagated to the other window if
> the other window is focused while the alert window is alive.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> (In reply to Jim Mathies [:jimm] from comment #18)
> > So what happens here? The browser window gains HWND_TOPMOST status or is the
> > owner some other window?
> 
> The browser window gains HWND_TOPMOST status when it is activated. The
> notification doesn't have an owner.

So if the alert window displays and firefox has foreground status, after the alert goes away firefox is locked in the foreground? Is that correct?

Is there some way to easily trigger one of these alerts with this work applied?
(In reply to Jim Mathies [:jimm] from comment #21)
> Is there some way to easily trigger one of these alerts with this work
> applied?

Either a test page like https://people.mozilla.org/~mnoorenberghe/w3c_notifications.htm
or with
> new Notification("some text")
in a Web or Browser Console.
Attached patch WIP v2 (obsolete) — Splinter Review
(In reply to Jim Mathies [:jimm] from comment #21)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15)
> > The HWND_TOPMOST window position only gets propagated to the other window if
> > the other window is focused while the alert window is alive.
> 
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> > (In reply to Jim Mathies [:jimm] from comment #18)
> > > So what happens here? The browser window gains HWND_TOPMOST status or is the
> > > owner some other window?
> > 
> > The browser window gains HWND_TOPMOST status when it is activated. The
> > notification doesn't have an owner.
> 
> So if the alert window displays and firefox has foreground status, after the
> alert goes away firefox is locked in the foreground? Is that correct?

That's not entirely correct. Firefox only gets locked in the foreground if the Firefox window loses focus and is then re-activated while the notification is still being displayed.

If Firefox displays a notification, then nothing happens until the notification is closed (manually or via timeout), then the Firefox window is focused, the window will not be promoted to HWND_TOPMOST. If however, while the notification is still showing, the Firefox window is backgrounded by another application, and then Firefox is re-activated, then Firefox will gain HWND_TOPMOST.

This comment in nsXULWindow.cpp seems interesting:
    /* CalculateZPosition can tell us to be below nothing, because it tries
       not to change something it doesn't recognize. A request to verify
       being below an unrecognized window, then, is treated as a request
       to come to the top (below null) */

I'm spending today digging deeper here, but this new version of the patch removes the taskbarSuppressed flag that I introduced in the first version and tweaks nsContainerFrame::IsTopLevelWidget to return true for popups that have their popupLevel == ePopupLevelTop.
Attachment #8687516 - Attachment is obsolete: true
Attachment #8687516 - Flags: feedback?(jmathies)
nsAppShellWindowEnumerator.cpp does some checks to make sure that the windows are of the same type, so I tried changing alert.xul to have windowtype="navigator:browser" to match the browser windows but the bug still exists.

Note that in the attached patch, `#pragma optimize("", off)` and `#pragma optimize("", on)` are used to disable and re-enable MSVC optimizations, respectively. I just inserted those to help with debugging.
I accidentally cleared the needinfo flag for Jim in an earlier comment.

Jim, can you apply the attached patch and see if you can reproduce the issue where the browser window gets promoted to HWND_TOPMOST if given focus while a notification is open?
Flags: needinfo?(jmathies)
Attached patch Patch (obsolete) — Splinter Review
The alwaysRaised=1 window feature was causing the issues that I was having with other Firefox windows getting their window position changed.

It turns out that the only changes needed are the change in nsContainerFrame.cpp to recognize a top-level popup as a top-level window and changing the background to transparent.

roc, I flagged you for review since you wrote this function as part of bug 322074.
Attachment #8690092 - Attachment is obsolete: true
Flags: needinfo?(neil)
Flags: needinfo?(jmathies)
Attachment #8690251 - Flags: review?(roc)
Attachment #8690251 - Flags: review?(MattN+bmo)
Comment on attachment 8690251 [details] [diff] [review]
Patch

Review of attachment 8690251 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, I was optimistic for something as minimal as this.

r=me with a fix to avoid making Linux worse.

::: toolkit/themes/shared/alert-common.css
@@ +11,5 @@
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  
> +#alertNotification {
> +  -moz-appearance: none;
> +  background: transparent;

I said I tested putting this in shared and it worked on Linux (which I double-checked) but it no longer has the desired effect on Linux after the nsContainerFrame change as it renders a transparent box with the native window shadow and no contents. When the alert shifts due to others fading out, it translates the video memory from that original region to the new position which is definitely not right. I guess there is some Linux widget code that isn't handling transparent top-level widgets properly so we should either go back to putting this style in the windows stylesheet (and make it clear that this bug is about Windows), or make the IsTopLevelWidget not apply to Linux for now?

We will need to fix the Linux issue in a separate bug if we want to use the same shared animation for XUL widgets.
Attachment #8690251 - Flags: review?(MattN+bmo) → review+
To be clear, I meant that the above CSS didn't cause any Linux regression before the IsTopLevelWidget change but now it does.
Nice, thanks for the effort!
Comment on attachment 8690251 [details] [diff] [review]
Patch

Review of attachment 8690251 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsContainerFrame.cpp
@@ +599,5 @@
>  {
>    nsWindowType windowType = aWidget->WindowType();
> +  if (windowType == eWindowType_popup) {
> +    nsBaseWidget* baseWidget = static_cast<nsBaseWidget*>(aWidget);
> +    return baseWidget->PopupLevel() == ePopupLevelTop;

Seems to me this should return true for all popups, logically.
Attachment #8690251 - Flags: review?(roc)
Attached patch Patch v1.1Splinter Review
I tested this patch out on Windows and Linux and didn't see any irregularities related to the popuplevel change. Due to the graphics issues that MattN and I saw, we are keeping the style changes separate from Linux so it will still have the opaque background but no graphics issues.

Windows and OSX pre-Notification Center will have transparent alert dialogs.
Attachment #8690251 - Attachment is obsolete: true
Attachment #8691403 - Flags: review?(roc)
s/transparent/translucent/
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #32)
> s/transparent/translucent/

What is the difference in this context?
(In reply to Alfred Kayser from comment #33)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #32)
> > s/transparent/translucent/
> 
> What is the difference in this context?

Just that transparent is usually used when all light comes through an object (the object between is not visible) and translucent means some light but not all light comes through the object.
https://hg.mozilla.org/integration/fx-team/rev/06a5ac492313d07196cecbfce01e55177536d5a4
Bug 1211635 - Popups should be treated as top-level windows, allowing XUL alerts translucency. r=MattN r=roc
https://hg.mozilla.org/mozilla-central/rev/06a5ac492313
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.