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]
:
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 | 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 | 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 | Review
fix v0.3 [checked in] (16.70 KB, patch)
2008-08-20 14:46 PDT, Markus Stange [:mstange]
roc: superreview+
Details | Diff | 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 | Review

Description 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 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 Markus Stange [:mstange] 2008-08-17 05:07:17 PDT
Created attachment 334164 [details]
Screenshot: shadow for identity panel
Comment 3 Markus Stange [:mstange] 2008-08-17 05:08:15 PDT
Created attachment 334165 [details]
Screenshot: shadow for "Edit bookmark" panel
Comment 4 Markus Stange [:mstange] 2008-08-17 05:09:01 PDT
Created attachment 334166 [details]
Screenshot: shadow for Ctrl-Tab panel
Comment 5 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 Robert O'Callahan (:roc) (Exited; 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 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 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 Robert O'Callahan (:roc) (Exited; 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 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 Robert O'Callahan (:roc) (Exited; 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 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 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 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 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 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 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 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 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 Markus Stange [:mstange] 2008-10-13 03:10:10 PDT
Alex: see comment 19 (I forgot to CC you)
Comment 21 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 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 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 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 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 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.