483533 - drawing glitches with inset box-shadows when there's a border-radius

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase

This testcase (simplified from Håkon Lie's talk today at SXSW) shows drawing glitches with inset box shadows when there is a border-radius.

Steps to reproduce: load testcase

Expected results: Green and yellow shapes cleanly surround red

Actual results:  Extra blotches of red in the yellow band near the upper left and lower right corners.

I'm seeing this on my Linux debug build from a week or so ago.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The slides from which this was simplified are at http://people.opera.com/howcome/2009/talks/03-css3.html (although they require changing the projection style sheet to screen to see things in Gecko).

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We should perhaps try to get a fix for this for 1.9.1

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Zack might find this interesting also

Comment 4

[Michael Ventnor]

This is probably a problem with the way we calculate/use rounded corners. We get the inner radii of the border, then inset to make the shadow, then use that radii that we got from the original frame size, which could be too big for the inset shadow. This is why following an edge of the shadow gives the exact same line as the inner border edge.

The fix would be to figure out what scenario(s) we should limit our border radii for inset shadows, and what we should limit it to.

Comment 5

[Michael Ventnor]

So this is because we're using improper radii on shadowClipRect.

My suggestion to fix this was to add the spread distance to the gfxFloat[4] of borderSizes, and call nsCSSBorderRenderer::ComputeInnerRadii with the original borderRadii, but with the new border sizes. This should give back the inner radii that the shadowClipRect would use.

While this improves things slightly, it still doesn't make the problem go away completely, and I'm at a loss as to why.

Comment 6

[Michael Ventnor]

Attachment: Solution?

I can't find anything wrong with this logic, so I'm wondering whether its a bug with my computer or display driver (wouldn't be the first time). Does this patch solve the problem for anyone, especially non-Linux folk?

Comment 7

[Michael Ventnor]

Attachment: Patch

This makes this testcase a bit better but there is still a bug with the way we calculate the rounded corner path (and I don't think that's part of my code).

However, this is a general solution we really need to take as without it, inset box-shadows for rounded boxes are very broken in general. I can't make a reftest for it because I can't make pixel-perfect duplicates with any approach I can think of.

Comment 8

[Michael Ventnor]

Attachment: Patch 2

Actually, turns out we need this patch regardless, because the spec says that spread should not change the shape of the box shadow. Our box shadows are changing shape based on spread. So we need this (preferably for 1.9.1) not only to fix spread but also fix our compliance.

Reftests pass.

Comment 9

[Michael Ventnor]

Attachment: Patch 2.1

I was able to do this with slightly less code.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 376611 [details] [diff] [review]
Patch 2.1

+    if (hasBorderRadius) {
+      gfxCornerSizes clipRectRadii;
+      gfxFloat spreadDistance = shadowItem->mSpread * -1 / twipsPerPixel;
+      gfxFloat borderSizes[4] = {
+        spreadDistance, spreadDistance,
+        spreadDistance, spreadDistance
+      };
+      nsCSSBorderRenderer::ComputeInnerRadii(borderRadii, borderSizes,
+                                             &clipRectRadii);
+      shadowContext->RoundedRectangle(shadowRect, clipRectRadii);
+    } else
       shadowContext->Rectangle(shadowRect);

{} around the the ->Rectangle call.

+      gfxFloat spreadDistance = shadowItem->mSpread * -1 / twipsPerPixel;

Why not just -shadowItem->mSpread/twipsPerPixel?

Comment 11

[Michael Ventnor]

Attachment: Patch 2.2

For checkin.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/68d219cad3a0

Comment 13

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cfbbfb8b4d53