Bug 110853 – Reading border radius from style property returns in wrong order.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jared Wyles]

Reading border radius from style property returns in wrong order.

[Jared Wyles]

Created attachment 190220 [details]
Patch

[Jared Wyles]

Reproduction steps are:

 var myDiv = document.createElement("div");
 myDiv.style.borderRadius = "1px 2px 3px 4px";
 document.write(myDiv.style.borderRadius);

[WebKit Review Bot]

Attachment 190220 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/borders/border-radius-parsing-expected.txt', u'LayoutTests/fast/borders/border-radius-parsing.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/StylePropertyShorthand.cpp']" exit_code: 1
LayoutTests/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 190220 [details]
Patch

Attachment 190220 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16757978

New failing tests:
inspector/elements/elements-panel-styles.html

[WebKit Review Bot]

Comment on attachment 190220 [details]
Patch

Attachment 190220 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16779058

New failing tests:
inspector/elements/elements-panel-styles.html

[WebKit Review Bot]

Comment on attachment 190220 [details]
Patch

Attachment 190220 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16776190

New failing tests:
inspector/elements/elements-panel-styles.html

[Jared Wyles]

Created attachment 190332 [details]
Patch

[Build Bot]

Comment on attachment 190332 [details]
Patch

Attachment 190332 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16782227

New failing tests:
inspector/elements/elements-panel-styles.html

[Jared Wyles]

Created attachment 190364 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 190364 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Please mention that you've updated an existing test and its name.

> Source/WebCore/ChangeLog:11
> +        Updating the order of border-radius to return in the order specified
> +        in http://www.w3.org/TR/css3-background/#the-border-radius

This should appear before the explanation about the tests.

> Source/WebCore/css/StylePropertyShorthand.cpp:131
> -        CSSPropertyBorderTopRightRadius,
>          CSSPropertyBorderTopLeftRadius,
> -        CSSPropertyBorderBottomLeftRadius,
> -        CSSPropertyBorderBottomRightRadius
> +        CSSPropertyBorderTopRightRadius,
> +        CSSPropertyBorderBottomRightRadius,
> +        CSSPropertyBorderBottomLeftRadius

Are there any compat. concerns? Specifically, is it likely that there are Web contents out there that depend on our current behavior?
It doesn't seem far fetched to think there are some mobile content that specifically target WebKit that depends on this behavior.

[Jared Wyles]

Created attachment 190425 [details]
Patch

[Jared Wyles]

Updated the patch to address your comments. 

I am not too sure about compatibility concerns. Anyone attempting to read a cssstylerule should hopefully be using getComputedStyles which behaves as expected. It would have been pretty obvious to anyone relying on this behaviour early on that it was returning out of order (i hope).

I am a new to this process, should i just browser things like like jquery mobile, kendo-ui and other such mobile frameworks to see if i can find any evidence of it being used and relied on?

[Ryosuke Niwa]

(In reply to comment #12)
>
> I am a new to this process, should i just browser things like like jquery mobile, kendo-ui and other such mobile frameworks to see if i can find any evidence of it being used and relied on?

That would be helpful.

[Jared Wyles]

(In reply to comment #13)
> (In reply to comment #12)
> >
> > I am a new to this process, should i just browser things like like jquery mobile, kendo-ui and other such mobile frameworks to see if i can find any evidence of it being used and relied on?
> 
> That would be helpful.

Had a look through jquery-mobile it depends on jquery for all its css. Uses the .css function, which under calls to a function like..
getStyles = function( elem ) {
		return window.getComputedStyle( elem, null );
}; 

So good there, i had a quick look at kendo-ui, it also has a hard dependency on jquery so they are using the same approach. 

There may be a small problem with anyone using zepto as it seems to favour using .style over getComputedStyle.   

 css: function(property, value){
      if (value === undefined && typeof property == 'string')
        return (
          this.length == 0
            ? undefined
            : this[0].style[camelize(property)] || getComputedStyle(this[0], '').getPropertyValue(property))

[Jared Wyles]

As mentioned in irc. While this is possible to break something. This is also inconsistent for people using getComputedStyle in webkit browsers.

[Ryosuke Niwa]

Comment on attachment 190425 [details]
Patch

Per IRC discussion, we're even inconsistent with getComputedStyle so fixing this ordering seems like the right thing to do.
Please put relevant compatibility information in the change log.

[Jared Wyles]

Created attachment 192108 [details]
Patch

[Jared Wyles]

Created attachment 192113 [details]
Patch

[WebKit Review Bot]

Comment on attachment 192113 [details]
Patch

Clearing flags on attachment: 192113

Committed r145172: <http://trac.webkit.org/changeset/145172>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.