Bug 38354 – Support for Percentage Values in border-radius

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

[Divya Manian]

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

[Rob Buis]

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.

[Rob Buis]

Created attachment 64895 [details]
Improved patch

[WebKit Review Bot]

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.

[Rob Buis]

(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.

[Rob Buis]

Created attachment 64930 [details]
Fixed style issues

[Rob Buis]

Created attachment 65066 [details]
Patch

[Rob Buis]

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.

[Darin Adler]

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

[Rob Buis]

This was landed in 66615 after incorporating Darin's suggestions.

[Peter Kasting]

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?

[Rob Buis]

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.

[Peter Kasting]

(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.

[Csaba Osztrogonác]

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.