Implement -moz-window-shadow functionality on Mac OS X

RESOLVED FIXED in mozilla1.9.1b2

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

Trunk
mozilla1.9.1b2
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 334161 [details] [diff] [review]
fix v0.1

Bug 450939 implements the parsing for -moz-window-shadow.
This bug's purpose is to make it work on Mac OS X.
Attachment #334161 - Flags: superreview?(roc)
Attachment #334161 - Flags: review?(hwaara)
(Assignee)

Comment 1

9 years ago
Created attachment 334162 [details] [diff] [review]
No window shadow for the autoscroller [checked in]

This patch turns the window shadow off for the autoscroller.
I didn't turn off the shadow for the identity popup, the bookmarks edit panel and the Ctrl-Tab panel because I think it looks good there.
Attachment #334162 - Flags: review?(mconnor)
(Assignee)

Comment 2

9 years ago
Created attachment 334164 [details]
Screenshot: shadow for identity panel
Attachment #334164 - Flags: ui-review?(beltzner)
(Assignee)

Comment 3

9 years ago
Created attachment 334165 [details]
Screenshot: shadow for "Edit bookmark" panel
Attachment #334165 - Flags: ui-review?(beltzner)
(Assignee)

Comment 4

9 years ago
Created attachment 334166 [details]
Screenshot: shadow for Ctrl-Tab panel
Attachment #334166 - Flags: ui-review?(beltzner)

Comment 5

9 years ago
Comment on attachment 334161 [details] [diff] [review]
fix v0.1

When this bug is fixed, you might as well close bug 415443 too because there will be nothing left to do there.
Attachment #334161 - Flags: review?(hwaara) → review+
We'd rather avoid unnecessary dependence on nsStyleConsts from widget. How about defining shadow styles in nsIWidget and then referencing them from nsStyleConsts.h?
(Assignee)

Comment 7

9 years ago
Created attachment 334356 [details] [diff] [review]
fix v0.2: Move shadow style consts to nsIWidget, limit shadow invalidation frequency
Attachment #334161 - Attachment is obsolete: true
Attachment #334356 - Flags: superreview?(roc)
Attachment #334161 - Flags: superreview?(roc)
Comment on attachment 334162 [details] [diff] [review]
No window shadow for the autoscroller [checked in]

r=mconnor
Attachment #334162 - Flags: review?(mconnor) → review+
Hmm. If the window changes twice in a short interval, but then stops changing, doesn't this mean that the shadow is invalidated after the first change, but it's never invalidated for the second change?
(Assignee)

Comment 10

9 years ago
Created attachment 334769 [details] [diff] [review]
fix v0.3 [checked in]

(In reply to comment #9)
> Hmm. If the window changes twice in a short interval, but then stops changing,
> doesn't this mean that the shadow is invalidated after the first change, but
> it's never invalidated for the second change?

Oops.
Attachment #334769 - Flags: superreview?(roc)
(Assignee)

Updated

9 years ago
Attachment #334356 - Attachment is obsolete: true
Attachment #334356 - Flags: superreview?(roc)
Comment on attachment 334769 [details] [diff] [review]
fix v0.3 [checked in]

+    [self performSelector:@selector(invalidateShadow)
+               withObject:nil
+               afterDelay:(sShadowInvalidationInterval - elapsed) / 1000.0];

I assume this automatically cancels if 'self' is destroyed. Or does it keep self alive, and if so, have you tested that nothing bad happens if a window closes while the invalidate is pending?
Attachment #334769 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 12

9 years ago
I haven't found a clear statement on that in the docs, but I can't imagine that it keeps 'self' somehow alive.

Moreover, performSelector:withObject:afterDelay is also used for normal window invalidations (in setNeedsPendingDisplay), so we'd probably be in big trouble if it had funny side-effects.
Comment on attachment 334165 [details]
Screenshot: shadow for "Edit bookmark" panel

These look fine to me, but faaborg actually should get the final call :)
Attachment #334165 - Flags: ui-review?(beltzner) → ui-review?(faaborg)
Attachment #334166 - Flags: ui-review?(beltzner) → ui-review?(faaborg)
Attachment #334164 - Flags: ui-review?(beltzner) → ui-review?(faaborg)
I think it's a really great improvement. It would be really nice to have the more compact panel-type shadow. Since we can't have that though, this is far superior to no shadow at all. Right now the panels are just kind of flat.
Created attachment 342528 [details]
Finder HUD panel shadow

