The default bug view has changed. See this FAQ.

Add support for Aero Glass effects to Widget layer

RESOLVED FIXED in mozilla1.9.1a2

Status

()

Core
Widget: Win32
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: vlad, Assigned: robarnold)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9.1a2
x86
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 9 obsolete attachments)

To do Aero Glass, we need two pieces:

1) Compositor
2) The way to specify Glass regions at the widget layer.

The way Glass works is that you hand the OS a list of rectangles that you want the glass effect applied to in your window.  Because of this API, it would be fairly difficult to get the effect in place before compositor.

Ideally, once this is in place, we will have an associated CSS property, e.g. background: -moz-glass or something to specify when glass should be used.

Note that there may be ways to apply glass to the entire background of the window, but I'm not sure if this is such a good idea -- that could cause unexpected issues with extensions and other things that want to interact with the core chrome.
Flags: blocking1.9-
(Reporter)

Comment 1

9 years ago
The other approach is to use a color key, with a certain color that's replaced with the glass effect.  This is potentially even harder.
(Assignee)

Updated

9 years ago
Depends on: 444005
(Assignee)

Updated

9 years ago
Depends on: 444007
(Assignee)

Updated

9 years ago
Depends on: 444013
(Assignee)

Updated

9 years ago
No longer depends on: 444007
(Assignee)

Comment 2

9 years ago
There are problems with the first two options:
1. The outer frame remains, plus we need to keep track of the rectangles with glass.
2. The color keying makes text rendering looks funny. We'd have to find a color for the key that blends well given the text color.

Here's the third option which I have almost completed (except for the fallback cases). Using DwmExtendFrameIntoClientArea(mWnd, { -1, -1, -1, -1}), the entire window becomes a sheet of glass. The glass regions are controled by the alpha values of the back buffer.

Achieving glass now becomes easy: the root window is given -moz-appearance: glass and any areas of the window that want to be on glass need background: transparent. This also has the benefit of not depending on Compositor.

When non-opaque glass is not available (i.e. no Vista compositor), we have to fake the opaque glass effect by filling the surface with a solid color (the color of the window frame). In classic windows, this is the frame color. In Aero Basic, the frame color changes when the window is focused and unfocused but I'm pretty sure it's a solid color that fills the frame. We'd also want a nice way to let the ui know somehow that glass is disabled.
Assignee: nobody → tellrob
(Assignee)

Updated

9 years ago
Depends on: 444679
(Assignee)

Comment 3

9 years ago
Created attachment 330305 [details] [diff] [review]
v1.0

First draft
Attachment #330305 - Flags: review?(vladimir)
(Assignee)

Updated

9 years ago
Depends on: 438987
No longer depends on: 444005
(Assignee)

Comment 4

9 years ago
Created attachment 330308 [details] [diff] [review]
the real v1.0

I forgot to qrefresh
Attachment #330305 - Attachment is obsolete: true
Attachment #330308 - Flags: review?(vladimir)
Attachment #330305 - Flags: review?(vladimir)
(Assignee)

Updated

9 years ago
Attachment #330308 - Flags: review?(roc)
(Assignee)

Comment 5

9 years ago
Comment on attachment 330308 [details] [diff] [review]
the real v1.0

Hmm, turns out this is still a WIP. Almost there though
Attachment #330308 - Flags: review?(vladimir)
Attachment #330308 - Flags: review?(roc)
(Assignee)

Comment 6

9 years ago
Created attachment 330598 [details] [diff] [review]
Ok, this works (really, I swear!)

Fixed the frame/transparency issues and some stale data bug with theme data
Attachment #330308 - Attachment is obsolete: true
Attachment #330598 - Flags: review?(vladimir)
Attachment #330598 - Flags: review?(roc)
(Assignee)

Comment 7

9 years ago
Comment on attachment 330598 [details] [diff] [review]
Ok, this works (really, I swear!)

There are some odd redraw issues in the fallback cases. I think this will be better handled by letting CSS handle the fallback just as it does with the native theme
Attachment #330598 - Flags: review?(vladimir)
Attachment #330598 - Flags: review?(roc)
(Assignee)

Updated

9 years ago
Depends on: 447045
(Assignee)

Comment 8

9 years ago
Created attachment 330860 [details] [diff] [review]
v1.1

Fallback code is eliminated. Added a -moz-system-metric value to query if the DWM compositor is enabled. Moves mHasAeroGlass from nsWindow to sHasCompositor in nsUXThemeData which simplified a lot of code.
Attachment #330598 - Attachment is obsolete: true
Attachment #330860 - Flags: review?(vladimir)
Attachment #330860 - Flags: review?(roc)
(Assignee)

