Bug 116745

Summary: [CSS Shapes] rectangle and inset-rectangle do not properly handle rx and ry
Product: WebKit Reporter: Bem Jones-Bey <bjonesbe>
Component: CSSAssignee: Bem Jones-Bey <bjonesbe>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, esprehn+autocc, giles_joplin, glenn, macpherson, menard, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://dev.w3.org/csswg/css-shapes/#basic-shapes-from-svg-syntax
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
Patch
none
Patch
none
Patch none

Description Bem Jones-Bey 2013-05-24 16:06:03 PDT
According to the SVG spec (http://www.w3.org/TR/SVG/shapes.html#RectElement), the default value for rx and ry when one is unspecified is the value of the other. The current implementation always sets an unspecified rx or ry to 0.
Comment 1 Bem Jones-Bey 2013-06-05 09:35:36 PDT
It looks like the behavior of rx and ry is more incorrect than originally thought, they should follow all of the rules in the SVG spec for the rx and ry parameters of rect.
Comment 2 Bem Jones-Bey 2013-06-06 17:33:23 PDT
Created attachment 203981 [details]
Patch

Fix to repsect the rules in the spec, if ry is omitted, it defaults to rx, and if rx > width/2, rx = width/2, and if ry > height/2, ry = height/2
Comment 3 Build Bot 2013-06-06 23:43:25 PDT
Comment on attachment 203981 [details]
Patch

Attachment 203981 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/772585

New failing tests:
css3/masking/clip-path-animation.html
Comment 4 Build Bot 2013-06-06 23:43:27 PDT
Created attachment 204003 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 5 Build Bot 2013-06-07 06:58:44 PDT
Comment on attachment 203981 [details]
Patch

Attachment 203981 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/772668

New failing tests:
css3/masking/clip-path-animation.html
Comment 6 Build Bot 2013-06-07 06:58:46 PDT
Created attachment 204040 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3
Comment 7 Bem Jones-Bey 2013-06-07 08:25:35 PDT
*sigh* I take this as a sign that I'm doing way too many different things. Will be fixing. Than you EWS for catching my fail.
Comment 8 Bem Jones-Bey 2013-06-07 14:19:31 PDT
Created attachment 204067 [details]
Patch

Update missed test
Comment 9 Bem Jones-Bey 2013-06-12 10:13:15 PDT
Created attachment 204454 [details]
Patch

Change for new spec behavior: the radii get now get scaled the as defined for border radius, not clamped as in SVG.
Comment 10 Dirk Schulze 2013-06-12 10:57:08 PDT
Comment on attachment 204454 [details]
Patch

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

r=me but please add the assertions to be sure.

> Source/WebCore/css/BasicShapeFunctions.cpp:-53
> -        if (!rectangle->cornerRadiusX().isUndefined()) {

Was skeptical about about removing this if clause. Bem wants to add an ASSERT, since he says it can never be undefined. He should be right, since we always set the value on style resolving.

> Source/WebCore/css/BasicShapeFunctions.cpp:-105
> -        if (!rectangle->cornerRadiusX().isUndefined()) {

Ditto.
Comment 11 Bem Jones-Bey 2013-06-12 11:26:41 PDT
Created attachment 204464 [details]
Patch

Update to add asserts
Comment 12 WebKit Commit Bot 2013-06-12 11:59:08 PDT
The commit-queue encountered the following flaky tests while processing attachment 204464 [details]:

media/video-played-collapse.html bug 58630 (authors: annacc@chromium.org, jamesr@chromium.org, pnormand@igalia.com, and vrk@chromium.org)
media/audio-garbage-collect.html bug 117555 (author: rniwa@webkit.org)
transitions/color-transition-rounding.html bug 114182 (author: simon.fraser@apple.com)
transitions/cubic-bezier-overflow-svg-length.html bug 114183 (author: peter@chromium.org)
transitions/interrupt-zero-duration.html bug 114184 (authors: cmarrin@apple.com, rniwa@webkit.org, and simon.fraser@apple.com)
transitions/multiple-background-transitions.html bug 114185 (author: simon.fraser@apple.com)
transitions/cubic-bezier-overflow-color.html bug 114186 (author: peter@chromium.org)
transitions/mismatched-shadow-transitions.html bug 114188 (author: simon.fraser@apple.com)
transitions/color-transition-all.html bug 114189 (authors: ossy@webkit.org and simon.fraser@apple.com)
transitions/cubic-bezier-overflow-shadow.html bug 114191 (author: peter@chromium.org)
transitions/min-max-width-height-transitions.html bug 114192 (author: simon.fraser@apple.com)
transitions/cancel-transition.html bug 114193 (authors: ojan@chromium.org, rniwa@webkit.org, and simon.fraser@apple.com)
transitions/border-radius-transition.html bug 114194 (author: simon.fraser@apple.com)
transitions/flex-transitions.html bug 114195 (author: tony@chromium.org)
transitions/mixed-type.html bug 114196 (author: mikelawther@chromium.org)
transitions/multiple-mask-transitions.html bug 114197 (author: simon.fraser@apple.com)
transitions/color-transition-premultiplied.html bug 114198 (author: simon.fraser@apple.com)
transitions/mismatched-shadow-styles.html bug 114199 (author: simon.fraser@apple.com)
transitions/mask-transitions.html bug 114200 (authors: ojan@chromium.org, oliver@apple.com, and simon.fraser@apple.com)
transitions/cubic-bezier-overflow-length.html bug 114201 (author: peter@chromium.org)
transitions/multiple-background-size-transitions.html bug 114202 (authors: mitz@webkit.org and simon.fraser@apple.com)
transitions/clip-transition.html bug 114203 (authors: dglazkov@chromium.org, krit@webkit.org, and simon.fraser@apple.com)
transitions/cubic-bezier-overflow-transform.html bug 114204 (author: peter@chromium.org)
transitions/interrupted-accelerated-transition.html bug 56242 (authors: rniwa@webkit.org, simon.fraser@apple.com, and tonyg@chromium.org)
transitions/background-transitions.html bug 114206 (author: simon.fraser@apple.com)
http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html bug 114208 (authors: abarth@webkit.org and rniwa@webkit.org)
http/tests/inspector/inspect-element.html bug 78869 (author: pfeldman@chromium.org)
fast/repaint/table-cell-collapsed-border-scroll.html bug 117529 (author: robert@webkit.org)
fast/loader/javascript-url-in-object.html bug 114210 (authors: rniwa@webkit.org and sam@webkit.org)
platform/mac/editing/deleting/deletionUI-single-instance.html bug 114181 (author: rniwa@webkit.org)
The commit-queue is continuing to process your patch.
Comment 13 WebKit Commit Bot 2013-06-12 12:00:08 PDT
Comment on attachment 204464 [details]
Patch

Clearing flags on attachment: 204464

Committed r151517: <http://trac.webkit.org/changeset/151517>
Comment 14 WebKit Commit Bot 2013-06-12 12:00:12 PDT
All reviewed patches have been landed.  Closing bug.