Closed Bug 1162649 Opened 9 years ago Closed 9 years ago

Transparent `accentcolor` on lightweight theme causes window drop shadow to disappear on 10.9

Categories

(Core :: Widget: Cocoa, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

(Whiteboard: [mentor=mstange][lang=c++])

Attachments

(2 files, 3 obsolete files)

In Bug 1162490 we had to preprocess out the `accentcolor: transparent` value for the devedition theme OSX, since it was causing the window's drop shadow to disappear in 10.9.

What this property does, most relevantly, is set the background color of the root element in browser.xul: https://dxr.mozilla.org/mozilla-central/search?q=regexp%3A%28.*%29%3D%28.*%29\.accentcolor&case=false.

I'm assuming that any lightweight theme using this value will have the same problem.
Patch that reverts the workaround from Bug 1162490 and should retrigger the issue.

STR:
Apply the patch (if needed - the workaround has only landed on fx-team at the moment)
Go to about:addons -> Appearance -> Developer Edition

From what I understand, this makes the drop shadow disappear in 10.9
Flags: needinfo?(mstange)
We can fix this by removing support for transparent top level windows. I don't think anybody has ever used that capability intentionally.
So in nsCocoaWindow.mm, for windows that aren't of type popup, we should never call setOpaque:NO or setHasShadow:NO, and the window's background color should always be white (which will be blocked out by the Gecko content on top).

If somebody wants to fix this, please ask me for more information.
Flags: needinfo?(mstange)
Whiteboard: [mentor=mstange][lang=c++]
Attached patch transparent-windows.patch (obsolete) — Splinter Review
(In reply to Markus Stange [:mstange] from comment #2)
> We can fix this by removing support for transparent top level windows. I
> don't think anybody has ever used that capability intentionally.
> So in nsCocoaWindow.mm, for windows that aren't of type popup, we should
> never call setOpaque:NO or setHasShadow:NO, and the window's background
> color should always be white (which will be blocked out by the Gecko content
> on top).
> 
> If somebody wants to fix this, please ask me for more information.

I can try, although I don't know anything about this code - can you point me in the right direction?  I also don't have a 10.9 machine to test on (and I haven't been able to reproduce on 10.10).

I see a comment about nsChildView::SetTransparencyMode being used for non-popup windows, so the change in nsCocoaWindow::SetTransparencyMode is maybe in the wrong place.
Attachment #8603034 - Flags: feedback?(mstange)
Comment on attachment 8603034 [details] [diff] [review]
transparent-windows.patch

Review of attachment 8603034 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good! nsChildView::SetTransparencyMode just calls through to nsCocoaWindow::SetTransparencyMode, so this is in the right place.

Let's also add a comment that says that we only support transparent / shadowless windows on popup windows.
Attachment #8603034 - Flags: feedback?(mstange) → review+
Attached patch transparent-windows.patch (obsolete) — Splinter Review
Added comments.  I have an ongoing try push that also has the accentcolor included so that I can ask someone on 10.9 to test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27de4f82c0ff
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8603055 - Flags: review+
I actually would have assumed that as a platform feature, this (ie transparent windows) would be kind of useful? Especially if you want to have non-square floating widgets of some kind...
Comment on attachment 8603055 [details] [diff] [review]
transparent-windows.patch

I think you uploaded the wrong paatch since there's no comment added here :-)

Markus, there's a comment in nsCocoaWindow.mm just above void nsCocoaWindow::SetTransparencyMode(nsTransparencyMode aMode) that says:
1020 // This is called from nsMenuPopupFrame when making a popup transparent.
1021 // For other window types, nsChildView::SetTransparencyMode is used.
1022 void nsCocoaWindow::SetTransparencyMode(nsTransparencyMode aMode)

Is that comment wrong (or am I lost here?)? :-)
Attached patch transparent-windows.patch (obsolete) — Splinter Review
Updated to patch that actually has the comments
Attachment #8603034 - Attachment is obsolete: true
Attachment #8603055 - Attachment is obsolete: true
Attachment #8603329 - Flags: review+
(In reply to :Gijs Kruitbosch from comment #6)
> I actually would have assumed that as a platform feature, this (ie
> transparent windows) would be kind of useful? Especially if you want to have
> non-square floating widgets of some kind...

(In reply to Stefan [:stefanh] from comment #7)
> Markus, there's a comment in nsCocoaWindow.mm just above void
> nsCocoaWindow::SetTransparencyMode(nsTransparencyMode aMode) that says:
> 1020 // This is called from nsMenuPopupFrame when making a popup transparent.
> 1021 // For other window types, nsChildView::SetTransparencyMode is used.
> 1022 void nsCocoaWindow::SetTransparencyMode(nsTransparencyMode aMode)
> 
> Is that comment wrong (or am I lost here?)? :-)

ni? for the previous two comments
Flags: needinfo?(mstange)
(In reply to :Gijs Kruitbosch from comment #6)
> I actually would have assumed that as a platform feature, this (ie
> transparent windows) would be kind of useful? Especially if you want to have
> non-square floating widgets of some kind...

I would have thought so too, but I've never seen it used for non-popup windows. Floating widgets are usually panels, and we still support transparency for those.

(In reply to Stefan [:stefanh] from comment #7)
> Markus, there's a comment in nsCocoaWindow.mm just above void
> nsCocoaWindow::SetTransparencyMode(nsTransparencyMode aMode) that says:
> 1020 // This is called from nsMenuPopupFrame when making a popup transparent.
> 1021 // For other window types, nsChildView::SetTransparencyMode is used.
> 1022 void nsCocoaWindow::SetTransparencyMode(nsTransparencyMode aMode)
> 
> Is that comment wrong (or am I lost here?)? :-)

It's definitely misleading. Let's change it to
// This is called from nsMenuPopupFrame when making a popup transparent, or
// from nsChildView::SetTransparencyMode for other window types.
Flags: needinfo?(mstange)
Updated comment for nsCocoaWindow::SetTransparencyMode
Attachment #8603329 - Attachment is obsolete: true
Attachment #8603385 - Flags: review+
Markus was able to check this out on a 10.9 and confirm it was showing the drop shadow with Dev Ed theme applied.  I did a new push to try since the last one looked like is had some probably unrelated orange: https://treeherder.mozilla.org/#/jobs?repo=try&revision=878646b360eb
https://hg.mozilla.org/mozilla-central/rev/790507f83bf3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.