Comment 9

9 years ago
Created attachment 330864 [details] [diff] [review]
v1.2

Removed some extra cruft that came about during development
Attachment #330860 - Attachment is obsolete: true
Attachment #330864 - Flags: review?(vladimir)
Attachment #330864 - Flags: review?(roc)
Attachment #330860 - Flags: review?(vladimir)
Attachment #330860 - Flags: review?(roc)
(Assignee)

Comment 10

9 years ago
Created attachment 330866 [details] [diff] [review]
v1.3

Forgot to remove some of the fallback bits (the invalidate calls in WM_SETFOCUS and WM_KILLFOCUS)
Attachment #330864 - Attachment is obsolete: true
Attachment #330866 - Flags: review?(vladimir)
Attachment #330866 - Flags: review?(roc)
Attachment #330864 - Flags: review?(vladimir)
Attachment #330864 - Flags: review?(roc)
Rename FrameHasTransparency to GetTransparencyMode.

+      if(rootFrame && NS_THEME_GLASS == rootFrame->GetStyleDisplay()->mAppearance)
+        mode = eTransparencyGlass;

Why is this needed? Shouldn't we just pass rootFrame instead of aFrame to FrameHasTransparency?

+CSS_KEY(glass,glass)

Shouldn't this be -moz-win like the others?

+    sSystemMetrics->AppendElement(do_GetAtom("dwm-compositor"));

For consistency, "windows-dwm-compositor"

