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.
Created attachment 114650 [details] Reduced demonstration of border-radius interaction with box-shadow
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.}
Created attachment 114653 [details] Further reduced example
Confirmed in WebKit Nightly, Version 5.1.2 (6534.52.7, r109280).
Created attachment 142947 [details] Patch
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?
Created attachment 146249 [details] Patch
(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
(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...
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.
Created attachment 146498 [details] Patch
(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
Comment on attachment 146498 [details] Patch Attachment 146498 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12922438
Comment on attachment 146498 [details] Patch As fmax is not available in windows, I will fix and upload a new patch again.
Created attachment 146526 [details] Patch
Created attachment 156014 [details] Patch
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
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
Created attachment 156055 [details] Patch
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
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
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
Created attachment 156296 [details] Patch
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
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
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
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
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
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
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
Created attachment 157176 [details] Patch
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
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
Created attachment 157371 [details] Patch
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.
Created attachment 157426 [details] Patch
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.
Created attachment 157630 [details] Patch
(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
Comment on attachment 157630 [details] Patch Clearing flags on attachment: 157630 Committed r125304: <http://trac.webkit.org/changeset/125304>
All reviewed patches have been landed. Closing bug.