User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20101003 Firefox/4.0b7pre Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20101003 Firefox/4.0b7pre Latest build enabled D3d10 layer and using "Personas" doesn't show the maximize, minimize, close buttons in the upper right corner Reproducible: Always Steps to Reproduce: 1. Use Minefield latest build (03/10/10) 2. Use a "Personas" theme 3. enable D3D10 layer (about:config - layers.use-d3d10;true) Actual Results: maximize minimize close buttons do not show in the upper right corner of the Minefield window. Sometimes if you pass the mouse over the location of the buttons they do show, but not everytime. Expected Results: Show maximize minimize close buttons ass other windows do. about:buildconfig Source Built from http://hg.mozilla.org/mozilla-central/rev/7593c4baa1f7 Build platform target i686-pc-mingw32 Build tools Compiler Version Compiler flags d;D:\mozilla-build\msys\mozilla-build\python25\python2.5.exe -O e;D:\mozilla-build\msys\builds\moz2_slave\mozilla-central-win32-nightly\build\build\cl.py cl 14.00.50727.762 -TC -nologo -W3 -Gy -Fdgenerated.pdb -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1 d;D:\mozilla-build\msys\mozilla-build\python25\python2.5.exe -O e;D:\mozilla-build\msys\builds\moz2_slave\mozilla-central-win32-nightly\build\build\cl.py cl 14.00.50727.762 -GR- -TP -nologo -Zc:wchar_t- -W3 -Gy -Fdgenerated.pdb -wd4800 -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1 Configure arguments --enable-application=browser --enable-update-channel=nightly --enable-update-packaging --enable-jemalloc --enable-tests

what graphics, driver, etc. ? check "Graphics" in about:support.

