Bug 38354 - Support for Percentage Values in border-radius
Summary: Support for Percentage Values in border-radius
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-29 15:03 PDT by Divya Manian
Modified: 2013-09-17 14:13 PDT (History)
7 users (show)

See Also:


Attachments
A demonstration of border-radius with % units. (525 bytes, text/html)
2010-04-29 15:03 PDT, Divya Manian
no flags Details
Work in progress patch (16.61 KB, patch)
2010-08-16 12:49 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Improved patch (25.40 KB, patch)
2010-08-19 13:41 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Fixed style issues (25.39 KB, patch)
2010-08-20 00:10 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (25.51 KB, patch)
2010-08-22 13:13 PDT, Rob Buis
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Divya Manian 2010-04-29 15:03:38 PDT
Created attachment 54740 [details]
A demonstration of border-radius with % units. 

The CSS3 Specification has support for percentage values in border-radius property [1] but webkit has not supported it yet. 

I am attaching a demonstration page which will render in Firefox 3.5, and Opera 10.5 but not in Webkit nightlies. 


[1] http://www.w3.org/TR/css3-background/#the-border-radius
Comment 1 Rob Buis 2010-08-16 12:49:04 PDT
Created attachment 64510 [details]
Work in progress patch

So this is a work in progress patch for this bug. It actually fixes the testcase! But it needs tests at least, I just
implemented from what I could figure out from the spec and how FF supports the percentages. The only part
of the patch that I know is wrong is the RenderObject.cpp change, I am not sure what values to use there in
the percentage case. Still I thought I'd put it up for early feedback to see whether I am on the right track.
Cheers,

Rob.
Comment 2 Rob Buis 2010-08-19 13:41:17 PDT
Created attachment 64895 [details]
Improved patch
Comment 3 WebKit Review Bot 2010-08-19 13:42:59 PDT
Attachment 64895 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSStyleSelector.cpp:4797:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/css/CSSStyleSelector.cpp:4799:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/css/CSSComputedStyleDeclaration.cpp:391:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Rob Buis 2010-08-19 13:56:36 PDT
(In reply to comment #2)
> Created an attachment (id=64895) [details]
> Improved patch

Even though the patch currently has style issues, I think the bigger question is whether the approach
is right, so I decided to upload it anyway :)
Cheers,

Rob.
Comment 5 Rob Buis 2010-08-20 00:10:07 PDT
Created attachment 64930 [details]
Fixed style issues
Comment 6 Rob Buis 2010-08-22 13:13:06 PDT
Created attachment 65066 [details]
Patch
Comment 7 Rob Buis 2010-08-22 13:15:39 PDT
With my latest patch up, I basically changed two things. I realized calcValue(100) was cryptic and not needed, rawValue can be used as well and is quicker. The second thing is that I noticed the pixel test for border-radius-huge-assert.html was failing. It turns out that the used valu is too big for the 28-bit limit of Length, so I needed to clamp there to the 28-bit limits like in CSSPrimitiveValue, now there are no regressions anymore.
Cheers,

Rob.
Comment 8 Darin Adler 2010-08-29 13:10:45 PDT
Comment on attachment 65066 [details]
Patch

> +        const int intMaxForLength = 0x7ffffff; // max value for a 28-bit int
> +        const int intMinForLength = (-0x7ffffff - 1); // min value for a 28-bit int

OK, but where does "28-bit" come from? I presume from Length. These constants should be in Length.h, not here inline in the code.

> +    BorderData() : m_topLeft(Length(0, Fixed), Length(0, Fixed)), m_topRight(Length(0, Fixed), Length(0, Fixed))
> +                 , m_bottomLeft(Length(0, Fixed), Length(0, Fixed)), m_bottomRight(Length(0, Fixed), Length(0, Fixed)) {}

This is not the correct formatting. Our style guide has a specific format for this:

    BorderData()
        : m_topLeft(Length(0, Fixed), Length(0, Fixed))
        , m_topRight(Length(0, Fixed), Length(0, Fixed))
        , m_bottomLeft(Length(0, Fixed), Length(0, Fixed))
        , m_bottomRight(Length(0, Fixed), Length(0, Fixed))
    {
    }

r=me, but please consider fixing the two issues above
Comment 9 Rob Buis 2010-09-02 10:16:59 PDT
This was landed in 66615 after incorporating Darin's suggestions.
Comment 10 Peter Kasting 2010-09-02 18:02:17 PDT
Rob, any reason you manually clamped against intMin/MaxForLength instead of using cumputeLengthIntForLength()?  If the problem is that that function returns 0 for values outside the bounds, perhaps there are other problems that causes, and we should simply fix the function?
Comment 11 Rob Buis 2010-09-03 02:11:41 PDT
Hello Peter,

(In reply to comment #10)
> Rob, any reason you manually clamped against intMin/MaxForLength instead of using cumputeLengthIntForLength()?  If the problem is that that function returns 0 for values outside the bounds, perhaps there are other problems that causes, and we should simply fix the function?

You are right, cumputeLengthIntForLength returning 0 was the problem in my use case, since I need real clamping there. I'll add that I was not feeling that great about the solution in my patch.

Do you mean problems before or after my patch?

One solution may be to add an extra boolean param to cumputeLengthIntForLength that toggles the clamping behaviour. Or do we need a new function for the behaviour I want for the border-radius case? Let me know what you think.
Cheers,

Rob.
Comment 12 Peter Kasting 2010-09-03 10:44:53 PDT
(In reply to comment #11)
> Do you mean problems before or after my patch?

Before -- I'm wondering if returning 0 in this case is just always wrong.

> One solution may be to add an extra boolean param to cumputeLengthIntForLength that toggles the clamping behaviour. Or do we need a new function for the behaviour I want for the border-radius case? Let me know what you think.

If at all possible, I'd like to use the same behavior everywhere.  Clamping makes far more sense to me than returning 0 for out-of-bounds, so I'm hoping we can just do that.  Only if we truly need both behaviors would I want to make things toggleable.

I'll file a new bug about this stuff and CC you.
Comment 13 Csaba Osztrogonác 2013-09-17 14:13:01 PDT
Comment on attachment 65066 [details]
Patch

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

> WebCore/css/CSSParser.cpp:1227
>      case CSSPropertyOutlineOffset:
> -        validPrimitive = validUnit(value, FLength, m_strict);
> +        validPrimitive = validUnit(value, FLength | FPercent, m_strict);

It is a so ugly typo and was a hidden bug for 3 years ...
but Reni's ultimate fuzzer caught it. :)

See https://bugs.webkit.org/show_bug.cgi?id=120469 for details and fix.