Closed Bug 450939 Opened 11 years ago Closed 11 years ago

Add -moz-window-shadow CSS property

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #334158 - Flags: review?(dbaron)
The style tests pass.
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.
Er, actually, I think you should add something in nsStyleUIReset::CalcDifference for handling dynamic changes.  I'm not entirely sure what, though.
(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.
Attachment #334158 - Attachment is obsolete: true
Attachment #334193 - Flags: review?(dbaron)
Attachment #334158 - Flags: review?(dbaron)
(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?
(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?
Attachment #334193 - Attachment description: use SetDirect, add NS_STYLE_HINT_REFLOW for dynamic changes → use SetDiscrete, add NS_STYLE_HINT_REFLOW for dynamic changes
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.
Attachment #334193 - Flags: review?(dbaron) → review-
Attached patch address commentsSplinter Review
(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.
Attachment #334193 - Attachment is obsolete: true
Attachment #337203 - Flags: review?(dbaron)
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.
Attachment #337203 - Flags: superreview+
Attachment #337203 - Flags: review?(dbaron)
Attachment #337203 - Flags: review+
pushed: http://hg.mozilla.org/mozilla-central/rev/03bbada2e82f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
You need to log in before you can comment on or make changes to this bug.