889085 - Sheets should be semi-transparent

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Stefan [:stefanh]]

(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 :-)

Comment 1

[Stefan [:stefanh]]

Attachment: wip

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)

Comment 2

[Stefan [:stefanh]]

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.

Comment 3

[Markus Stange [:mstange]]

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?

Comment 4

[Stefan [:stefanh]]

(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

Comment 5

[Markus Stange [:mstange]]

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.

Comment 6

[Stefan [:stefanh]]

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?

Comment 7

[Markus Stange [:mstange]]

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?

Comment 8

[Stefan [:stefanh]]

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

Comment 9

[Markus Stange [:mstange]]

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.

Comment 10

[Stefan [:stefanh]]

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.

Comment 11

[Stefan [:stefanh]]

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

Comment 12

[Stefan [:stefanh]]

Attachment: 889085.1.diff

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

Comment 13

[Markus Stange [:mstange]]

Attachment: don't draw CSS background color for themed root frames

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

Comment 14

[Markus Stange [:mstange]]

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!

Comment 15

[Stefan [:stefanh]]

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

Comment 16

[Markus Stange [:mstange]]

That would be great.

Comment 17

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

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?

Comment 18

[Markus Stange [:mstange]]

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.

Comment 19

[Stefan [:stefanh]]

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

Comment 20

[Stefan [:stefanh]]

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

Comment 21

[Phil Ringnalda (:philor)]

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

Comment 22

[Mike Kaply [:mkaply]]

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.

Comment 23

[Stefan [:stefanh]]

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

Comment 24

[Mike Kaply [:mkaply]]

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

If you do that, the min max controls in the upper turn black and don't work.

Comment 25

[Mike Kaply [:mkaply]]

Requesting backout due to bug 987783

Comment 26

[Mike Kaply [:mkaply]]

Attachment: Diff for backout

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 27

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d5baed67b88

Comment 28

[Mike Kaply [:mkaply]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Markus Stange [:mstange]]

Thanks, Mike, for taking care of this.

Comment 31

[Stefan [:stefanh]]

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

Comment 32

[Stefan [:stefanh]]

Attachment: Now also with vibrancy

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

Comment 33

[Stefan [:stefanh]]

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/[e-mail]-95236f44fd59

Comment 34

[Stefan [:stefanh]]

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 35

[Markus Stange [:mstange]]

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 36

[Stefan [:stefanh]]

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?

Comment 37

[Stefan [:stefanh]]

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

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7c9a6e1394e

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff31a31f9203

Comment 40

[Stefan [:stefanh]]

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

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 42

[Ryan VanderMeulen [:RyanVM]]

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.