Inset -moz-box-shadow can break outside of buttons if blur radius is 0.

RESOLVED FIXED in mozilla2.0b8

Status

()

defect
RESOLVED FIXED
9 years ago
10 months ago

People

(Reporter: nmrugg, Assigned: mats)

Tracking

({regression, testcase})

unspecified
mozilla2.0b8
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

()

Attachments

(3 attachments, 4 obsolete attachments)

Reporter

Description

9 years ago
User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b7) Gecko/20100101 Firefox/4.0b7
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b7) Gecko/20100101 Firefox/4.0b7

The shadow of a button element with an inset -moz-box-shadow CSS style with no blur radius can break out of the element and be visible.

Reproducible: Always

Steps to Reproduce:
1. Apply an inset -moz-box-shadow style to a button with a blur radius of 0.

Simple example: data:text/html,<button style="background-color: #0F0;-moz-box-shadow: inset -20px 10px 0 #F00;">No Blur Radius</button> <button style="background-color: #0F0;-moz-box-shadow: inset -20px 10px 1px #F00;">1px Blur Radius</button>
Actual Results:  
The shadow appears outside of the element.

Expected Results:  
The shadow should be clipped.

It works correctly on other elements, like <div> and <span> tags.

Note: Buttons need additional style (like background or border) in order to display the shadow at all.
Reporter

Comment 1

