450939 - Add -moz-window-shadow CSS property

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Markus Stange [:mstange]]

Attachment: patch

I'd like to add a new CSS property for controlling window shadow for bug 415443.

Name:       -moz-window-shadow
Values:     default (initial value), none
Inherited:  no
Location:   nsStyleUIReset

Resulting behavior:
 - default: The window should have the shadow style that's default for its
            window level.
 - none:    No shadow.

The list of values can later be extended for different window types. However, we don't need that yet because toggling between different shadow styles is quite difficult on Mac OS X. For an initial implementation, an on / off toggle is enough.

Using a CSS property instead of a XUL attribute is better for themers.

Comment 1

[Markus Stange [:mstange]]

The style tests pass.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Codewise the patch looks fine except that nsRuleNode::ComputeUIResetData should use SetDiscrete (see bug 441367, where it was added).

I need to go reread the discussion of whether we want this, etc.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, actually, I think you should add something in nsStyleUIReset::CalcDifference for handling dynamic changes.  I'm not entirely sure what, though.

Comment 4

[Markus Stange [:mstange]]

Attachment: use SetDiscrete, add NS_STYLE_HINT_REFLOW for dynamic changes

(In reply to comment #3)
> Er, actually, I think you should add something in
> nsStyleUIReset::CalcDifference for handling dynamic changes.  I'm not entirely
> sure what, though.

In the implementation in bug 450944 I propagate the shadow setting to the window in nsContainerFrame::SyncFrameViewGeometryDependentProperties. That seems to be called after a reflow so I chose NS_STYLE_HINT_REFLOW. It works.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #4)
> In the implementation in bug 450944 I propagate the shadow setting to the
> window in nsContainerFrame::SyncFrameViewGeometryDependentProperties. That
> seems to be called after a reflow so I chose NS_STYLE_HINT_REFLOW.

In that case, you should be able to just use nsChangeHint_SyncFrameView.  Note that NS_STYLE_HINT_REFLOW is nsChangeHint(nsChangeHint_RepaintFrame | nsChangeHint_SyncFrameView | nsChangeHint_ReflowFrame).

> It works.

By "It works.", you mean that dynamic changes to the property start working when you do add the appropriate code to CalcDifference, whereas they didn't work before?

Comment 6

[Markus Stange [:mstange]]

(In reply to comment #5)
> (In reply to comment #4)
> > In the implementation in bug 450944 I propagate the shadow setting to the
> > window in nsContainerFrame::SyncFrameViewGeometryDependentProperties. That
> > seems to be called after a reflow so I chose NS_STYLE_HINT_REFLOW.
> 
> In that case, you should be able to just use nsChangeHint_SyncFrameView.  Note
> that NS_STYLE_HINT_REFLOW is nsChangeHint(nsChangeHint_RepaintFrame |
> nsChangeHint_SyncFrameView | nsChangeHint_ReflowFrame).

Oh, I didn't notice that.

> > It works.
> 
> By "It works.", you mean that dynamic changes to the property start working
> when you do add the appropriate code to CalcDifference, whereas they didn't
> work before?

Yes, that's what I meant.

I've just tested all the nsChangHint_* flags and only nsChangeHint_ReflowFrame seems to do what I need. However, I don't know the code well enough to make any sense of that.
Should I post a new patch that uses nsChangeHint_ReflowFrame instead of NS_STYLE_HINT_REFLOW?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 334193 [details] [diff] [review]
use SetDiscrete, add NS_STYLE_HINT_REFLOW for dynamic changes

The CalcDifference changes are wrong, since they make us not return NS_STYLE_HINT_FRAMECHANGE (which is higher than reflow) if mForceBrokenImageIcon changes.

I'm still a bit puzzled about why you needed ReflowFrame rather than SyncFrameView given what you said above.  Given the way the CalcDifference is structured, you probably want to either (1) use NS_STYLE_HINT_REFLOW or (2) restructure that CalcDifference method so it ors bits together.

Comment 8

[Markus Stange [:mstange]]

Attachment: address comments

(In reply to comment #7)
> (From update of attachment 334193 [details] [diff] [review])
> The CalcDifference changes are wrong, since they make us not return
> NS_STYLE_HINT_FRAMECHANGE (which is higher than reflow) if
> mForceBrokenImageIcon changes.

Fixed.

> I'm still a bit puzzled about why you needed ReflowFrame rather than
> SyncFrameView given what you said above.

I think I've found the reason. I need SyncFrameViewGeometryDependentProperties to be called on the surrounding ViewportFrame, not on the window's MenuPopupFrame. This only seems to happen after a reflow.
Does that make sense?

Anyway, it's probably not worth spending more time figuring that out.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 337203 [details] [diff] [review]
address comments

>+  if (mWindowShadow != aOther.mWindowShadow)
>+    return NS_STYLE_HINT_REFLOW;

Could you just add a comment here that says something like:
  // We really need just an nsChangeHint_SyncFrameView, except
  // on an ancestor of the frame, so we get that by doing a
  // reflow.


With that, r+sr=dbaron.

Comment 10

[Markus Stange [:mstange]]

pushed: http://hg.mozilla.org/mozilla-central/rev/03bbada2e82f