+enum nsTransparencyMode {
+  eTransparencyOpaque = 0,
+  eTransparencyTransparent,
+  eTransparencyGlass

Document these please

+    return sHaveCompositor = (compositionIsEnabled ? PR_TRUE : PR_FALSE);

Can't you just return compositionIsEnabled? If you can't, at least write it as compositionIsEnabled != 0.

-  mIsTransparent      = PR_FALSE;
+  mTransparencyMode      = eTransparencyOpaque;

Fix indent

+    if (eTransparencyTransparent == mTransparencyMode)
+    {
+      SetupTranslucentWindowMemoryBitmap(eTransparencyOpaque);

Hmm. Shouldn't we really be testing for mTransparencyMode != Opaque && CompositionEnabled? I asssume that when composition is enabled, transparency-without-glass works the same way as with-glass, i.e. cairo draws RGBA and stuff just works. It would be great if we can use that. If so, we should abstract that check to a new inline function or something.

I think nsIWidget::GetHasTransparentBackground should return nsTransparencyMode. And both it and SetHasTransparentBackground should be renamed to Get/SetTransparencyMode.
> Shouldn't we really be testing for mTransparencyMode != Opaque &&
> CompositionEnabled?

I mean "mTransparencyMode != Opaque && !CompositionEnabled"

Comment 13

9 years ago
On VS 2008 this fails to build with

ON=\"1.9a1\" -DMOZILLA_SKIN_VERSION=\"1.8\"  -D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIE
NT /e/mozilla-build/mozilla/src/layout/generic/nsContainerFrame.cpp
nsContainerFrame.cpp
e:/mozilla-build/mozilla/src/layout/generic/nsContainerFrame.cpp(502) : error C2
039: 'GetRootElementStyleFrame' : is not a member of 'nsCSSFrameConstructor'
        e:/mozilla-build/mozilla/src/layout/generic/../base\nsCSSFrameConstructo
r.h(84) : see declaration of 'nsCSSFrameConstructor'
make[6]: *** [nsContainerFrame.obj] Error 2

Comment 14

9 years ago
Argh, nevermind, I'm blind. Sorry for spam.
(In reply to comment #7)
> (From update of attachment 330598 [details] [diff] [review])
> There are some odd redraw issues in the fallback cases. I think this will be
> better handled by letting CSS handle the fallback just as it does with the
> native theme

The default theme detection is a hack that allows us to hardcode colors, borders etc. that would better be handled by the native theme code. Please don't take it as a leading case. If there's a way to fix the odd redraw issues, that would be great.
(Assignee)

Comment 16

9 years ago
(In reply to comment #11)
> Rename FrameHasTransparency to GetTransparencyMode.
> 
> +      if(rootFrame && NS_THEME_GLASS ==
> rootFrame->GetStyleDisplay()->mAppearance)
> +        mode = eTransparencyGlass;
> 
> Why is this needed? Shouldn't we just pass rootFrame instead of aFrame to
> FrameHasTransparency?

I found that if I passed rootFrame instead, ordinary firefox windows would be transparent.

> 
> +CSS_KEY(glass,glass)
> 
> Shouldn't this be -moz-win like the others?

I don't think there are any -moz-appearance values which have a -moz-win prefix even though some are windows specific.

> 
> +    sSystemMetrics->AppendElement(do_GetAtom("dwm-compositor"));
> 
> For consistency, "windows-dwm-compositor"

I figured dwm implied windows. How about windows-compositor?

> +    return sHaveCompositor = (compositionIsEnabled ? PR_TRUE : PR_FALSE);
> 
> Can't you just return compositionIsEnabled? If you can't, at least write it as
> compositionIsEnabled != 0.

I was hoping to avoid implicit BOOL to PRBool conversions. Also, the assignment is the important part of the function. Returning the value is just a convenience to the caller in the WM_DWMCOMPOSITIONCHANGE handler.

> 
> +    if (eTransparencyTransparent == mTransparencyMode)
> +    {
> +      SetupTranslucentWindowMemoryBitmap(eTransparencyOpaque);
> 
> Hmm. Shouldn't we really be testing for mTransparencyMode != Opaque &&
> CompositionEnabled? I asssume that when composition is enabled,
> transparency-without-glass works the same way as with-glass, i.e. cairo draws
> RGBA and stuff just works. It would be great if we can use that. If so, we
> should abstract that check to a new inline function or something.

Transparency paints differently than glass. With transparency, we render with alpha to an offscreen DC and use ::UpdateLayeredWindow to blit to the window. Glass is rendered much like an opaque window except that we push a color-alpha group instead of a color group to draw on.
(In reply to comment #16)
> I found that if I passed rootFrame instead, ordinary firefox windows would be
> transparent.

> > +CSS_KEY(glass,glass)
> > 
> > Shouldn't this be -moz-win like the others?
> 
> I don't think there are any -moz-appearance values which have a -moz-win
> prefix even though some are windows specific.

Isn't -moz-win-media-toolbox directly above an example of that?

> > +    sSystemMetrics->AppendElement(do_GetAtom("dwm-compositor"));
> > 
> > For consistency, "windows-dwm-compositor"
> 
> I figured dwm implied windows. How about windows-compositor?

Sure.

> I was hoping to avoid implicit BOOL to PRBool conversions. Also, the 
> assignment is the important part of the function. Returning the value is just
> a convenience to the caller in the WM_DWMCOMPOSITIONCHANGE handler.

Just use != 0 then.

> Transparency paints differently than glass. With transparency, we render with
> alpha to an offscreen DC and use ::UpdateLayeredWindow to blit to the window.
> Glass is rendered much like an opaque window except that we push a color-alpha
> group instead of a color group to draw on.

I'm saying, when composition is available should we render transparency the same way as glass? It seems plausible that might work better than UpdateLayeredWindow.
(In reply to comment #17)
> (In reply to comment #16)
> > I found that if I passed rootFrame instead, ordinary firefox windows would
> > be transparent.

Oops ... I understand now. The issue here is that the CSS 'background' propagates from the root element's frame (rootFrame) to the real root frame (nsViewportFrame), so we need to call FrameHasTransparency on that. But -moz-appearance does not propagate so we need to check that directly on rootFrame.

So I'm OK with this part of your patch, but you might want to write what I just said in as a comment.
(Assignee)

Comment 19

9 years ago
(In reply to comment #17)

> > I don't think there are any -moz-appearance values which have a -moz-win
> > prefix even though some are windows specific.
> 
> Isn't -moz-win-media-toolbox directly above an example of that?

Oops, yes. I was thinking of my menu changes from last year which were Vista/Windows only but had no -moz prefix. I'm not sure if I like having a second -moz prefix (since there's one for -moz-appearance already). What about aero-glass?

> > Transparency paints differently than glass. With transparency, we render with
> > alpha to an offscreen DC and use ::UpdateLayeredWindow to blit to the window.
> > Glass is rendered much like an opaque window except that we push a color-alpha
> > group instead of a color group to draw on.
> 
> I'm saying, when composition is available should we render transparency the
> same way as glass? It seems plausible that might work better than
> UpdateLayeredWindow.
> 
I don't think we can. Transparency requires layered windows (because hit detection works differently with transparency vs glass and transparency works with the old window manager). I'll try it out though to be sure.
(In reply to comment #19)
> (In reply to comment #17)
> 
> > > I don't think there are any -moz-appearance values which have a -moz-win
> > > prefix even though some are windows specific.
> > 
> > Isn't -moz-win-media-toolbox directly above an example of that?
> 
> Oops, yes. I was thinking of my menu changes from last year which were
> Vista/Windows only but had no -moz prefix. I'm not sure if I like having a
> second -moz prefix (since there's one for -moz-appearance already). What about
> aero-glass?

We already had that discussion about media-toolbox and friends, let's not reopen it.

> I don't think we can. Transparency requires layered windows (because hit
> detection works differently with transparency vs glass and transparency works
> with the old window manager). I'll try it out though to be sure.

Good point about hit detection.
(Assignee)

Comment 21

9 years ago
(In reply to comment #15)
> 
> The default theme detection is a hack that allows us to hardcode colors,
> borders etc. that would better be handled by the native theme code. Please
> don't take it as a leading case. If there's a way to fix the odd redraw issues,
> that would be great.
> 

I hope we don't end up hardcoding colors. The colors needed for the fallback aren't documented however, so I'm not sure that I found the "right" ones in the theme. I think giving the theme the ability to change itself based on the availability of the compositor is better than doing the fallback in the code because the fallback I pick might not be what they want (ex: they might want to alter the layout or swap images around or whatnot). I also feel that the fix for the redraw issues would be a larger hack than hard coding colors.
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 22

9 years ago
Created attachment 331010 [details] [diff] [review]
v1.4

Roc's comments are addressed. Rendering of content breaks the glass effect still.
Attachment #330866 - Attachment is obsolete: true
Attachment #330866 - Flags: review?(vladimir)
Attachment #330866 - Flags: review?(roc)
(Assignee)

Comment 23

9 years ago
Created attachment 331234 [details] [diff] [review]
v1.4 - reduced

This patch breaks the rendering of content on transparent windows but I can't quite figure out why. I've tried to reduce the patch to eliminate the changes I made except for the change from a boolean to enum for transparency and the addition of the windows-compositor css selector.
(Assignee)

Comment 24

9 years ago
Created attachment 332447 [details] [diff] [review]
v1.5

Content no longer breaks glass. Instead content is now rendered non-opaquely. Since content never quite rendered properly on transparent windows before, I don't really care too much. Supposedly compositor will fix this/make the fix easier.
Attachment #331010 - Attachment is obsolete: true
Attachment #331234 - Attachment is obsolete: true
Attachment #332447 - Flags: superreview?(vladimir)
Attachment #332447 - Flags: review?(roc)
(Reporter)

Comment 25

9 years ago
Comment on attachment 332447 [details] [diff] [review]
v1.5


Looks fine, with some comments:
 
> #ifdef MOZ_XUL
>-      if (mIsTransparent) {
>+      if (eTransparencyGlass == mTransparencyMode && nsUXThemeData::sHaveCompositor) {
>+        thebesContext->PushGroup(gfxASurface::CONTENT_COLOR_ALPHA);
>+        thebesContext->SetOperator(gfxContext::OPERATOR_CLEAR);
>+        thebesContext->Paint();
>+        thebesContext->SetOperator(gfxContext::OPERATOR_OVER);
>+      } else if (eTransparencyTransparent == mTransparencyMode) {

When you PushGroup, it should already be cleared -- you shouldn't need the SetOperator/Paint/SetOperator bits here.

>+  NS_IMETHOD              GetTransparencyMode(nsTransparencyMode& aMode);

Since you're changing this anyway, should we make it sane and just have it be 'nsTransparencyMode GetTransparencyMode()' everywhere?  That would simplify a lot of the:

nsTransparencyMode trans = eTransparencyModeOpaque;
GetTransparencyMode(trans);
if (trans == ..) { ... }

patterns a lot as well.  roc, what do you think?  nsIWindow isn't a real XPCOM interface any more anyway, right?
Attachment #332447 - Flags: superreview?(vladimir) → superreview+
nsIWidget? Absolutely not a real XPCOM interface. deCOMtaminate at will.
Rename FrameHasTransparency to GetFrameTransparency.

+     * The value of this metric is not used on other platforms. These platforms
+     * should return NS_ERROR_NOT_IMPLEMENTED when queried for this metric.

You need to mention "Windows" somewhere around here.

+    if(nsUXThemeData::CheckForCompositor() && mTransparencyMode == eTransparencyGlass) {

Space after "if"

+      } else if (eTransparencyGlass == mTransparencyMode && result) {
+        thebesContext->PopGroupToSource();
+        thebesContext->SetOperator(gfxContext::OPERATOR_SOURCE);
+        thebesContext->Paint();

Why is this needed? The following "if (result)" clause does the same thing...

Otherwise looks great.
(Assignee)

Comment 28

9 years ago
Created attachment 333422 [details] [diff] [review]
v1.6

Review comments addressed. Patch went through try server ok. There's probably more deCOMtamination to be done for nsIWidget, but that's outside the scope of this patch.
Attachment #332447 - Attachment is obsolete: true
Attachment #332447 - Flags: review?(roc)
(Assignee)

Comment 29

9 years ago
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/460fce61ebc7
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Comment on attachment 333422 [details] [diff] [review]
v1.6

>+  // UXTheme.dll Function typedefs and declarations
>   typedef HANDLE (WINAPI*OpenThemeDataPtr)(HWND hwnd, LPCWSTR pszClassList);
>   typedef HRESULT (WINAPI*CloseThemeDataPtr)(HANDLE hTheme);
>   typedef HRESULT (WINAPI*DrawThemeBackgroundPtr)(HANDLE hTheme, HDC hdc, int iPartId, 
>                                             int iStateId, const RECT *pRect,
>                                             const RECT* pClipRect);
>   typedef HRESULT (WINAPI*DrawThemeEdgePtr)(HANDLE hTheme, HDC hdc, int iPartId, 
>                                             int iStateId, const RECT *pDestRect,
>                                             uint uEdge, uint uFlags,
>@@ -120,21 +127,37 @@ public:
>                                      int iStateId, int iPropId, OUT COLORREF* pFont);
>   typedef HRESULT (WINAPI*GetThemeMarginsPtr)(HANDLE hTheme, HDC hdc, int iPartId,
>                                            int iStateid, int iPropId,
>                                            LPRECT prc, MARGINS *pMargins);
>   typedef BOOL (WINAPI*IsAppThemedPtr)(VOID);
>   typedef HRESULT (WINAPI*GetCurrentThemeNamePtr)(LPWSTR pszThemeFileName, int dwMaxNameChars,
>                                                   LPWSTR pszColorBuff, int cchMaxColorChars,
>                                                   LPWSTR pszSizeBuff, int cchMaxSizeChars);
>+  typedef COLORREF (WINAPI*GetThemeSysColorPtr)(HTHEME hTheme, int iColorID);
Any chance we can change this to HANDLE a) for consistency with the other entries b) because it doesn't work with some older supported SDKs?
Created attachment 333729 [details] [diff] [review]
HTHEME (pushed: 198b8dde7a94)
Attachment #333729 - Flags: review?(tellrob)
(Assignee)

Updated

9 years ago
Attachment #333729 - Flags: review?(tellrob) → review+

Updated

9 years ago
Attachment #333729 - Flags: review?(emaijala)
(In reply to comment #30)
> b) because it doesn't work with some older supported SDKs?

Trying to build "default" Firefox, with VC++ 2005/v8 + PSDK 2003R2:
{{
nsWindow.cpp
...\widget\src\windows\nsUXThemeData.h(135)
: error C2061: syntax error : identifier 'HTHEME'
make[6]: *** [nsWindow.obj] Error 2
}}
(Reporter)

Updated

9 years ago
Attachment #333729 - Flags: review?(emaijala) → review+

Updated

9 years ago
Attachment #333729 - Attachment description: HTHEME → HTHEME (pushed: 198b8dde7a94)
(In reply to comment #32)

V.Fixed, on this part, after additional checkin.
Target Milestone: --- → mozilla1.9.1a2
(Assignee)

Updated

9 years ago
Blocks: 450767
(Reporter)

Comment 34

9 years ago
Created attachment 341167 [details] [diff] [review]
small crash fix

Small crash fix that shows up if you use -chrome with a .html file without a background on the body element.  The transparency bitmap isn't created by the time we get to OnPaint, and things blow up.  This isn't really well-supported (-chrome with .html), but we should avoid the rash in any case.
Attachment #341167 - Flags: review?(tellrob)
(Assignee)

Updated

9 years ago
Attachment #341167 - Flags: review?(tellrob) → review+
(Assignee)

Comment 35

9 years ago
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/8d8ef01ef749

Comment 36

9 years ago
I guess this code is really buggy. Why does it make black colored labels shown in glass too? Have a look at the source of the Addon glasser - it shows glass on the top of the window AND let's the black labels of eg. the menus shown in black.

Also I think there should be a possibility to only show glass for x pixels from top. This is not that difficult to develop...
Could you file new bugs for that?
(Assignee)

Comment 38

9 years ago
I'm not sure I understand what you mean by black colored labels being shown on glass. Do you want them shown or not? Please file a bug and explain there.

As for only showing glass x pixels from the top, that's not entirely desirable (we'd like something more flexible). See bug 450767 for that.

Comment 39

9 years ago
https://bugzilla.mozilla.org/show_bug.cgi?id=458407

Updated

9 years ago
Depends on: 458407
You need to log in before you can comment on or make changes to this bug.