Markus: really sorry for the delay in getting to the ui-reviews on this.  Overall the change is fantastic, and really improves the look of the panels.  I want us to try to match the native shadows as closely as possible.  It's a little hard to see how close we are since the screenshots don't have the shadows over a white content area, but attached is a guide to what we want to match.

Bottom: 10 pixel spread from black at 54% opacity to 1% opacity
Right and Left: 6 pixel spread from black at 31% opacity to 1% opacity
Comment on attachment 334164 [details]
Screenshot: shadow for identity panel

see comment 15
Attachment #334164 - Flags: ui-review?(faaborg) → ui-review-
Comment on attachment 334165 [details]
Screenshot: shadow for "Edit bookmark" panel

see comment 15
Attachment #334165 - Flags: ui-review?(faaborg) → ui-review-
Comment on attachment 334166 [details]
Screenshot: shadow for Ctrl-Tab panel

see comment 15
Attachment #334166 - Flags: ui-review?(faaborg) → ui-review-
(Assignee)

Comment 19

9 years ago
Oh, I should have clarified that before requesting ui-review...
Mac OS X doesn't give us any way of specifying the exact shadow style for a window. We only get a an on/off switch. The patch in this bug just makes the switch default to "on", so all panels end up having a shadow.

I suggest landing this patch now (it's necessary for bug 391984, which has waited long enough) and filing a new bug for the correct shadow style. Is that OK with you, Alex?

It's actually possible to get the shadow style exactly right - just not with -moz-window-shadow, but with -moz-box-shadow and some trickery that Aronnax showed me. I'll play with that in the new bug. The guide you attached will be very helpful, thanks!
(Assignee)

Comment 20

9 years ago
Alex: see comment 19 (I forgot to CC you)
>Oh, I should have clarified that before requesting ui-review...
>Mac OS X doesn't give us any way of specifying the exact shadow style for a
>window. We only get a an on/off switch. The patch in this bug just makes the
>switch default to "on", so all panels end up having a shadow.

>I suggest landing this patch now (it's necessary for bug 391984, which has
>waited long enough) and filing a new bug for the correct shadow style. Is that
>OK with you, Alex?

Sounds great, I'll switch my reviews for the panels to +'s.  For the control-tab panel we want to try to match the native alt-tab interface, which is light and doesn't have a shadow to convey that releasing the keys will dismiss the interface.
Comment on attachment 334164 [details]
Screenshot: shadow for identity panel

Good for now, let's pick up the native shadow in a separate bug.
Attachment #334164 - Flags: ui-review- → ui-review+
Comment on attachment 334165 [details]
Screenshot: shadow for "Edit bookmark" panel

Good for now, let's pick up the native shadow in a separate bug.
Attachment #334165 - Flags: ui-review- → ui-review+
(Assignee)

Comment 24

9 years ago
Created attachment 343053 [details] [diff] [review]
No window shadow for the Ctrl+Tab panel [checked in]
Attachment #343053 - Flags: review?(gavin.sharp)
(Assignee)

Updated

9 years ago
Attachment #334769 - Attachment description: fix v0.3 → fix v0.3 [checked in]
(Assignee)

Updated

9 years ago
Attachment #334162 - Attachment description: No window shadow for the autoscroller → No window shadow for the autoscroller [checked in]
(Assignee)

Comment 25

9 years ago
pushed:
http://hg.mozilla.org/mozilla-central/rev/fbb71be15c4d
http://hg.mozilla.org/mozilla-central/rev/16f593d3105b

I'll leave this bug open until attachment 343053 [details] [diff] [review] is reviewed.
Attachment #343053 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 26

9 years ago
Comment on attachment 343053 [details] [diff] [review]
No window shadow for the Ctrl+Tab panel [checked in]

pushed: http://hg.mozilla.org/mozilla-central/rev/1889da60bbf4
Attachment #343053 - Attachment description: No window shadow for the Ctrl+Tab panel → No window shadow for the Ctrl+Tab panel [checked in]
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2

Updated

9 years ago
Depends on: 460587
(Assignee)

Updated

8 years ago
Blocks: 527682
You need to log in before you can comment on or make changes to this bug.