Closed Bug 581222 Opened 14 years ago Closed 14 years ago

-moz-box-shadow doesn't apply on natively themed elements

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: mounir, Assigned: roc)

References

(Depends on 1 open bug)

Details

Attachments

(7 files, 1 obsolete file)

Attached file Preview
-moz-box-shadow should be used for :invalid and :-moz-submit-invalid default styles but unfortunately it doesn't apply on natively themed elements.
see bug 476062 – -moz-box-shadow only works on author styled widgets
Probably because we draw box-shadow in background painting, and we skip background painting when there's a native-theme background.

Probably fixable with some refactoring, but would the shadow really be the desired shape?
(In reply to comment #2)
> Probably because we draw box-shadow in background painting, and we skip
> background painting when there's a native-theme background.
> 
> Probably fixable with some refactoring, but would the shadow really be the
> desired shape?

There would be a way to show a box-shadow on a MacOS button for example? According to the bug mentioned by philippe in comment 1 we can't.
Apparently I was wrong, and we changed it in bug 476062, but the summary of bug 476062 is wrong.
Bug 476062 was a deliberate change. box-shadow often doesn't look good when applied to native-themed elements, and of course authors can't necessarily tell when an element is native-themed. The change matches what Webkit does.
In bug 566045 we really need box-shadow to work well on native-themed widgets. So I propose we back out 476062 and fix box-shadow.

Here's how I propose we fix it:
1) Add an nsITheme API to tell us whether the native background is a (rounded) rectangle that the shadow should go around (returning the corner radius, which will usually be zero). For particular theme/widgets, we can hardcode knowledge there for performance. (This API would return true for every widget where GetWidgetTransparency returns opaque, since every opaque widget must exactly fill its rectangle.)
2) For box-shadows on themed controls, call that API. If it returns true, we can use the normal optimized box-shadow path, using the returned corner radius.
3) If that API returns false, use a new box-shadow drawing path where we extract the alpha channel of the widget background and treat that as the box shape. That mask gets offset, blurred, and then we paint the shadow color with the mask.

I think this is actually not hard.

In step 3, we might want to fill the content-box of the mask with opaque fill, to cover up any "holes" in the theme rendering. E.g. imagine a theme that renders a textarea with a rounded border. I guess it wouldn't look too bad if there's glow on the inside of the border, but it would be nice to avoid.
We'll need this so we can render native Mac widgets to an alpha-only image surface.
Attachment #460824 - Flags: review?(vladimir)
I probably should rename these classes, but for now I've just added a "spread" parameter.

We'll need this so that we can apply "spread" to a shape that's nothing more than an alpha mask.
Attachment #460825 - Flags: review?(vladimir)
This is fairly straightforward actually. I didn't bother adding new theme API. Basically for themed widgets we behave just like normal frames except that
a) we don't try to clip the border-box out of the shadow, since the theme background is going to be drawn over the shadow
b) instead of making the border-box be the shadow shape, the shadow shape is the alpha-mask of the theme background
c) the spread is computed over the shadow pixels rather than by inflating the shadow rect. For a spread of N pixels, each destination pixel gets the alpha value that's the max of the source pixels in a square of side length 2N+1 centered on the pixel. (I was wrong in the gfxBlur comments --- this is L_infinity norm, not L_1 --- will fix.)
Attachment #460827 - Flags: review?(dbaron)
Comment on attachment 460825 [details] [diff] [review]
Part 3: Add spread option to gfxAlphaBoxBlur/nsContextBoxBlur

This looks ok to me, though you may want to get jeff to take a look at the spread functions as well.  Also, any reason to have the "doVertical = 1" in there?  It's only checked in one place and is effectively constant true..
Attachment #460825 - Flags: review?(vladimir) → review+
I'll take out doVertical. That was just a debugging thing.
Asking blocking-2.0 flag because we need this for the default invalid form element style.
blocking2.0: --- → ?
(In reply to comment #11)
> Created attachment 460827 [details] [diff] [review]
> Part 4: support outer box-shadows on themed widgets

Does this mean inset won't work? It's being used in the findbar on Linux and Windows.
I guess we should enable it for chrome like we did before.
Attached patch Part 4 v2Splinter Review
Reenable inner box-shadows for chrome docs
Attachment #465464 - Flags: review?(dbaron)
Attachment #460827 - Attachment is obsolete: true
Attachment #460827 - Flags: review?(dbaron)
Comment on attachment 465464 [details] [diff] [review]
Part 4 v2

Mats could review this instead of dbaron
Attachment #465464 - Flags: review?(dbaron) → review?(matspal)
Comment on attachment 465464 [details] [diff] [review]
Part 4 v2

>+    // There's no way of getting hold of a shape corresponding to a
>+    // "padding-box" for native-themed widgets, so just don't draw
>+    // inner box-shadows for them. But we allow chrome to paint inner
>+    // box shadows since chrome can be aware of the platform theme.
>+    return;

This isn't quite true; native-theme widgets must report border areas, so
we do have a padding box.  The issue is more that such a box doesn't
make sense for all widgets, but chrome has more knowledge of which 
widgets it does and doesn't make sense for.  (And our code may be more 
likely to get it right for native-theme widgets that are containers,
because it matters more for them.)