(In reply to comment #2) > what graphics, driver, etc. ? > check "Graphics" in about:support. There you go: Graphics: Adapter Description Intel(R) HD Graphics Vendor ID8086 Device ID0046 Adapter RAM Unknown Adapter Drivers igdumd64 igd10umd64 igdumdx32 igd10umd32Driver Version8.15.10.2202 Driver Date 8-25-2010 Direct2D Enabled true DirectWrite Enabled true GPU Accelerated Windows 1/1 Direct3D 10

I can confirm this on 20101004 build. Graphic information: Adapter Description NVIDIA GeForce 8400 GS Vendor ID10de Device ID06e4 Adapter RAM 512 Adapter Drivers nvd3dum nvwgf2um,nvwgf2um Driver Version 8.17.12.6063 Driver Date 9-10-2010 Direct2D Enabled true DirectWrite Enabled true GPU Accelerated Windows1/1 Direct3D 10

Ah yes, this is because D3D10 does not support hard clipping of your back buffer. This requires

Err, this requires the caption button area to be drawn transparent black whenever it is drawn.

Felipe, if you want to take this from Bas, I'm sure he'll be happy.

Okay, it took me a whiles to find this bug but after a little hunting I found the correct one. I starting seeing this issue recently too. I even had one nightly where as someone reported in another bug for Beta 6 that the area turned all black. Adapter DescriptionNVIDIA GeForce 8400M GS Vendor ID10de Device ID0427 Adapter RAM128 Adapter Driversnvd3dum nvwgf2um,nvwgf2um Driver Version8.17.12.6063 Driver Date9-10-2010 Direct2D Enabledtrue DirectWrite Enabledtrue GPU Accelerated Windows1/1 Direct3D 10 I'm on Vista Home Premium with a This is a wierd one. At first, I thought it was due to my taskbar being on top. When I dragged it back down to the bottom, it seemed the problem was resolved, then it started reoccurring. I wanted to see if it was something with certain personas or all personas. I was initially going to report that when you previewed certain personas, the buttons reappeared and others they would disappear. But then they ones that looked they worked started acting the same way. The only thing that appears to clear this up is using the default pre7 appearance. I discovered that it appears that the buttons are being rendered behind the browser window. In the attached screenshot, you can see my mouse arrow is almost to the top. While there are no buttons, Windows sees the missing buttons. You can see there is something being partially highlighted just over the top of the browser window. You can even see the popup description. If I hover where the "x" should be, the highlight is in red. You can actually click in this area and the buttons will work (but not display). It does seem like I have to move the arrow higher though then when it works correctly. Just rehashing from earlier in this comment, on some (but not all) personas that you preview, the buttons will render correctly the first time and maybe even for a few subsequent times. You can just test this on the main persona page with the popular personas shown.

As noted in my comments, it appears the buttons ARE there as Windows can see them (note the highlight appearing above the edge when Windows is not maximized or all the way to top), but they appeared to be covered over by personas.

Felipe is working on this, implementing the 'correct' solution for this problem hopefully.

Yeah I'm working on this bug, here's an update on the progress. The approach I'm taking is the following: The titlebar buttons have corresponding XUL elements which takes -moz-appearance for native rendering. Currently they don't paint anything, and what I'm making them to do is to clear the area where they are. This is unusual as no other widget do this. Note that we can't simply paint as transparent, we need to clear what's in there because the background image is painted below them. So on gfxWindowsNativeDrawing::PaintToContext if the widget set a new mNativeDrawingFlag representing to clear that area, I set the OPERATOR_CLEAR and fill the rectangle on the thebesContext (instead of grabbing the pattern from the temporary context and painting it). Vlad, what do you think of this approach? Do you think it's a good solution? Bas suggested that the solution should probably live in theme-related code, so that's what lead into this path. I'm mostly worried if in the future the mContext at the gfxWindowsNativeDrawing becomes no longer the final context where things are drawn, and its content is just painted over another surface (with something already drawn) where the transparent hole will be lost. Maybe that's not a worry though.

This is the current work in progress. There's a lot of hacked stuff that I'm cleaning up. And there are some problems yet to solve: - The XUL buttons are not precisely positioned where they should be because it currently uses metrics from the Aero classic theme instead of glass. - If you apply this patch and turn Personas on you'll see a black area instead of the buttons. That's because the background-color of the personas theme turns the surface to opaque mode. It's needed to clear the background-color or use an rgba color to see the patch working.

> - If you apply this patch and turn Personas on you'll see a black area instead > of the buttons. That's because the background-color of the personas theme turns > the surface to opaque mode. It's needed to clear the background-color or use an > rgba color to see the patch working. Anyone got any ideas on how to fix this? (short of a front-end hack to force an alpha color to be applied)

(In reply to comment #13) > > - If you apply this patch and turn Personas on you'll see a black area instead > > of the buttons. That's because the background-color of the personas theme turns > > the surface to opaque mode. It's needed to clear the background-color or use an > > rgba color to see the patch working. > > Anyone got any ideas on how to fix this? (short of a front-end hack to force an > alpha color to be applied) Hmm, that's indeed very annoying. We should probably not make the surface turn opaque when your clear element is used.

If you hadn't noticed this yet, there are some various oddities here with moving the mouse pointer through the titlebar zone vs toolbars and content area. The caption buttons will show quickly and hide. If you resize the window, the clip area shows up also then hides again.

I've noticed that too but I didn't comment because it is hard to specify where and when it occurs. I noticed before if you go over the Minefield bar and go horizontally across (without going out of that narrow height zone), the buttons would stay there. However, at least as of 1009 release I can't do that anymore. In fact, it is even worse trying to get the buttons to pop up at all. About the only thing I seem to be able to make them pop at all is clicking the Minefield button and then if they do appear, they disappear again.

> > > - If you apply this patch and turn Personas on you'll see a black area instead > > > of the buttons. That's because the background-color of the personas theme turns > > > the surface to opaque mode. It's needed to clear the background-color or use an > > > rgba color to see the patch working. > > > > Anyone got any ideas on how to fix this? (short of a front-end hack to force an > > alpha color to be applied) > So, I've been thinking about this, and I think that the right thing to do is to force the background to be not opaque when "-moz-appearance: -moz-win-glass" is set on the element. This would give us the right thing wanted without forcing any unecessary non-opaqueness. The way to do it seemed to be at the nsDisplaySolidColor::IsOpaque impl, but the problem is that the mAppearance on that frame is always set to 0, and the actual appearance value is found at the rootElementStyleFrame, which seems wrong to access from nsDisplaySolidColor. I'll try to see if it's possible to set some flag for that purpose during nsDisplayListBuilder creation or EnterPresShell.

I don't quite understand the situation here, but the solid color item probably gets it color from the root element style frame here http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5904 even though the mFrame of the display item is usually the root frame.

Jeff, just a straightforward patch to remove the temp workaround we added for d3d9, plus all the event.region stuff we added for the basic layers manager (With this patch we won't anymore create complex regions to exclude the caption buttons from the drawing area)

Move the DWM query to retrieve the caption buttons rect to nsUXThemeData, and get rid of UpdateCaptionButtonsClippingRect and the mCaptionButtonsRoundedRegion

This part perfoms the actual widget drawing that clears the widget area. This widget is a #titlebar-buttonbox <hbox> element on XUL positioned with the data from the previous patch

CSS changes needed to display the #titlebar-buttonbox element on Aero glass with personas, and -moz-box-align moved to #titlebar-content because #titlebar-buttonbox needs to have the correct height specified (and not stretch as is the default)

This forces the transparency for the nsDisplaySolidColor when -moz-win-glass is applied. Just adjusting the Layers::CONTENT_OPAQUE flag on PopThebesLayerData wasn't enough, so I followed tn's suggestion to set a flag on the presshell and use that during nsDisplaySolidColor creation

Vlad, this patch isn't strictly necessary to the bug, but I don't know if it's a good bonus or not worth the change to add a new flag and use it on gfxWindowsNativeDrawing. Asking feedback to see what's your opinion on it

hg diff -r qparent (without the optional patch) in case someone wants to try it.

Roc, if you wanna take a look for a sanity check on the approach, the main patches are part 3 and 5.

(In reply to comment #23) > This forces the transparency for the nsDisplaySolidColor when -moz-win-glass is > applied. > > Just adjusting the Layers::CONTENT_OPAQUE flag on PopThebesLayerData wasn't > enough, so I followed tn's suggestion to set a flag on the presshell and use > that during nsDisplaySolidColor creation This is not the right approach in general. Someone might layer their own content over the window --- e.g. a regular element with an opaque background --- in such a way that we think the window is opaque and this bug reappears again. Whhat went wrong with the Layers::CONTENT_OPAQUE flag?

Even if I never set the Layers::CONTENT_OPAQUE flag, the buttons still appear as black. I'm guessing that there are other operations (like the visibleRegion calculations) that depend on the return value of IsOpaque

Hmm. Do you need to modify nsWindow::UpdatePossiblyTransparentRegion to make sure that the Aero Glass margins extend down to cover the buttons?

Comment on attachment 483046 [details] [diff] [review] Optional patch - avoid double pass for this element I actually think we should take this patch, no need to do alpha recovery unnecessarily -- but: case RENDER_STATE_ALPHA_RECOVERY_BLACK_DONE: mRenderState = RENDER_STATE_ALPHA_RECOVERY_WHITE; - return PR_TRUE; + return !(mNativeDrawFlags & DONT_NEED_ALPHA_RECOVERY); should just set mRenderState to NATIVE_DRAWING_DONE and return PR_FALSE, instead of setting it to _WHITE and return FALSE to keep the state consistent.

Comment on attachment 483045 [details] [diff] [review] Part 5 - Force transparency for -moz-win-glass See comment #27

Comment on attachment 483042 [details] [diff] [review] Part 2 - System metrics + aResult->width = nsUXThemeData::sCommandButtons[CMDBUTTONIDX_BUTTONBOX].cx - 3; Might as well do that when you populate the values in sCommandButtons. You're already subtracting 1 from cy there.

Hey vlad, sorry for asking a re-review on this patch, but I didn't know about this possible issue before. This new patch ensures that the drawing never takes the fallback path to cairo_d2d_acquire_dest_image, by resetting the clip and drawing each rectangle one at a time, so it never draws to a complex region.

(In reply to comment #29) > Hmm. Do you need to modify nsWindow::UpdatePossiblyTransparentRegion to make > sure that the Aero Glass margins extend down to cover the buttons? Hmm no, there's already code for that. See http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#2460

Ok I got what the problem was, there's another place that also sets the Layer::CONTENT_OPAQUE flag. I was changing only PopThebesLayerData, but there is BuildContainerLayerFor. Both need to be handled. Patch incoming

Now correctly setting the Layer::CONTENT_OPAQUE flags using an outparam from IsOpaque. I think I still need that presshell dance to pass the info from the RootStyleFrame down to the frame that will hold the background. Or would there be a better way to mark the layer from PresShell::UpdateCanvasBackground and not even need the outparam and other changes?

+ * True if this layer is meant to hold a transparent background, + * even if the entire region is opaque. + */ + PRPackedBool mTransparentBackground; Let's call it mForceTransparentSurface and make the comment something like * Set if the layer should be treated as transparent, even if its entire * area is covered by opaque display items. For example, this needs to * be set if something is going to "punch holes" in the layer by clearing * part of its surface. + virtual PRBool IsOpaque(nsDisplayListBuilder* aBuilder, + PRBool* aOutTransparentBackground = nsnull) + { + return PR_FALSE; + } This needs to set *aOutTransparentBackground if aOutTransparentBackground is non-null. Also the parameter should probably be called aForceTransparentSurface or something like that. I don't think you need the changes to nsDisplaySolidColor or PresShell. We should have an nsDisplayBackground with -moz-win-glass appearance; that should be able to pass true for the out parameter.

(In reply to comment #37) > + virtual PRBool IsOpaque(nsDisplayListBuilder* aBuilder, > + PRBool* aOutTransparentBackground = nsnull) > + { > + return PR_FALSE; > + } > > This needs to set *aOutTransparentBackground if aOutTransparentBackground is > non-null. Also the parameter should probably be called aForceTransparentSurface > or something like that. Ok. Does this mean I should do this to every implementation of IsOpaque? > > I don't think you need the changes to nsDisplaySolidColor or PresShell. We > should have an nsDisplayBackground with -moz-win-glass appearance; that should > be able to pass true for the out parameter. So I looked more into this. There's indeed an nsDisplayBackground created with a -moz-win-glass frame, but the IsOpaque of this item is never called

(In reply to comment #38) > Ok. Does this mean I should do this to every implementation of IsOpaque? Yeah, I guess so. > > I don't think you need the changes to nsDisplaySolidColor or PresShell. We > > should have an nsDisplayBackground with -moz-win-glass appearance; that should > > be able to pass true for the out parameter. > > So I looked more into this. There's indeed an nsDisplayBackground created with > a -moz-win-glass frame, but the IsOpaque of this item is never called Hmm, odd. Does it get processed by ProcessDisplayItems? What happens to it?

Comment on attachment 483044 [details] [diff] [review] Part 4 - CSS changes >+ #main-window[sizemode="maximized"] #titlebar-buttonbox { >+ margin-right: 2px; >+ } What's the deal with this?

(In reply to comment #40) > Comment on attachment 483044 [details] [diff] [review] > Part 4 - CSS changes > > >+ #main-window[sizemode="maximized"] #titlebar-buttonbox { > >+ margin-right: 2px; > >+ } > > What's the deal with this? That's the visual gap between the close button and the screen edge that can be seen for maximized windows

(In reply to comment #41) > (In reply to comment #40) > > Comment on attachment 483044 [details] [diff] [review] [details] > > Part 4 - CSS changes > > > > >+ #main-window[sizemode="maximized"] #titlebar-buttonbox { > > >+ margin-right: 2px; > > >+ } > > > > What's the deal with this? > > That's the visual gap between the close button and the screen edge that can be > seen for maximized windows So the close button isn't supposed to be the click target when moving the mouse to the screen edge?

Oh ti will still be the click target. Mouse events on that area of the screen is specially handled by Windows and is always targeted for the buttons, independent on what the app is drawing there.

Ok. How about RTL? Don't you want -moz-margin-end for that?

True, it should be -moz-margin-end instead

(In reply to comment #39) > > So I looked more into this. There's indeed an nsDisplayBackground created with > > a -moz-win-glass frame, but the IsOpaque of this item is never called > > Hmm, odd. Does it get processed by ProcessDisplayItems? What happens to it? It is processed by ProcessDisplayItems. What happens is that on ComputeVisibilityForSublist, item->ComputeVisibility returns false because this item has propagated away its background. @ nsDisplayBackground::ComputeVisbility: // Return false if the background was propagated away from this // frame. We don't want this display item to show up and confuse // anything. nsStyleContext* bgSC; return mIsThemed || nsCSSRendering::FindBackground(mFrame->PresContext(), mFrame, &bgSC); Could I return true there instead in this situation (given that comment above)?

why isn't mIsThemed true? Seems like it should be

This implements the forceTransparentSurface flags to control layers' opaqueness, and makes the theme support -moz-win-glass and -moz-win-borderless glass. Couple of notes: - the change in nsLayoutUtils is needed because as IsThemed() now returns true the eTransparency{Borderless}Glass transparency mode would never be set on the widget - the big chunk on DrawWidgetBackground is just moving the code to bail out of drawing upper in the function to avoid calling some unecessary functions (GetThemePartAndState, ScaleInverse and addRef'ing the DC)

+ mForceTransparentSurface = (disp->mAppearance == NS_THEME_WIN_BORDERLESS_GLASS || + disp->mAppearance == NS_THEME_WIN_GLASS); Instead of a variable here, why not just check mAppearance in nsDisplayBackground::IsOpaque?

(In reply to comment #49) > + mForceTransparentSurface = (disp->mAppearance == > NS_THEME_WIN_BORDERLESS_GLASS || > + disp->mAppearance == NS_THEME_WIN_GLASS); > > Instead of a variable here, why not just check mAppearance in > nsDisplayBackground::IsOpaque? No particular reason. As mIsThemed is cached I just followed the pattern and added this variable to avoid calling GetStyleDisplay() every time in IsOpaque.

(In reply to comment #50) > (In reply to comment #49) > > + mForceTransparentSurface = (disp->mAppearance == > > NS_THEME_WIN_BORDERLESS_GLASS || > > + disp->mAppearance == NS_THEME_WIN_GLASS); > > > > Instead of a variable here, why not just check mAppearance in > > nsDisplayBackground::IsOpaque? > > No particular reason. As mIsThemed is cached I just followed the pattern and > added this variable to avoid calling GetStyleDisplay() every time in IsOpaque. It's simpler to not cache, so let's not cache. IsOpaque is not called very often and checking the display type is relatively cheap.

This one doesn't cache mAppearance on nsDisplayBackground

+ if (aForceTransparentSurface) { + const nsStyleDisplay* disp = mFrame->GetStyleDisplay(); + *aForceTransparentSurface = disp->mAppearance == NS_THEME_WIN_BORDERLESS_GLASS || + disp->mAppearance == NS_THEME_WIN_GLASS; + } This should check mIsThemed as well. In fact it's probably best to set it to PR_FALSE first and then move this code to inside the if (mIsThemed) block.

Like so. To set it to PR_FALSE first it'll check the outparam pointer twice. Or I could move that after the if (mIsThemed) block

Comment on attachment 484210 [details] [diff] [review] Part 5 - Force transparency for -moz-win-glass - v5 This is fine.

Comment on attachment 483544 [details] [diff] [review] Part 3 - Widget drawing v2 + ctx->NewPath(); + ctx->Rectangle(buttonbox1, PR_TRUE); + ctx->Fill(); + + ctx->NewPath(); + ctx->Rectangle(buttonbox2, PR_TRUE); + ctx->Fill(); + + ctx->NewPath(); + ctx->Rectangle(buttonbox3, PR_TRUE); + ctx->Fill(); How about just ctx->NewPath(); ctx->Rectangle(...); ctx->Rectangle(...); ctx->Rectangle(...); ctx->Fill(); ?I assume we try to ensure that these buttons are drawn last, topmost in the window?

Ignore the last comment, it doesn't really matter. If people want to draw over them, they can.

Yes correct, they can be drawn over if other element is positioned over them. The reason why there's a NewPath() before every rectangle is to avoid taking the fallback path to cairo_d2d_acquire_dest. If we don't do that the complex clip with operator_clear takes that path.

Comment on attachment 483544 [details] [diff] [review] Part 3 - Widget drawing v2 OK, please add a comment explaining that!

Comment on attachment 483044 [details] [diff] [review] Part 4 - CSS changes >+ #titlebar-buttonbox:not(:-moz-lwtheme), >+ #titlebar-buttonbox > * { > display: none; > } Can you add a titlebar-button class and use that instead of #titlebar-buttonbox > *? >+ #main-window[sizemode="maximized"] #titlebar-buttonbox { >+ margin-right: 2px; Need to use -moz-margin-start here per earlier discussion.

(In reply to comment #60) > Comment on attachment 483044 [details] [diff] [review] > Part 4 - CSS changes > > >+ #titlebar-buttonbox:not(:-moz-lwtheme), > >+ #titlebar-buttonbox > * { > > display: none; > > } > > Can you add a titlebar-button class and use that instead of #titlebar-buttonbox > *? Done. > >+ #main-window[sizemode="maximized"] #titlebar-buttonbox { > >+ margin-right: 2px; > > Need to use -moz-margin-start here per earlier discussion. Done. (I assume you meant -moz-margin-end) (In reply to comment #59) > Comment on attachment 483544 [details] [diff] [review] > Part 3 - Widget drawing v2 > > OK, please add a comment explaining that! Comment added

http://hg.mozilla.org/mozilla-central/rev/be51d3e6905b http://hg.mozilla.org/mozilla-central/rev/f1dce6bc53eb http://hg.mozilla.org/mozilla-central/rev/91073aa8ef72 http://hg.mozilla.org/mozilla-central/rev/d73eaa05f59d http://hg.mozilla.org/mozilla-central/rev/c9d327fec92e

I realized this morning that this probably just regressed bug 590468. Before we pretended we didn't support -moz-win-glass for drawing, so nothing was drawn. Can you get a layer tree dump (gDumpPaintList=1) for a basic browser window so we can check if we're allocating large multiple overlapping ThebesLayers for the content area?

(In reply to comment #63) > so nothing was drawn. I mean, the area of the element wasn't added to the ThebesLayer.

Painting --- retained layer tree: D3D9LayerManager (0x851d788) D3D9ContainerLayer (0x37757c8) [visible=< (x=0, y=0, w=722, h=600); >] [metric s={ viewport=(w=722, h=600) viewportScroll=(x=0, y=0) displayport=(x=0, y=0, w=0 , h=0) }] D3D9ThebesLayer (0x86f5fe8) [visible=< (x=0, y=0, w=722, h=96); >] [valid=< (x=0, y=0, w=722, h=96); >] D3D9ContainerLayer (0x37758e0) [clip=(x=0, y=96, w=722, h=504)] [visible=< ( x=0, y=96, w=722, h=504); >] [opaqueContent] D3D9ThebesLayer (0x86f6258) [clip=(x=0, y=0, w=0, h=0)] [transform=[ 1 0; 0 1; 0 96; ]] D3D9ColorLayer (0x37759f8) [clip=(x=0, y=96, w=722, h=504)] [transform=[ 1 0; 0 1; 0 96; ]] [visible=< (x=0, y=0, w=722, h=504); >] [opaqueContent] [noTex t] [noTextOverTransparent] [color=rgba(255, 255, 255, 1)]

Yeah, that does look OK. Phew!

regression. http://forums.mozillazine.org/viewtopic.php?p=10037249#p10037249 should file a new bug ?

(In reply to comment #67) > regression. > http://forums.mozillazine.org/viewtopic.php?p=10037249#p10037249 > > > should file a new bug ? Yep, looks like it's wrongly using the patch for Basic too!

(In reply to comment #68) > (In reply to comment #67) > > regression. > > http://forums.mozillazine.org/viewtopic.php?p=10037249#p10037249 > > > > > > should file a new bug ? > > Yep, looks like it's wrongly using the patch for Basic too! filed, bug 605806

Thunderbird issue: It is 01/29/13 and I did the latest update to 17.0.2 last week and now the X (close), minimize, and restore on the upper right along with the bar they sit within is not there. I must click on "File" then "Exit" to close. This thread is from 2010, so it is apparent the bug is still present in 2013. No problem until the update to the 17.0.2 version last week.

Sounds like it is not this bug then.