Last Comment Bug 450944 - Implement -moz-window-shadow functionality on Mac OS X
: Implement -moz-window-shadow functionality on Mac OS X
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: mozilla1.9.1b2
Assigned To: Markus Stange [:mstange]
:
: Markus Stange [:mstange]
Mentors:
Depends on: 450939 460587
Blocks: 415443 527682
  Show dependency treegraph
 
Reported: 2008-08-17 04:48 PDT by Markus Stange [:mstange]
Modified: 2009-11-10 07:56 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v0.1 (12.46 KB, patch)
2008-08-17 04:48 PDT, Markus Stange [:mstange]
hwaara: review+
Details | Diff | Splinter Review
No window shadow for the autoscroller [checked in] (671 bytes, patch)
2008-08-17 04:54 PDT, Markus Stange [:mstange]
mconnor: review+
Details | Diff | Splinter Review
Screenshot: shadow for identity panel (50.82 KB, image/png)
2008-08-17 05:07 PDT, Markus Stange [:mstange]
faaborg: ui‑review+
Details
Screenshot: shadow for "Edit bookmark" panel (85.40 KB, image/png)
2008-08-17 05:08 PDT, Markus Stange [:mstange]
faaborg: ui‑review+
Details
Screenshot: shadow for Ctrl-Tab panel (188.11 KB, image/png)
2008-08-17 05:09 PDT, Markus Stange [:mstange]
faaborg: ui‑review-
Details
fix v0.2: Move shadow style consts to nsIWidget, limit shadow invalidation frequency (16.29 KB, patch)
2008-08-18 15:41 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
fix v0.3 [checked in] (16.70 KB, patch)
2008-08-20 14:46 PDT, Markus Stange [:mstange]
roc: superreview+
Details | Diff | Splinter Review
Finder HUD panel shadow (6.07 KB, image/png)
2008-10-09 18:19 PDT, Alex Faaborg [:faaborg] (Firefox UX)
no flags Details
No window shadow for the Ctrl+Tab panel [checked in] (696 bytes, patch)
2008-10-14 08:24 PDT, Markus Stange [:mstange]
gavin.sharp: review+
Details | Diff | Splinter Review

Description User image Markus Stange [:mstange] 2008-08-17 04:48:17 PDT
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.
Comment 1 User image Markus Stange [:mstange] 2008-08-17 04:54:15 PDT
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.
Comment 2 User image Markus Stange [:mstange] 2008-08-17 05:07:17 PDT
Created attachment 334164 [details]
Screenshot: shadow for identity panel
Comment 3 User image Markus Stange [:mstange] 2008-08-17 05:08:15 PDT
Created attachment 334165 [details]
Screenshot: shadow for "Edit bookmark" panel
Comment 4 User image Markus Stange [:mstange] 2008-08-17 05:09:01 PDT
Created attachment 334166 [details]
Screenshot: shadow for Ctrl-Tab panel
Comment 5 User image Håkan Waara 2008-08-17 05:23:24 PDT
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.
Comment 6 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-08-17 21:18:43 PDT
We'd rather avoid unnecessary dependence on nsStyleConsts from widget. How about defining shadow styles in nsIWidget and then referencing them from nsStyleConsts.h?
Comment 7 User image Markus Stange [:mstange] 2008-08-18 15:41:21 PDT
Created attachment 334356 [details] [diff] [review]
fix v0.2: Move shadow style consts to nsIWidget, limit shadow invalidation frequency
Comment 8 User image Mike Connor [:mconnor] 2008-08-18 17:11:03 PDT
Comment on attachment 334162 [details] [diff] [review]
No window shadow for the autoscroller [checked in]

r=mconnor
Comment 9 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-08-19 21:42:34 PDT
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?
Comment 10 User image Markus Stange [:mstange] 2008-08-20 14:46:00 PDT
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.
Comment 11 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-08-20 16:08:48 PDT
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?
Comment 12 User image Markus Stange [:mstange] 2008-08-20 16:38:54 PDT
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 13 User image Mike Beltzner [:beltzner, not reading bugmail] 2008-09-16 08:46:12 PDT
Comment on attachment 334165 [details]
Screenshot: shadow for "Edit bookmark" panel

These look fine to me, but faaborg actually should get the final call :)
Comment 14 User image Stephen Horlander [:shorlander] 2008-09-16 09:11:39 PDT
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.
Comment 15 User image Alex Faaborg [:faaborg] (Firefox UX) 2008-10-09 18:19:26 PDT
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 16 User image Alex Faaborg [:faaborg] (Firefox UX) 2008-10-09 18:19:57 PDT
Comment on attachment 334164 [details]
Screenshot: shadow for identity panel

see comment 15
Comment 17 User image Alex Faaborg [:faaborg] (Firefox UX) 2008-10-09 18:20:09 PDT
Comment on attachment 334165 [details]
Screenshot: shadow for "Edit bookmark" panel

see comment 15
Comment 18 User image Alex Faaborg [:faaborg] (Firefox UX) 2008-10-09 18:20:25 PDT
Comment on attachment 334166 [details]
Screenshot: shadow for Ctrl-Tab panel

see comment 15
Comment 19 User image Markus Stange [:mstange] 2008-10-13 03:08:20 PDT
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!
Comment 20 User image Markus Stange [:mstange] 2008-10-13 03:10:10 PDT
Alex: see comment 19 (I forgot to CC you)
Comment 21 User image Alex Faaborg [:faaborg] (Firefox UX) 2008-10-13 13:18:09 PDT
>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 22 User image Alex Faaborg [:faaborg] (Firefox UX) 2008-10-13 13:18:41 PDT
Comment on attachment 334164 [details]
Screenshot: shadow for identity panel

Good for now, let's pick up the native shadow in a separate bug.
Comment 23 User image Alex Faaborg [:faaborg] (Firefox UX) 2008-10-13 13:19:14 PDT
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.
Comment 24 User image Markus Stange [:mstange] 2008-10-14 08:24:34 PDT
Created attachment 343053 [details] [diff] [review]
No window shadow for the Ctrl+Tab panel [checked in]
Comment 25 User image Markus Stange [:mstange] 2008-10-14 08:47:48 PDT
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.
Comment 26 User image Markus Stange [:mstange] 2008-10-14 12:30:00 PDT
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

Note You need to log in before you can comment on or make changes to this bug.