r=dbaron, and sorry for the delay
Attachment #465464 - Flags: review?(matspal) → review+
(In reply to comment #20)
> This isn't quite true; native-theme widgets must report border areas, so
> we do have a padding box.

We can compute a padding box based on the reported borders, but we don't really know what shape it is. E.g. the native widget might have rounded borders but we don't know that.
Keywords: checkin-needed
Whiteboard: [needs landing]
Comment on attachment 460823 [details] [diff] [review]
part 1: back out 476062

This will let us draw box-shadows on native-themed form controls, which is important for anyone who wants to use that property to highlight invalid-state controls (including possibly our UA style sheets).
Attachment #460823 - Flags: approval2.0?
Comment on attachment 460824 [details] [diff] [review]
Part 2: extend gfxQuartzNativeRenderer to handle all kinds of surface targets

This will let us draw box-shadows on native-themed form controls, which is important for anyone who wants to use that property to highlight invalid-state controls (including possibly our UA style sheets).
Attachment #460824 - Flags: approval2.0?
Comment on attachment 460825 [details] [diff] [review]
Part 3: Add spread option to gfxAlphaBoxBlur/nsContextBoxBlur

This will let us draw box-shadows on native-themed form controls, which is important for anyone who wants to use that property to highlight invalid-state controls (including possibly our UA style sheets).
Attachment #460825 - Flags: approval2.0?
Comment on attachment 465464 [details] [diff] [review]
Part 4 v2

This will let us draw box-shadows on native-themed form controls, which is important for anyone who wants to use that property to highlight invalid-state controls (including possibly our UA style sheets).
Attachment #465464 - Flags: approval2.0?
Roc, I was wondering if the same method could be used to have borders on natively themed controls?
No, it cannot. -moz-appearance is designed to suppress the CSS border and background; we assume nsITheme::DrawWidgetBackground is going to draw both borders and backgrounds, since that's what native theme APIs do.
Keywords: checkin-needed
Whiteboard: [needs landing] → [needs approval]
These patches no longer apply cleanly on tip.
a=sicking though since this blocks HTML5 forms which is a blocker.
I've got updated versions locally.
Whiteboard: [needs approval] → [needs landing]
No longer blocks: 580575
Blocks: 582277
Tests failed on try-server on Windows 7... we failed to draw the listbox shadow. The problem is that nsDisplayBackground::IsOpaque returns true for listboxes on Windows 7, but nsDisplayBackground::GetBounds returns the entire overflow area for the frame, so we think the shadow is completely covered by the listbox background. We should only return true for IsOpaque if the entire display item bounds is opaque, and so the right thing to do here is make the display item bounds correctly be the area drawn by the native theme background (which is less than the overflow area when we have shadows, and in other cases).
Attachment #468208 - Flags: review?(tnikkel)
Attachment #468208 - Flags: review?(tnikkel) → review+
http://hg.mozilla.org/mozilla-central/rev/524c0b6b290d caused a lot of new -Wshadow warnings:

../../dist/include/gfxRect.h: In member function ‘void gfxRect::Inset(const gfxIntSize&)’:
../../dist/include/gfxRect.h:140: warning: declaration of ‘size’ shadows a member of 'this' [-Wshadow]
../../dist/include/gfxRect.h: In member function ‘void gfxRect::Outset(const gfxIntSize&)’:
../../dist/include/gfxRect.h:162: warning: declaration of ‘size’ shadows a member of 'this' [-Wshadow]
Comment on attachment 472830 [details] [diff] [review]
fix warnings

Thanks.  r=dbaron
Attachment #472830 - Flags: review?(dbaron) → review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Depends on: 1327935
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: