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

RESOLVED FIXED in mozilla1.9.1b2

Status

()

defect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

Trunk
mozilla1.9.1b2
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 2 obsolete attachments)

Assignee

Description

11 years ago
Posted patch fix v0.1 (obsolete) — Splinter Review
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

11 years ago
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

11 years ago
Attachment #334164 - Flags: ui-review?(beltzner)
Assignee

Comment 3

11 years ago
Attachment #334165 - Flags: ui-review?(beltzner)
Assignee

Comment 4

11 years ago
Attachment #334166 - Flags: ui-review?(beltzner)

Comment 5

11 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

11 years ago
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

11 years ago
(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

11 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

11 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.
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

11 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

11 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

Updated

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

Updated

11 years ago
Attachment #334162 - Attachment description: No window shadow for the autoscroller → No window shadow for the autoscroller [checked in]
Attachment #343053 - Flags: review?(gavin.sharp) → review+
Assignee

Comment 26

11 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

11 years ago
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Depends on: 460587
Assignee

Updated

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