Bug 75434 - getComputedStyle should return shorthands properties with the minimum number of sides possible.
: getComputedStyle should return shorthands properties with the minimum number ...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Alexis Menard (darktears)
:
Depends on:
Blocks: 13658
  Show dependency treegraph
 
Reported: 2012-01-02 05:35 PST by Alexis Menard (darktears)
Modified: 2012-01-03 11:57 PST (History)
7 users (show)

See Also:


Attachments
Patch (34.43 KB, patch)
2012-01-02 05:38 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (34.66 KB, patch)
2012-01-03 10:35 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 2012-01-02 05:35:18 PST
getComputedStyle should return shorthands property with the minimum number of sides possible.
Comment 1 Alexis Menard (darktears) 2012-01-02 05:38:43 PST
Created attachment 120873 [details]
Patch
Comment 2 Tony Chang 2012-01-03 10:14:05 PST
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
Comment 3 Alexis Menard (darktears) 2012-01-03 10:16:39 PST
(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.
Comment 4 Alexis Menard (darktears) 2012-01-03 10:35:07 PST
Created attachment 120957 [details]
Patch for landing
Comment 5 WebKit Review Bot 2012-01-03 11:57:37 PST
Comment on attachment 120957 [details]
Patch for landing

Clearing flags on attachment: 120957

Committed r103953: <http://trac.webkit.org/changeset/103953>
Comment 6 WebKit Review Bot 2012-01-03 11:57:42 PST
All reviewed patches have been landed.  Closing bug.