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

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
2 years ago

People

(Reporter: mounir, Assigned: roc)

Tracking

(Depends on: 1 bug)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 beta5+)

Details

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 459628 [details]
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?
(Reporter)

Comment 3

9 years ago
(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.
I fixed the summary.
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.
Created attachment 460823 [details] [diff] [review]
part 1: back out 476062
Assignee: nobody → roc
Created attachment 460824 [details] [diff] [review]
Part 2: extend gfxQuartzNativeRenderer to handle all kinds of surface targets

We'll need this so we can render native Mac widgets to an alpha-only image surface.
Attachment #460824 - Flags: review?(vladimir)
Created attachment 460825 [details] [diff] [review]
Part 3: Add spread option to gfxAlphaBoxBlur/nsContextBoxBlur

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)
Created attachment 460827 [details] [diff] [review]
Part 4: support outer box-shadows on themed widgets

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.
(Reporter)

Comment 14

9 years ago
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.
Created attachment 465464 [details] [diff] [review]
Part 4 v2

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?
(Reporter)

Comment 26

8 years ago
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]
(Reporter)

Updated

8 years ago
No longer blocks: 580575
(Reporter)

Updated

8 years ago
Blocks: 582277
Created attachment 468208 [details] [diff] [review]
Fix opaque native-theme background bounds

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]
Keywords: checkin-needed
Whiteboard: [needs landing]

Updated

2 years ago
Depends on: 1327935
You need to log in before you can comment on or make changes to this bug.