9 years ago
Posted file Testcase
This is the same as the example above but easier to view (because copying and pasting HTML doesn't always work the way you would expect).
Reporter

Updated

9 years ago
Keywords: regression
You marked this as 'regression'; when did it regress?
Status: UNCONFIRMED → NEW
Component: Style System (CSS) → Layout: View Rendering
Ever confirmed: true
QA Contact: style-system → layout.view-rendering
blocking2.0: --- → ?
Reporter

Comment 3

9 years ago
It works on Firefox 3.6.  I'm not sure exactly when it regressed.

I also confirmed it on Windows XP.
OS: Linux → All
Assignee

Comment 4

9 years ago
Regression window: 2010-04-17-03 -- 2010-04-18-03
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=981132d2f5d6&tochange=c15e44e90870
I'm guessing bug 544099.  I'll investigate the details...
Assignee: nobody → matspal
Blocks: 544099
Keywords: testcase
Assignee

Comment 5

9 years ago
Posted file Testcase #2
Assignee

Comment 6

9 years ago
Posted patch Patch rev. 1 (obsolete) — Splinter Review
This patch fixes three separate problems.
1. nsContextBoxBlur::Init optimize the case when the blur radius is zero
http://hg.mozilla.org/mozilla-central/annotate/8ec78ba9e09c/layout/base/nsCSSRendering.cpp#l3877
by using the destination directly instead of a temporary surface.
http://hg.mozilla.org/mozilla-central/annotate/8ec78ba9e09c/layout/base/nsCSSRendering.cpp#l1411
PaintBoxShadowInner paints to 'shadowContext' without using a clip
which I think is ok in the unoptimized case since the temp surface
does that.
Clipping when renderContext == shadowContext fixes that.

2. we're drawing outside the padding area (see Testcase #2).
Clamping the damage and skip rect to the padding box fixes that.

3. the assert is triggered by <fieldset> which doesn't always use
the frame rect -- let's just mute it for <fieldset> for now...
http://hg.mozilla.org/mozilla-central/annotate/8ec78ba9e09c/layout/forms/nsFieldSetFrame.cpp#l280

This patch breaks the "boxshadow-inner-basic.html" reftest due to
different AA (on the rounded inner edge).  I don't have the SVG skills
to fix that, see the comment in:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/box-shadow/boxshadow-inner-basic-ref.svg?raw=1

(I had to exclude testing <span> with border-radius in the attached
reftest for the same reason)

It passed Try unit tests, except for "boxshadow-inner-basic.html".
Attachment #491948 - Flags: review?(ventnor.bugzilla)
Assignee

Comment 7

9 years ago
I noted that we render the first <span> in Testcase #2 differently from
Chrome (9.0.587.0 dev, on Linux).  We put a shadow on the left edge of
all the continuations, Chrome only does that on the first.
Assignee

Comment 8

9 years ago
Regarding split inline boxes.  The 'box-shadow' chapter doesn't say
anything explicit [1].  The 'box-decoration-break' chapter [2] defines
'slice' as the initial value and for that value "No box-shadow is
drawn at the broken edge" -- so it seems Chrome has the correct rendering
and we have a bug.
[1] http://www.w3.org/TR/css3-background/#the-box-shadow
[2] http://www.w3.org/TR/css3-background/#the-box-decoration-break
Assignee

Comment 9

9 years ago
BTW, the assertion for <fieldset> is bug 610482
Blocks: 610482
Assignee

Comment 10

9 years ago
[1] does in fact mention it explicitly, saying "If an element has multiple boxes, all of them get drop shadows, but shadows are only drawn where borders would also be drawn; see ‘box-decoration-break’."
I filed bug 613659 on this.
+    if (renderContext == shadowContext) {
+      shadowContext->Clip();
+    }
     shadowContext->Fill();

This can't be right. Adding a Clip before a Fill of the same region should not change anything.

(In reply to comment #6)
> 2. we're drawing outside the padding area (see Testcase #2).
> Clamping the damage and skip rect to the padding box fixes that.

I don't understand why.

Isn't the problem here that shadowGfxRect (rounded by innerRadii) and skipGfxRect overlap, so the code that tries to clip to "shadowGfxRect minus skipGfxRect" using renderContext->SetFillRule(gfxContext::FILL_RULE_EVEN_ODD) doesn't work (because that gives you shadowGfxRect XOR skipGfxRect)?

We should probably be doing this as two separate clips, one to "<something large> minus skipGfxRect" and another to shadowGfxRect (rounded by innerRadii).
Assignee

Comment 12

9 years ago
Posted patch Patch rev. 2 (obsolete) — Splinter Review
(In reply to comment #11)
> This can't be right. Adding a Clip before a Fill of the same region should not
> change anything.

Thanks, removing it actually fixed the AA problems I noted above.
Attachment #491948 - Attachment is obsolete: true
Attachment #492231 - Flags: review?(ventnor.bugzilla)
Attachment #491948 - Flags: review?(ventnor.bugzilla)
Why have you got -webkit-box-shadow in the tests here?

What about the rest of comment #11?

I don't think you should be changing skipRect or dirtyPadding.

Comment 14

9 years ago
I think I understand why you change the skipRect (due to my probably sloppy use of EVEN_ODD clipping) and am OK with that, but I don't know why you change the dirtyRect.
Changing the skipRect is not the right thing here. The skipRect should only be advisory, it is not meant to act as an extra clip rect.
Assignee

Comment 16

9 years ago
(In reply to comment #13)
> Why have you got -webkit-box-shadow in the tests here?

Sorry, I was comparing the rendering in Chrome.

> What about the rest of comment #11?

skipGfxRect is used together with shadowGfxRect in a even-odd rule
fill.  shadowGfxRect is the padding area (rounded by innerRadii).
If we don't restrict skipGfxRect to the padding area we will fill
also the part of skipGfxRect that isn't covered by shadowGfxRect,
i.e. outside the padding area. (as I understand it - my GFX fu is weak)

(In reply to comment #14)
> ... but I don't know why you change the dirtyRect.

It's used as a clip rect in 
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxBlur.cpp#457
I could swear I needed to clip it to the padding rect, but it doesn't
seem necessary any more...  ok, removed that bit.

(In reply to comment #15)
> The skipRect should only be
> advisory, it is not meant to act as an extra clip rect.

You mean the use for the blurring area is what should be advisory?
Ok, if I rewrite the code to only clip it to the padding area for the
latter use (the even-odd fill) that works too.
Assignee

Comment 17

9 years ago
Posted patch Patch rev. 3 (obsolete) — Splinter Review
Removed the unnecessary clipping of aDirtyRect.
Fixed the testcase.
Attachment #492231 - Attachment is obsolete: true
Attachment #492346 - Flags: review?(ventnor.bugzilla)
Attachment #492231 - Flags: review?(ventnor.bugzilla)
Assignee

Comment 18

9 years ago
Here's the version that leaves skipRect unrestricted for the
blurring area use, but clips it to the padding rect for the
even-odd fill use.
I think the only reason we're trying to clip out skipGfxRect here is because when there's a blur radius, blurringArea might fill skipGfxRect with invalid data, and we want to ensure that that undefined stuff is not rendered.

So I suggest that we should remove all clipping due to skipGfxRect from PaintBoxShadowInner. Instead, nsContextBoxBlur should clip away skipGfxRect when it's blurring.
Assignee

Comment 20

9 years ago
Posted patch Patch rev. 5Splinter Review
roc, is this the changes you suggested for PaintBoxShadowInner?
This seems to work fine without any changes to nsContextBoxBlur.
Can you think of a case where the suggested clipping in
nsContextBoxBlur would be necessary?
Comment on attachment 492667 [details] [diff] [review]
Patch rev. 5

When there's a blur radius, gfxAlphaBoxBlur leaves the skiprect area unchanged. And by construction the gfxSkipRect is not touched by the rendered shadow (even after blurring), so those pixels must be completely transparent in the shadow, so drawing them changes nothing. So you're right! But we should probably have a comment to explain that.
Attachment #492667 - Flags: review+
Assignee

Updated

9 years ago
Attachment #492346 - Flags: review?(ventnor.bugzilla)
Assignee

Comment 22

9 years ago
I added your comment in the code.
http://hg.mozilla.org/mozilla-central/rev/cc2a24cd718a
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Assignee

Updated

9 years ago
Attachment #492346 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Attachment #492348 - Attachment is obsolete: true
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.