Bug 72103 – box-shadow creates incorrect shadow when border-radius is too large

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Daniel Kurejwowski]

Created attachment 114649 [details]
Demonstrate border-radius interaction with box-shadow

When using box-shadow and border-radius on an element, the shadow is incorrectly drawn if border-radius's value is larger than the height or width of the element. This occurs in different ways depending on box-shadow's spread's sign. For negative values of box-shadow spread, the resulting shadow is rectangular. Additionally there is a section of the shadow that isn't painted (the element would occupy if it weren't for the rounded border).

If an element uses a large value for border-radius, the value is capped to a the maximum. If it is used along with box-shadow with a negative spread, then instead of the radius being capped it is ignored and a square shadow is created. The border radius' biggest value before it creates a square shadow seems to be the  width or height of the element (whichever is smaller) plus the spread.

The radius that triggers the missing shadow regions seems to be equal to the smaller of width or height and doesn't seem affected by the amount of spread.

So for example a width of 500px and height of 200px, a spread of -10px, "border-radius: 0 0 191px 0" will trigger a square shadow, while "0 0 190px 0" will not. "0 0 200px 0" will trigger both the square shadow and the missing region, "0 0 199px 0" will only trigger the square shadow.

[Daniel Kurejwowski]

Created attachment 114650 [details]
Reduced demonstration of border-radius interaction with box-shadow

[Daniel Kurejwowski]

Reproduced with 
-Safari (5.1.1 (7534.51.22)) for Windows XP
-Safari for iPad 2
-Rockmelt 0.9.68.1420 (Official Build 35c4f95) WebKit 535.1 (branches/chromium/835@96063) {note, only the square shadow is reproduceable, there is no apparent missing regions}
-Google Chrome	15.0.874.106 (Official Build 107270) WebKit 535.2 (@98043) {note, only the square shadow is reproducible, no missing regions.}

[Daniel Kurejwowski]

Created attachment 114653 [details]
Further reduced example

[David Barr]

Confirmed in WebKit Nightly, Version 5.1.2 (6534.52.7, r109280).

[Takashi Sakamoto]

Created attachment 142947 [details]
Patch

[Hajime Morrita]

Comment on attachment 142947 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142947&action=review

> Source/WebCore/platform/graphics/Path.cpp:-134
> -

Is this really OK? What will happen on other platforms?

[Takashi Sakamoto]

Created attachment 146249 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #6)
> (From update of attachment 142947 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142947&action=review
> 
> > Source/WebCore/platform/graphics/Path.cpp:-134
> > -
> 
> Is this really OK? What will happen on other platforms?

I see. I restricted the code, i.e. removing the condition, to chromium or mac.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 142947 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=142947&action=review
> > 
> > > Source/WebCore/platform/graphics/Path.cpp:-134
> > > -
> > 
> > Is this really OK? What will happen on other platforms?
> 
> I see. I restricted the code, i.e. removing the condition, to chromium or mac.

I think, I need someone's help for checking Windows or other platforms...

[Simon Fraser (smfr)]

Comment on attachment 146249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146249&action=review

> Source/WebCore/platform/graphics/Path.cpp:134
> +#if !PLATFORM(CHROMIUM) && !PLATFORM(MAC)
>      if (rect.width() < topLeftRadius.width() + topRightRadius.width()

Random platform #ifdefs are never a good thing in shared platform code. It's impossible to know, by reading this, why Chromium or Mac behave differently.

[Takashi Sakamoto]

Created attachment 146498 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #10)
> (From update of attachment 146249 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146249&action=review
> 
> > Source/WebCore/platform/graphics/Path.cpp:134
> > +#if !PLATFORM(CHROMIUM) && !PLATFORM(MAC)
> >      if (rect.width() < topLeftRadius.width() + topRightRadius.width()
> 
> Random platform #ifdefs are never a good thing in shared platform code. It's impossible to know, by reading this, why Chromium or Mac behave differently.

I see. I completely agree with you.

So I investigated how Firefox treats border-radiuses like this and implemented the logic in my new patch.
Now there are no #ifdefs in Path.cpp.

Best regards,
Takashi Sakamoto

[Build Bot]

Comment on attachment 146498 [details]
Patch

Attachment 146498 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12922438

[Takashi Sakamoto]

Comment on attachment 146498 [details]
Patch

As fmax is not available in windows, I will fix and upload a new patch again.

[Takashi Sakamoto]

Created attachment 146526 [details]
Patch

[Takashi Sakamoto]

Created attachment 156014 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156014 [details]
Patch

Attachment 156014 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13426145

New failing tests:
fast/borders/border-radius-larger-than-rect.html

[WebKit Review Bot]

Created attachment 156039 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Takashi Sakamoto]

Created attachment 156055 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156055 [details]
Patch

Attachment 156055 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13411897

New failing tests:
fast/borders/border-radius-larger-than-rect.html

[WebKit Review Bot]

Created attachment 156079 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Simon Fraser (smfr)]

Comment on attachment 156055 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156055&action=review

> Source/WebCore/platform/graphics/Path.cpp:138
> +        float maxRadiusWidth = std::max(topLeftRadius.width() + topRightRadius.width(), bottomLeftRadius.width() + bottomRightRadius.width());
> +        float maxRadiusHeight = std::max(topLeftRadius.height() + bottomLeftRadius.height(), topRightRadius.height() + bottomRightRadius.height());

We normally put a 'using namespace std' at the top of the file, then use min() and max() without the namespace qualifier.

> Source/WebCore/platform/graphics/Path.cpp:148
> +        float scale = maxRadiusWidth > maxRadiusHeight ? rect.width() / maxRadiusWidth : rect.height() / maxRadiusHeight;
> +        FloatSize adjustedTopLeftRadius(topLeftRadius);
> +        FloatSize adjustedTopRightRadius(topRightRadius);
> +        FloatSize adjustedBottomLeftRadius(bottomLeftRadius);
> +        FloatSize adjustedBottomRightRadius(bottomRightRadius);
> +        adjustedTopLeftRadius.scale(scale);
> +        adjustedTopRightRadius.scale(scale);
> +        adjustedBottomLeftRadius.scale(scale);
> +        adjustedBottomRightRadius.scale(scale);
> +        addPathForRoundedRect(rect, adjustedTopLeftRadius, adjustedTopRightRadius, adjustedBottomLeftRadius, adjustedBottomRightRadius, strategy);

Is it appropriate to have this code here, where it affects all path drawing, rather than in rendering code? What is the impact on other uses, like SVG and canvas?

It might be better to make RoundedFloatRect, then have a utility method on it that does the radius scaling, so we can call it from anywhere.

> LayoutTests/fast/borders/border-radius-larger-than-rect.html:12
> +    background-color: red;

Please don't use red in a passing test. Also, make the divs bigger so the issue is more apparent.

(In reply to comment #22)

> We normally put a 'using namespace std' at the top of the file, then use min() and max() without the namespace qualifier.

Not anymore: http://www.webkit.org/coding/coding-style.html#using-in-cpp

[Takashi Sakamoto]

Created attachment 156296 [details]
Patch

[Takashi Sakamoto]

Thank you for reviewing.

(In reply to comment #22)
> (From update of attachment 156055 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156055&action=review
> 
> > Source/WebCore/platform/graphics/Path.cpp:138
> > +        float maxRadiusWidth = std::max(topLeftRadius.width() + topRightRadius.width(), bottomLeftRadius.width() + bottomRightRadius.width());
> > +        float maxRadiusHeight = std::max(topLeftRadius.height() + bottomLeftRadius.height(), topRightRadius.height() + bottomRightRadius.height());
> 
> We normally put a 'using namespace std' at the top of the file, then use min() and max() without the namespace qualifier.
> 
> > Source/WebCore/platform/graphics/Path.cpp:148
> > +        float scale = maxRadiusWidth > maxRadiusHeight ? rect.width() / maxRadiusWidth : rect.height() / maxRadiusHeight;
> > +        FloatSize adjustedTopLeftRadius(topLeftRadius);
> > +        FloatSize adjustedTopRightRadius(topRightRadius);
> > +        FloatSize adjustedBottomLeftRadius(bottomLeftRadius);
> > +        FloatSize adjustedBottomRightRadius(bottomRightRadius);
> > +        adjustedTopLeftRadius.scale(scale);
> > +        adjustedTopRightRadius.scale(scale);
> > +        adjustedBottomLeftRadius.scale(scale);
> > +        adjustedBottomRightRadius.scale(scale);
> > +        addPathForRoundedRect(rect, adjustedTopLeftRadius, adjustedTopRightRadius, adjustedBottomLeftRadius, adjustedBottomRightRadius, strategy);
> 
> Is it appropriate to have this code here, where it affects all path drawing, rather than in rendering code? What is the impact on other uses, like SVG and canvas?
> 
> It might be better to make RoundedFloatRect, then have a utility method on it that does the radius scaling, so we can call it from anywhere.

I think, the code is just for fixing border-radius only when the radius is invalid. However, I reverted these codes and modified the renderer to fix this issue.

> 
> > LayoutTests/fast/borders/border-radius-larger-than-rect.html:12
> > +    background-color: red;
> 
> Please don't use red in a passing test. Also, make the divs bigger so the issue is more apparent.

I see. However I found that this test is not good for testing this issue. I recreated a new test from attached repro html.

Best regards,
Takashi Sakamoto

[WebKit Review Bot]

Comment on attachment 156296 [details]
Patch

Attachment 156296 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13431268

New failing tests:
fast/borders/border-shadow-large-radius.html

[WebKit Review Bot]

Created attachment 156307 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 156296 [details]
Patch

Attachment 156296 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13433029

New failing tests:
fast/borders/border-shadow-large-radius.html

[WebKit Review Bot]

Created attachment 156322 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Build Bot]

Comment on attachment 156296 [details]
Patch

Attachment 156296 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13426572

New failing tests:
fast/borders/border-shadow-large-radius.html

[Build Bot]

Created attachment 156325 [details]
Archive of layout-test-results from apple-mac-3

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: apple-mac-3  Port: <class 'webkitpy.common.config.ports.MacPort'>  Platform: Mac OS X 10.7.4

[Takashi Sakamoto]

Created attachment 157176 [details]
Patch

[WebKit Review Bot]

Comment on attachment 157176 [details]
Patch

Attachment 157176 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13461132

New failing tests:
fast/borders/border-shadow-large-radius.html

[WebKit Review Bot]

Created attachment 157191 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Takashi Sakamoto]

Created attachment 157371 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 157371 [details]
Patch

Please make the text not visible in the pixel results; it will cause spurious pixel comparison failures. You can have it as an HTML comment.

[Takashi Sakamoto]

Created attachment 157426 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 157426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157426&action=review

> Source/WebCore/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

This is still missing.

[Takashi Sakamoto]

Created attachment 157630 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #38)
> (From update of attachment 157426 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157426&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> 
> This is still missing.

Thank you. I fixed this.

Best regards,
Takashi Sakamoto

[WebKit Review Bot]

Comment on attachment 157630 [details]
Patch

Clearing flags on attachment: 157630

Committed r125304: <http://trac.webkit.org/changeset/125304>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.