Summary: | Reading border radius from style property returns in wrong order. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jared Wyles <wyles> | ||||||||||||||
Component: | New Bugs | Assignee: | 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
Jared Wyles
2013-02-26 00:19:22 PST
Created attachment 190220 [details]
Patch
Reproduction steps are: var myDiv = document.createElement("div"); myDiv.style.borderRadius = "1px 2px 3px 4px"; document.write(myDiv.style.borderRadius); 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 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 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 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 Created attachment 190332 [details]
Patch
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 Created attachment 190364 [details]
Patch
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. Created attachment 190425 [details]
Patch
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? (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. (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)) As mentioned in irc. While this is possible to break something. This is also inconsistent for people using getComputedStyle in webkit browsers. 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.
Created attachment 192108 [details]
Patch
Created attachment 192113 [details]
Patch
Comment on attachment 192113 [details] Patch Clearing flags on attachment: 192113 Committed r145172: <http://trac.webkit.org/changeset/145172> All reviewed patches have been landed. Closing bug. |