Bug 110853

Summary: Reading border radius from style property returns in wrong order.
Product: WebKit Reporter: Jared Wyles <wyles>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, dglazkov, esprehn+autocc, macpherson, menard, ojan.autocc, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Jared Wyles 2013-02-26 00:19:22 PST
Reading border radius from style property returns in wrong order.
Comment 1 Jared Wyles 2013-02-26 00:23:49 PST
Created attachment 190220 [details]
Patch
Comment 2 Jared Wyles 2013-02-26 00:34:48 PST
Reproduction steps are:

 var myDiv = document.createElement("div");
 myDiv.style.borderRadius = "1px 2px 3px 4px";
 document.write(myDiv.style.borderRadius);
Comment 3 WebKit Review Bot 2013-02-26 02:06:25 PST
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.
Comment 4 Build Bot 2013-02-26 04:59:29 PST
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
Comment 5 WebKit Review Bot 2013-02-26 07:11:58 PST
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
Comment 6 WebKit Review Bot 2013-02-26 08:16:24 PST
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
Comment 7 Jared Wyles 2013-02-26 11:27:19 PST
Created attachment 190332 [details]
Patch
Comment 8 Build Bot 2013-02-26 13:47:08 PST
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
Comment 9 Jared Wyles 2013-02-26 14:42:03 PST
Created attachment 190364 [details]
Patch
Comment 10 Ryosuke Niwa 2013-02-26 19:14:38 PST
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.
Comment 11 Jared Wyles 2013-02-26 19:52:29 PST
Created attachment 190425 [details]
Patch
Comment 12 Jared Wyles 2013-02-26 19:56:59 PST
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?
Comment 13 Ryosuke Niwa 2013-02-28 15:46:29 PST
(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.
Comment 14 Jared Wyles 2013-02-28 19:07:12 PST
(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))
Comment 15 Jared Wyles 2013-03-07 16:44:12 PST
As mentioned in irc. While this is possible to break something. This is also inconsistent for people using getComputedStyle in webkit browsers.
Comment 16 Ryosuke Niwa 2013-03-07 16:44:20 PST
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.
Comment 17 Jared Wyles 2013-03-07 16:55:45 PST
Created attachment 192108 [details]
Patch
Comment 18 Jared Wyles 2013-03-07 17:03:05 PST
Created attachment 192113 [details]
Patch
Comment 19 WebKit Review Bot 2013-03-07 19:54:16 PST
Comment on attachment 192113 [details]
Patch

Clearing flags on attachment: 192113

Committed r145172: <http://trac.webkit.org/changeset/145172>
Comment 20 WebKit Review Bot 2013-03-07 19:54:20 PST
All reviewed patches have been landed.  Closing bug.