Summary: | getComputedStyle should return shorthands properties with the minimum number of sides possible. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexis Menard (darktears) <menard> | ||||||
Component: | CSS | Assignee: | Alexis Menard (darktears) <menard> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, kling, koivisto, macpherson, simon.fraser, tony, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 13658 | ||||||||
Attachments: |
|
Description
Alexis Menard (darktears)
2012-01-02 05:35:18 PST
Created attachment 120873 [details]
Patch
Comment on attachment 120873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120873&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2419 > +PassRefPtr<CSSValueList> CSSComputedStyleDeclaration::getCSSProperty4ValuesForShorthandProperties(const int* properties) const > +{ > + RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated(); > + // Assume the properties are in the usual order top, right, bottom, left. I would try to make the name more clear that this function is for properties that apply to each direction/side. For example, getCSSPropertyValuesForSidesShorthand or something. > LayoutTests/fast/css/getComputedStyle/getComputedStyle-border-width-expected.txt:24 > +PASS computedStyle.getPropertyCSSValue('border-width').cssText is '10px 0px 0px' In a follow up patch, maybe we should change 0px to 0 since the unit identifier is optional on zero length units. http://www.w3.org/TR/css3-values/#length-value (In reply to comment #2) > (From update of attachment 120873 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120873&action=review > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2419 > > +PassRefPtr<CSSValueList> CSSComputedStyleDeclaration::getCSSProperty4ValuesForShorthandProperties(const int* properties) const > > +{ > > + RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated(); > > + // Assume the properties are in the usual order top, right, bottom, left. > > I would try to make the name more clear that this function is for properties that apply to each direction/side. For example, getCSSPropertyValuesForSidesShorthand or something. Yes I ran out of imagination on that one :(. I like your suggestion. > > > LayoutTests/fast/css/getComputedStyle/getComputedStyle-border-width-expected.txt:24 > > +PASS computedStyle.getPropertyCSSValue('border-width').cssText is '10px 0px 0px' > > In a follow up patch, maybe we should change 0px to 0 since the unit identifier is optional on zero length units. http://www.w3.org/TR/css3-values/#length-value I'll look at that one. Created attachment 120957 [details]
Patch for landing
Comment on attachment 120957 [details] Patch for landing Clearing flags on attachment: 120957 Committed r103953: <http://trac.webkit.org/changeset/103953> All reviewed patches have been landed. Closing bug. |