Sheets should be semi-transparent

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: stefanh, Assigned: stefanh)

Tracking

Trunk
mozilla41
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 wontfix, firefox30 wontfix, firefox41 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

6 years ago
(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 :-)
Assignee

Comment 1

6 years ago
Posted patch wip (obsolete) — Splinter Review
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)
Assignee

Comment 2

6 years ago
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.
Flags: needinfo?(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?
Flags: needinfo?(mstange)
Assignee

Comment 4

6 years ago
(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.
Assignee

Comment 6

6 years ago
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?
Assignee

Comment 8

6 years ago
(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.
Assignee

Comment 10

6 years ago
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.
Assignee

Updated

6 years ago
Assignee: nobody → stefanh
Assignee

Comment 11

6 years ago
(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.
Assignee

Comment 12

6 years ago
Posted patch 889085.1.diff (obsolete) — Splinter Review
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).
Attachment #769841 - Attachment is obsolete: true
Attachment #8356664 - Flags: review?(mstange)
With this patch, the background-color rule in global.css can stay.
Attachment #8359202 - Flags: review?(roc)
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!
Attachment #8356664 - Flags: review?(mstange) → review+
Assignee

Comment 15

5 years ago
(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.
Assignee

Comment 19

5 years ago
Landing this once we have settled a solution in bug 957209.
https://hg.mozilla.org/mozilla-central/rev/dddfd63f1414
https://hg.mozilla.org/mozilla-central/rev/f8c14bd80676
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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.
Assignee

Comment 23

5 years ago
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.
Depends on: 987783
Requesting backout due to bug 987783
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Posted patch Diff for backout (obsolete) — Splinter Review
I couldn't just back these out because IsWindowSheet was changed in the meantime.

So this is just a patch to backout both patches.
Attachment #8400624 - Flags: review?(roc)
Attachment #8400624 - Flags: review?(mstange)
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
Attachment #8400624 - Flags: review?(mstange)
Attachment #8400624 - Flags: approval-mozilla-beta?
Attachment #8400624 - Flags: approval-mozilla-aurora?
Attachment #8400624 - Flags: approval-mozilla-beta?
Attachment #8400624 - Flags: approval-mozilla-beta+
Attachment #8400624 - Flags: approval-mozilla-aurora?
Attachment #8400624 - Flags: approval-mozilla-aurora+
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
Thanks, Mike, for taking care of this.
Assignee

Comment 31

4 years ago
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).
Status: REOPENED → NEW
Assignee

Comment 32

4 years ago
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...
Attachment #8356664 - Attachment is obsolete: true
Attachment #8359202 - Attachment is obsolete: true
Attachment #8400624 - Attachment is obsolete: true
Attachment #8606996 - Flags: review?(mstange)
Assignee

Updated

4 years ago
Status: NEW → ASSIGNED
Assignee

Comment 34

4 years ago
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.)
Attachment #8606996 - Flags: review?(mstange) → review+
Assignee

Comment 36

4 years ago
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?
Attachment #8606996 - Flags: review?(roc)
Assignee

Comment 37

4 years ago
(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.
Assignee

Comment 40

4 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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.
Attachment #8400624 - Flags: approval-mozilla-beta+
Attachment #8400624 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.