Closed Bug 418454 Opened 17 years ago Closed 16 years ago

Add support for Aero Glass effects to Widget layer

Categories

(Core :: Widget: Win32, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1a2

People

(Reporter: vlad, Assigned: robarnold)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 9 obsolete files)

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-
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.
Depends on: 444005
Depends on: 444007
Depends on: 444013
No longer depends on: 444007
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
Depends on: 444679
Attached patch v1.0 (obsolete) (deleted) — Splinter Review
First draft
Attachment #330305 - Flags: review?(vladimir)
Depends on: 438987
No longer depends on: 444005
Attached patch the real v1.0 (obsolete) (deleted) — Splinter Review
I forgot to qrefresh
Attachment #330305 - Attachment is obsolete: true
Attachment #330308 - Flags: review?(vladimir)
Attachment #330305 - Flags: review?(vladimir)
Attachment #330308 - Flags: review?(roc)
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)
Attached patch Ok, this works (really, I swear!) (obsolete) (deleted) — Splinter Review
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)
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)
Depends on: 447045
Attached patch v1.1 (obsolete) (deleted) — Splinter Review
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)
Attached patch v1.2 (obsolete) (deleted) — Splinter Review
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)
Attached patch v1.3 (obsolete) (deleted) — Splinter Review
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"
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
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.
(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.
(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.
(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.
Status: NEW → ASSIGNED
Attached patch v1.4 (obsolete) (deleted) — Splinter Review
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)
Attached patch v1.4 - reduced (obsolete) (deleted) — Splinter Review
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.
Attached patch v1.5 (obsolete) (deleted) — Splinter Review
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)
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.
Attached patch v1.6 (deleted) — Splinter Review
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)
Status: ASSIGNED → RESOLVED
Closed: 16 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?
Attached patch HTHEME (pushed: 198b8dde7a94) (deleted) — Splinter Review
Attachment #333729 - Flags: review?(tellrob)
Attachment #333729 - Flags: review?(tellrob) → review+
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 }}
Attachment #333729 - Flags: review?(emaijala) → review+
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
Blocks: 450767
Attached patch small crash fix (deleted) — Splinter Review
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)
Attachment #341167 - Flags: review?(tellrob) → review+
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?
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.
Depends on: 458407
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: