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)
Core
CSS Parsing and Computation
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)
1.52 KB,
text/html
|
Details | |
5.28 KB,
patch
|
sicking
:
approval2.0+
|
Details | Diff | Splinter Review |
6.05 KB,
patch
|
vlad
:
review+
sicking
:
approval2.0+
|
Details | Diff | Splinter Review |
27.33 KB,
patch
|
vlad
:
review+
sicking
:
approval2.0+
|
Details | Diff | Splinter Review |
18.91 KB,
patch
|
dbaron
:
review+
sicking
:
approval2.0+
|
Details | Diff | Splinter Review |
1.19 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
1.76 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
-moz-box-shadow should be used for :invalid and :-moz-submit-invalid default styles but unfortunately it doesn't apply on natively themed elements.
Comment 1•14 years ago
|
||
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•14 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.
Apparently I was wrong, and we changed it in bug 476062, but the summary of bug 476062 is wrong.
Assignee | ||
Comment 5•14 years ago
|
||
I fixed the summary.
Assignee | ||
Comment 6•14 years ago
|
||
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.
Assignee | ||
Comment 7•14 years ago
|
||
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.
Assignee | ||
Comment 8•14 years ago
|
||
Assignee: nobody → roc
Assignee | ||
Comment 9•14 years ago
|
||
We'll need this so we can render native Mac widgets to an alpha-only image surface.
Attachment #460824 -
Flags: review?(vladimir)
Assignee | ||
Comment 10•14 years ago
|
||
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)
Assignee | ||
Comment 11•14 years ago
|
||
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)
Attachment #460824 -
Flags: review?(vladimir) → review+
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+
Assignee | ||
Comment 13•14 years ago
|
||
I'll take out doVertical. That was just a debugging thing.
Reporter | ||
Comment 14•14 years ago
|
||
Asking blocking-2.0 flag because we need this for the default invalid form element style.
blocking2.0: --- → ?
Comment 15•14 years ago
|
||
(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.
Comment 16•14 years ago
|
||
And here: http://hg.mozilla.org/mozilla-central/annotate/0df95f3a6de6/browser/themes/gnomestripe/browser/browser.css#l1353
Assignee | ||
Comment 17•14 years ago
|
||
I guess we should enable it for chrome like we did before.
Assignee | ||
Comment 18•14 years ago
|
||
Reenable inner box-shadows for chrome docs
Attachment #465464 -
Flags: review?(dbaron)
Assignee | ||
Updated•14 years ago
|
Attachment #460827 -
Attachment is obsolete: true
Attachment #460827 -
Flags: review?(dbaron)
Assignee | ||
Comment 19•14 years ago
|
||
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+
Assignee | ||
Comment 21•14 years ago
|
||
(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]
Assignee | ||
Comment 22•14 years ago
|
||
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?
Assignee | ||
Comment 23•14 years ago
|
||
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?
Assignee | ||
Comment 24•14 years ago
|
||
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?
Assignee | ||
Comment 25•14 years ago
|
||
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•14 years ago
|
||
Roc, I was wondering if the same method could be used to have borders on natively themed controls?
Assignee | ||
Comment 27•14 years ago
|
||
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.
Attachment #460823 -
Flags: approval2.0? → approval2.0+
Attachment #460824 -
Flags: approval2.0? → approval2.0+
Attachment #460825 -
Flags: approval2.0? → approval2.0+
Attachment #465464 -
Flags: approval2.0? → approval2.0+
a=sicking though since this blocks HTML5 forms which is a blocker.
Assignee | ||
Comment 30•14 years ago
|
||
I've got updated versions locally.
Whiteboard: [needs approval] → [needs landing]
Assignee | ||
Comment 31•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #468208 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 32•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/b9bc9fa58cc0 http://hg.mozilla.org/mozilla-central/rev/6524df64c077 http://hg.mozilla.org/mozilla-central/rev/524c0b6b290d http://hg.mozilla.org/mozilla-central/rev/bac07a0a4f00 http://hg.mozilla.org/mozilla-central/rev/1afaf2219f00
Status: NEW → RESOLVED
blocking2.0: ? → beta5+
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
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]
Assignee | ||
Comment 34•14 years ago
|
||
Attachment #472830 -
Flags: review?(dbaron)
Comment on attachment 472830 [details] [diff] [review] fix warnings Thanks. r=dbaron
Attachment #472830 -
Flags: review?(dbaron) → review+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/d85d2070d0f8
Keywords: checkin-needed
Whiteboard: [needs landing]
You need to log in
before you can comment on or make changes to this bug.
Description
•