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

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

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).

Comment 2

[Jim Mathies [:jimm]]

Not sure what this needinfo is about, yes it sounds like we need to add support for this.

Comment 3

[Jim Mathies [:jimm]]

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.

Comment 4

[Edwin Wong [:edwong]]

any updates here Jim? Push team would love for this to get landed by Fx44 uplift to aurora

Comment 6

[Jim Mathies [:jimm]]

(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.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 8

[Matthew N. [:MattN]]

Maybe we could wswitch to a <panel>/<popup> as they can be top-most and have transparency?

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

I'll investigate doing comment #8 when I finish my current round of bugs.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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).

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?

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 17

[Matthew N. [:MattN]]

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.

Comment 18

[Jim Mathies [:jimm]]

(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?

Comment 19

[Jim Mathies [:jimm]]

(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.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 21

[Jim Mathies [:jimm]]

(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?

Comment 22

[Matthew N. [:MattN]]

(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.

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP v2

(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.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

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.

Comment 27

[Matthew N. [:MattN]]

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.

Comment 28

[Matthew N. [:MattN]]

To be clear, I meant that the above CSS didn't cause any Linux regression before the IsTopLevelWidget change but now it does.

Comment 29

[Jim Mathies [:jimm]]

Nice, thanks for the effort!

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

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.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

s/transparent/translucent/

Comment 33

[Alfred Kayser]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #32)
> s/transparent/translucent/

What is the difference in this context?

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06a5ac492313