(Steven and I talked about this for a couple of months ago) Our sheets doesn't have that nice, semi-transparent background as the "real" native ones(Mac OS X sheets have been semi-transparent since 10.5/10.6). Afaik all our sheets are xul dialogs and we style them with '-moz-appearance: dialog'. I think the right approach would be to use nsNativeTheme to style sheets in the same way as we style dialogs. The problem is of course that it's hard for a consumer to know when a dialog becomes a "sheet" and we only want 'eWindowType_sheet' to be styled as sheet. So, I don't think '-moz-appearance: -moz-mac-sheet' will work here. I've been playing a bit with an approach that checks whether an opened window, styled with NS_THEME_DIALOG, is a sheet and then (if it is a sheet) uses a HITheme constant to paint a semi-transparent background. It works, except that I need to pretend that all dialogs are transparent... Markus - this is really your area and I'm sure you have better ideas :-)

Here's what I've been playing with. The nsAppShellService.cpp changes is to make sure that we don't end up with a semi-transparent sheet opened from the hidden window (to see such a sheet, open Thunderbird, close all windows and open a contact)

I almost forgot about this. Markus, I think you're the man to comment on this approach so I put this on your list - we talked about this for a while ago.

Whoa, sorry for the delay here. The approach looks good. (In reply to Stefan [:stefanh] from comment #1) > (to see such a sheet, open Thunderbird, close all windows and > open a contact) How do I open a contact with no open windows?

(In reply to Markus Stange [:mstange] from comment #3) > How do I open a contact with no open windows? 1. Launch Thunderbird 2. Close all open windows 3. File —> New —> Address Book Contact

Oh, that looks interesting! Can you open a new bug for the nsAppShellService.cpp part of the patch and ask smichaud for review? I'll give you r+ for the nsNativeThemeCocoa.mm part.

Cool, thanks - I'll open a new bug and put up an altered patch here tomorrow and ask you for review :-) You don't see any problem with returning eTransparent in GetWidgetTransparency for all dialogs, then?

Oh, right, I hadn't thought that part through completely. Can you change the patch to only return eTransparent for sheets, just to be safe? Or does that not work for some reason?

(In reply to Markus Stange [:mstange] from comment #7) > Oh, right, I hadn't thought that part through completely. Can you change the > patch to only return eTransparent for sheets, just to be safe? Or does that > not work for some reason? I tried to do that in GetWidgetTransparency, but it didn't worked. That was of course for 4 months ago :-). But I remember feeling unsure of how GetWidgetTransparency was getting called since I always ended up with no transparency.

Oh, it probably gets called before the sheet is shown. And there's this comment in nsCocoaWindow.mm: 87 // A note on testing to see if your object is a sheet... 88 // |mWindowType == eWindowType_sheet| is true if your gecko nsIWidget is a sheet 89 // widget - whether or not the sheet is showing. |[mWindow isSheet]| will return 90 // true *only when the sheet is actually showing*. Choose your test wisely.

Yeah, I've seen that. Hmm, I have to test this again... I recall playing with both variants without success, but I could be wrong.

(In reply to Markus Stange [:mstange] from comment #5) > Can you open a new bug for the nsAppShellService.cpp part of the patch and > ask smichaud for review? Filed bug 957209.

So, I had to remove the background-color from global.css in order to get the transparency (setting "-moz-appearance: none" will then have a certain side-effect).

With this patch, the background-color rule in global.css can stay.

Comment on attachment 8356664 [details] [diff] [review] 889085.1.diff >diff --git a/widget/cocoa/nsNativeThemeCocoa.mm b/widget/cocoa/nsNativeThemeCocoa.mm >+bool >+nsNativeThemeCocoa::IsWindowSheet(nsIFrame* aFrame) >+{ >+ NSWindow* win = NativeWindowForFrame(aFrame); >+ id winDelegate = [win delegate]; >+ nsIWidget* widget = [(WindowDelegate *)winDelegate geckoWidget]; >+ nsWindowType windowType; Let's move this variable down after the if. >+ if (!widget) { >+ return false; >+ } >+ widget->GetWindowType(windowType); >+ return (windowType == eWindowType_sheet); >+} r+ with the global.css change removed. Thanks!

(In reply to Markus Stange [:mstange] from comment #13) > Created attachment 8359202 [details] [diff] [review] > don't draw CSS background color for themed root frames > > With this patch, the background-color rule in global.css can stay. Awesome, thanks :-). I can land your patch at the same time as mine, if you want.

That would be great.

Comment on attachment 8359202 [details] [diff] [review] don't draw CSS background color for themed root frames Review of attachment 8359202 [details] [diff] [review]: ----------------------------------------------------------------- It would be more efficient and maybe no more complex to prevent the creation of the display item that renders the background color. Did you try that?

PresShell::AddCanvasBackgroundColorItem already has an early exit for the case that mCanvasBackgroundColor is completely transparent, so this patch already causes no display item to be created.

Landing this once we have settled a solution in bug 957209.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dddfd63f1414 https://hg.mozilla.org/integration/mozilla-inbound/rev/f8c14bd80676

https://hg.mozilla.org/mozilla-central/rev/dddfd63f1414 https://hg.mozilla.org/mozilla-central/rev/f8c14bd80676

I'm tracking a regression in bug 987783 and this seems a likely candidate. You can no longer set the background color of the main window on windows.

Even with "window { -moz-appearance: none; }"?

> Even with "window { -moz-appearance: none; }"? If you do that, the min max controls in the upper turn black and don't work.

Requesting backout due to bug 987783

I couldn't just back these out because IsWindowSheet was changed in the meantime. So this is just a patch to backout both patches.

Comment on attachment 8400624 [details] [diff] [review] Diff for backout [Approval Request Comment] Bug caused by (feature/regressing bug #): This bug caused bug 987783 User impact if declined: Personas/backgrounds missing background color. Background colors won't work on main window. Testing completed (on m-c, etc.): verified on mozilla-central Risk to taking this patch (and alternatives if risky): None. Backout of "nice to have" patch. String or IDL/UUID changes made by this patch: None

https://hg.mozilla.org/mozilla-central/rev/1d5baed67b88 https://hg.mozilla.org/releases/mozilla-aurora/rev/5baa22f9859f https://hg.mozilla.org/releases/mozilla-beta/rev/51e5b0ec21b3

Thanks, Mike, for taking care of this.

Lots of things have happened since I last looked at this and we can now achive the transparancy in a slightly different way (which doesn't affect win/nix).

I haven't been able to figure out how Apple do the sheet vibrancy (the lldb output I get in Safari seems to be from the toolbar/title bar), but I think this is the right way (looks the same to me). Also: @@ -3798,17 +3824,21 @@ nsNativeThemeCocoa::WidgetProvidesFontSm { switch (aWidgetType) { case NS_THEME_MAC_VIBRANCY_LIGHT: case NS_THEME_MAC_VIBRANCY_DARK: case NS_THEME_TOOLTIP: case NS_THEME_MENUPOPUP: case NS_THEME_MENUITEM: case NS_THEME_CHECKMENUITEM: + case NS_THEME_DIALOG: { + if (aWidgetType == NS_THEME_DIALOG && !IsWindowSheet(aFrame)) { + return false; + } I felt that this was the safest way since I don’t want to end up with [childView vibrancyFontSmoothingBackgroundColorForThemeGeometryType:type] when the type is eThemeGeometryTypeUnknown. That said, I don’t think we’ll ever hit that case - at least I haven’t been able to do it, but...

Oh, and I've tested this on 10.7.5, 10.9.5 and 10.10.3. Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95236f44fd59 Try builds: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/stefanh@inbox.com-95236f44fd59

I have now tested the patch on 10.6.8 and the results are a bit confusing: 1) The warning sheet you get when trying to close multiple tabs is not transparent. I know that at some time it used to be transparent on 10.6.8, but checking a recent nightly now reveals that it's not transparent anymore. 2) The network settings sheet (Advanced --> Network, click "Settings..." is transparent. My guess is that the difference is caused by 1) being an alert and 2) not being an alert. I don't think I'll be able sort out why not all sheets are transparent on 10.6.8, though (and I'm not sure it matters that much).

Comment on attachment 8606996 [details] [diff] [review] Now also with vibrancy Review of attachment 8606996 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay, this looks very good! Yeah, let's not worry about 10.6. As long as things don't look completely wrong on 10.6, we shouldn't spend any energy on it. ::: widget/cocoa/nsChildView.mm @@ +2510,3 @@ > > MakeRegionsNonOverlapping(vibrantLightRegion, vibrantDarkRegion, menuRegion, > + tooltipRegion, highlightedMenuItemRegion, sheetRegion); I'd put sheetRegion at the start of the list. That way, if somebody wants to have dark vibrancy inside a sheet, they can. (Because the dark vibrancy will override the sheet vibrancy.)

Comment on attachment 8606996 [details] [diff] [review] Now also with vibrancy Thanks Markus. roc, mind take a look at the gfx/src/nsITheme.h and layout/base/nsDisplayList.cpp parts?

(In reply to Markus Stange [:mstange] from comment #35) > > MakeRegionsNonOverlapping(vibrantLightRegion, vibrantDarkRegion, menuRegion, > > + tooltipRegion, highlightedMenuItemRegion, sheetRegion); > > I'd put sheetRegion at the start of the list. That way, if somebody wants to > have dark vibrancy inside a sheet, they can. (Because the dark vibrancy will > override the sheet vibrancy.) Will do that. Just to be consistent, I'll also put sheetRegion first in the nsIntRegion inits above.

The last push was a result of me noticing that I had added an extra comma in VibrancyManager.h (now removed).

https://hg.mozilla.org/mozilla-central/rev/a7c9a6e1394e https://hg.mozilla.org/mozilla-central/rev/ff31a31f9203

Comment on attachment 8400624 [details] [diff] [review] Diff for backout Removing these old approvals so this bug doesn't turn up in the "needs uplift" bug queries.