Bug 27577 – [CSS3 Backgrounds and Borders] Add background-size to the background shorthand

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dave Hyatt]

background-size is supposed to be part of the background shorthand.  It appears after background-position followed by a '/'.

[Boris Zbarsky]

Note bug 8464.

[Boris Zbarsky]

And is this the bug tracking the fact that "background" doesn't reset the background-size?  This is causing people to write CSS like this:

  .foo {
    background-size: contain;
    height: 200px;
    background: url(something) no-repeat center center;
  }

that doesn't work in spec-compliant browsers but works in WebKit...

[Radar WebKit Bug Importer]

<rdar://problem/10218264>

[Joe Thomas]

Created attachment 139659 [details]
ProposedPatch

[Joe Thomas]

*** Bug 8464 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Comment on attachment 139659 [details]
ProposedPatch

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

> LayoutTests/fast/backgrounds/size/backgroundSize-in-background-shorthand.html:36
> +The background property is a shorthand property for setting most background properties at the same place in the style sheet. 
> +The number of comma-separated items defines the number of background layers.
> +Given a valid declaraion, for each layer the shorthand first sets the corresponding layer of each of background-image,
> +background-position, background-size, background-repeat, background-origin, background-clip and background-attachment
> +to that propertys initial value, then assigns any explicit values specified for this layer in the declaration. 
> +Finally background-color is set to the specified color, if any, else set to its initial value.
> +If one <box> value is present then it sets both background-origin and background-clip to that value. 
> +If two values are present, then the first sets background-origin and the second background-clip. 

Could we have just used manually sized divs instead of this wall of text?

[WebKit Review Bot]

Comment on attachment 139659 [details]
ProposedPatch

Attachment 139659 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12588649

New failing tests:
fast/inspector-support/style.html
inspector/styles/lazy-computed-style.html
inspector/styles/styles-new-API.html
inspector/styles/styles-computed-trace.html

[WebKit Review Bot]

Created attachment 139665 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Joe Thomas]

Created attachment 139667 [details]
Patch-Updated

Fixed Layout test failures.
Incorporated Eric's review comments

[Joe Thomas]

(In reply to comment #6)
> (From update of attachment 139659 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139659&action=review
> 
> > LayoutTests/fast/backgrounds/size/backgroundSize-in-background-shorthand.html:36
> 
> Could we have just used manually sized divs instead of this wall of text?


Thanks for the review. Patch looks much better now.

[Eric Seidel (no email)]

Comment on attachment 139667 [details]
Patch-Updated

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

Do you know who has recently been working on the shorthand stuff?  It seems they should be CC'd if they aren't already. I know we've had several patches in this area as of late.

> LayoutTests/fast/backgrounds/size/backgroundSize-in-background-shorthand.html:12
> +#size-cover
> +{
> +background: url('resources/flower.jpg') center / cover no-repeat;
> +}

It doesn't matter, but by putting these selectors all on one line, you could cut this file size in half w/o hurting readability. :)

#size-cover { background: url('resources/flower.jpg') center / cover no-repeat; }

It's also not clear to me if you don't just want to inline these on the div elements themselves. :)

[Joe Thomas]

Created attachment 139681 [details]
Patch-Updated

[Joe Thomas]

(In reply to comment #11)
> (From update of attachment 139667 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139667&action=review
> 
> Do you know who has recently been working on the shorthand stuff?  It seems they should be CC'd if they aren't already. I know we've had several patches in this area as of late.

CCed Antti Koivisto, Kling & rniwa, whom I could locate from recent patches.
> 
> > LayoutTests/fast/backgrounds/size/backgroundSize-in-background-shorthand.html:12
> > +#size-cover
> > +{
> > +background: url('resources/flower.jpg') center / cover no-repeat;
> > +}
> 
> It doesn't matter, but by putting these selectors all on one line, you could cut this file size in half w/o hurting readability. :)
> 
> #size-cover { background: url('resources/flower.jpg') center / cover no-repeat; }
> 
> It's also not clear to me if you don't just want to inline these on the div elements themselves. :)

Done.

[Alexis Menard (darktears)]

Comment on attachment 139681 [details]
Patch-Updated

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

In the same way you also should patch StylePropertySet::getPropertyValue, StylePropertyShorthand::backgroundShorthand() and CSSComputedStyleDeclaration and it would be nice to add a test case to cover these cases. You also want to test other values than initial (well maybe you did but it seemed there was a problem when you uploaded)

> LayoutTests/fast/backgrounds/size/backgroundSize-in-background-shorthand-expected.html:16
> +</html>

It doesn't seem to be the right file.

[Joe Thomas]

(In reply to comment #14)
> (From update of attachment 139681 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139681&action=review
> 
Thanks for the review.

> In the same way you also should patch StylePropertySet::getPropertyValue, StylePropertyShorthand::backgroundShorthand() and CSSComputedStyleDeclaration 
and it would be nice to add a test case to cover these cases. 

I will cover these cases.

You also want to test other values than initial (well maybe you did but it seemed there was a problem when you uploaded)
> 
Can you please let me know what is the problem that you are seeing? It looks fine to me.

> > LayoutTests/fast/backgrounds/size/backgroundSize-in-background-shorthand-expected.html:16
> > +</html>
> 
> It doesn't seem to be the right file.

This is a new reftest added by me. Do you mean to say that the path is wrong?

[Alexis Menard (darktears)]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 139681 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=139681&action=review
> > 
> Thanks for the review.
> 
> > In the same way you also should patch StylePropertySet::getPropertyValue, StylePropertyShorthand::backgroundShorthand() and CSSComputedStyleDeclaration 
> and it would be nice to add a test case to cover these cases. 
> 
> I will cover these cases.
> 
> You also want to test other values than initial (well maybe you did but it seemed there was a problem when you uploaded)
> > 
> Can you please let me know what is the problem that you are seeing? It looks fine to me.
> 
> > > LayoutTests/fast/backgrounds/size/backgroundSize-in-background-shorthand-expected.html:16
> > > +</html>
> > 
> > It doesn't seem to be the right file.
> 
> This is a new reftest added by me. Do you mean to say that the path is wrong?

Maybe you don't need a reftest. I simple JS based test like in fast/css/getComputedStyle/ is good enough. I feel this :

shouldBe("computedStyle.getPropertyValue('border-width')", "'10px 5px 4px 3px'");

is more easy to read and is more what you want to test.

[Joe Thomas]

Created attachment 140100 [details]
Patch-updated

Added the background-size property to StylePropertySet::getPropertyValue & StylePropertyShorthand::backgroundShorthand()

But I haven't added background-size to background shorthand property in CSSComputedStyleDeclaration. Tried to add / seperator between background-position and background-size in CSSComputedStyleDeclaration::getCSSPropertyValuesForShorthandProperties with 

 if (shorthand.properties()[i] == CSSPropertyBackgroundSize)
    list->append(cssValuePool().createValue("/", CSSPrimitiveValue::CSS_STRING));
    
This adds the seperator into the shorthand with single quotes, like 

rgb(255, 0, 0) url(dummy://test.png) repeat scroll 50px 50px '/' 50% 25%

The single quote is getting added to the separator in CSSPrimitiveValue::customCssText() with "quoteCSSStringIfNeeded(m_value.string)". 

Can someone please suggest a way to avoid this? Is it a good idea to create a PlainTextCSSValue class derived from CSSValue and return the string alone without any modification?

[Alexis Menard (darktears)]

(In reply to comment #17)
> Created an attachment (id=140100) [details]
> Patch-updated
> 
> Added the background-size property to StylePropertySet::getPropertyValue & StylePropertyShorthand::backgroundShorthand()
> 
> But I haven't added background-size to background shorthand property in CSSComputedStyleDeclaration. Tried to add / seperator between background-position and background-size in CSSComputedStyleDeclaration::getCSSPropertyValuesForShorthandProperties with 
> 
>  if (shorthand.properties()[i] == CSSPropertyBackgroundSize)
>     list->append(cssValuePool().createValue("/", CSSPrimitiveValue::CSS_STRING));
> 
> This adds the seperator into the shorthand with single quotes, like 
> 
> rgb(255, 0, 0) url(dummy://test.png) repeat scroll 50px 50px '/' 50% 25%
> 
> The single quote is getting added to the separator in CSSPrimitiveValue::customCssText() with "quoteCSSStringIfNeeded(m_value.string)". 
> 
> Can someone please suggest a way to avoid this? Is it a good idea to create a PlainTextCSSValue class derived from CSSValue and return the string alone without any modification?

You want to look at 
static PassRefPtr<CSSValueList> getBorderRadiusShorthandValue(const RenderStyle* style, RenderView* renderView)

We add a slash there.

[Joe Thomas]

Created attachment 140130 [details]
Patch-Updated

[Joe Thomas]

(In reply to comment #18)
> (In reply to comment #17)
> > Created an attachment (id=140100) [details] [details]
> > Patch-updated
> > 
> > 
> > Can someone please suggest a way to avoid this? Is it a good idea to create a PlainTextCSSValue class derived from CSSValue and return the string alone without any modification?
> 
> You want to look at 
> static PassRefPtr<CSSValueList> getBorderRadiusShorthandValue(const RenderStyle* style, RenderView* renderView)
> 
> We add a slash there.

Thanks very much :)

[Alexis Menard (darktears)]

Comment on attachment 140130 [details]
Patch-Updated

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

It's missing also tests for broken cases like for example : red url(dummy://test.png) / cover basically making sure you parse correctly or drop correctly the rule.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2701
> +    size_t count = 0;

unsigned is better, and call it i.

> Source/WebCore/css/CSSParser.cpp:2674
>                  }

I don't understand when the CSSPropertyBackgroundSize is added with addProperty. You should definitively follow the same pattern as the other cases.

> Source/WebCore/css/StylePropertySet.cpp:365
> +                if (!foundBackgroundPositionYCSSProperty && shorthand.properties()[j] == CSSPropertyBackgroundSize) 

What if the background-position is : 50px and not 50px 50px. background-shorthand-with-backgroundSize-style.html doesn't cover that case.

> LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style-expected.txt:11
> +PASS testPercentage.style.background is 'red url(dummy://test.png) no-repeat 50px 50px / 50% 75%'

Try also different background-position values.

> LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.html:6
> +<div id="test-percentage" style="background: red url(dummy://test.png) no-repeat 50px 50px / 50% 75%"></div>

Would you mind moving around the couple background-position / background-size so we can test better your change in parseFillShorthand. You always add them in the end, for example you could move it before the image. The order in the shorthand does not matter (as soon as the the position comes with the size).

[Alexis Menard (darktears)]

Comment on attachment 140130 [details]
Patch-Updated

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

Also add a test with a broken background-image value : e.g. / cover cover, / 3px cover

>> Source/WebCore/css/CSSParser.cpp:2674
>>                  }
> 
> I don't understand when the CSSPropertyBackgroundSize is added with addProperty. You should definitively follow the same pattern as the other cases.

Ok found it nevermind.

[Joe Thomas]

Created attachment 140226 [details]
Patch-with-More-testCases

[Joe Thomas]

(In reply to comment #21)
> (From update of attachment 140130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140130&action=review
> 
Thanks for the review.

> It's missing also tests for broken cases like for example : red url(dummy://test.png) / cover basically making sure you parse correctly or drop correctly the rule.
> 
It will drop the rule. Added testcase.

> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2701
> > +    size_t count = 0;
> 
> unsigned is better, and call it i.
> 
Done

> > Source/WebCore/css/CSSParser.cpp:2674
> >                  }
> 
> I don't understand when the CSSPropertyBackgroundSize is added with addProperty. You should definitively follow the same pattern as the other cases.
> 
> > Source/WebCore/css/StylePropertySet.cpp:365
> > +                if (!foundBackgroundPositionYCSSProperty && shorthand.properties()[j] == CSSPropertyBackgroundSize) 
> 
> What if the background-position is : 50px and not 50px 50px. background-shorthand-with-backgroundSize-style.html doesn't cover that case.
> 
Added testcase. backgroundPositionY will be 50%. Similar test case in LayoutTests/fast/css/getComputedStyle/getComputedStyle-background-position.html

> > LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style-expected.txt:11
> > +PASS testPercentage.style.background is 'red url(dummy://test.png) no-repeat 50px 50px / 50% 75%'
> 
> Try also different background-position values.

Done. Added test cases for background-position with top left, center, percentage and pixels

> 
> > LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.html:6
> > +<div id="test-percentage" style="background: red url(dummy://test.png) no-repeat 50px 50px / 50% 75%"></div>
> 
> Would you mind moving around the couple background-position / background-size so we can test better your change in parseFillShorthand. You always add them in the end, for example you could move it before the image. The order in the shorthand does not matter (as soon as the the position comes with the size).

Done.

[Joe Thomas]

(In reply to comment #22)
> (From update of attachment 140130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140130&action=review
> 
> Also add a test with a broken background-image value : e.g. / cover cover, / 3px cover
> 
Done. It will drop the rule.

[Alexis Menard (darktears)]

Comment on attachment 140226 [details]
Patch-with-More-testCases

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2708
> +    RefPtr<CSSValueList> listOnOrAfterBackgroundSize = CSSValueList::createSpaceSeparated();

The name is strange. what about backgroundSizeList.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2710
> +        RefPtr<CSSValue> value = getPropertyCSSValue(shorthand.properties()[i], DoNotUpdateLayout);

You don't need this for loop. Just call getPropertyCSSValue(CSSPropertyBackgroundSize, DoNotUpdateLayout); what else could be after the CSSPropertyBackgroundSize?

[Alexis Menard (darktears)]

Comment on attachment 140226 [details]
Patch-with-More-testCases

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2696
> +PassRefPtr<CSSValueList> CSSComputedStyleDeclaration::getBackgroundShorthandValue(const StylePropertyShorthand& shorthand) const

Also you are making a dedicated function now we don't need to pass a list. Just call getPropertyCSSValue on the properties you need. The array existed because we were using getCSSPropertyValuesForShorthandProperties.

[Joe Thomas]

Created attachment 140591 [details]
patch

Review comments from Alexis incorporated.

[Joe Thomas]

(In reply to comment #26)
> (From update of attachment 140226 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140226&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2708
> > +    RefPtr<CSSValueList> listOnOrAfterBackgroundSize = CSSValueList::createSpaceSeparated();
> 
> The name is strange. what about backgroundSizeList.
> 
I changed the names to better ones, please let me know if it is fine.
list -> backgroundShorthandList
listBeforeBackgroundSize -> list
listOnOrAfterBackgroundSize -> removed and added backgroundSizeValue

> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2710
> > +        RefPtr<CSSValue> value = getPropertyCSSValue(shorthand.properties()[i], DoNotUpdateLayout);
> 
> You don't need this for loop. Just call getPropertyCSSValue(CSSPropertyBackgroundSize, DoNotUpdateLayout); what else could be after the CSSPropertyBackgroundSize?

Currently background-clip and background-origin are not returned in getComputedStyle of background shorthand property. I am planning to add the support for them after this lands. Second 'For' loop was added so that I can add these missing properties towards the end of the array. 

I removed the second for loop from this patch. I will add background-clip and background-origin before background-position in the next patch.

[Alexis Menard (darktears)]

Comment on attachment 140591 [details]
patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2699
> +    const CSSPropertyID properties[5] = { CSSPropertyBackgroundColor, CSSPropertyBackgroundImage,

you can make that array static :D

[Andreas Kling]

Comment on attachment 140591 [details]
patch

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

This change looks wrong, it's adding an extra layer of indirection to the computed CSSValue, turning this:
{ color, image, repeat, attachment, position }
Into this:
{ { color, image, repeat, attachment, position }, size }

Is this behavior change correct?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2709
> +    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
> +    for (unsigned i = 0; i < WTF_ARRAY_LENGTH(properties); ++i) {
> +        RefPtr<CSSValue> value = getPropertyCSSValue(properties[i], DoNotUpdateLayout);
> +        list->append(value);
> +    }
> +    backgroundShorthandList->append(list);

This looks like a copy-paste of getCSSPropertyValuesForShorthandProperties(). Better to just call it.

[Joe Thomas]

(In reply to comment #31)
> (From update of attachment 140591 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140591&action=review
> 
> This change looks wrong, it's adding an extra layer of indirection to the computed CSSValue, turning this:
> { color, image, repeat, attachment, position }
> Into this:
> { { color, image, repeat, attachment, position }, size }
> 
> Is this behavior change correct?

The spec http://www.w3.org/TR/css3-background/ does not mention anything about the internal representation of computed CSS value. Do we have any other spec that I can refer to?

The same kind of indirection we are adding in case of border-radius shorthand value to add a '/' between horizontal and vertical radii in getBorderRadiusShorthandValue.

Please let me know if we need to avoid this. Also see comment #17.

[Joe Thomas]

(In reply to comment #32)
> (In reply to comment #31)
> > (From update of attachment 140591 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=140591&action=review
> > 
> > This change looks wrong, it's adding an extra layer of indirection to the computed CSSValue, turning this:
> > { color, image, repeat, attachment, position }
> > Into this:
> > { { color, image, repeat, attachment, position }, size }
> > 
> > Is this behavior change correct?
> 
> The spec http://www.w3.org/TR/css3-background/ does not mention anything about the internal representation of computed CSS value. Do we have any other spec that I can refer to?
> 
> The same kind of indirection we are adding in case of border-radius shorthand value to add a '/' between horizontal and vertical radii in getBorderRadiusShorthandValue.
> 
> Please let me know if we need to avoid this. Also see comment #17.

It is not so important the internal representation of computed CSSValue of shorthand properties as getPropertyCSSValue of shorthand property is expected to return NULL. As per http://www.w3.org/TR/DOM-Level-2-Style/css.html, Shorthand property values can only be accessed and modified as strings using the getPropertyValue and setProperty methods.

Paraphrasing from http://www.w3.org/TR/DOM-Level-2-Style/css.html:

"getPropertyCSSValue: Used to retrieve the object representation of the value of a CSS property if it has been explicitly set within this declaration block. This method returns null if the property is a shorthand property. Shorthand property values can only be accessed and modified as strings, using the getPropertyValue and setProperty methods."

[Joe Thomas]

Created attachment 140774 [details]
Patch-Updated

Review comments from Alexis and Kling incorporated.

[Darin Adler]

Comment on attachment 140774 [details]
Patch-Updated

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2349
>          case CSSPropertyBackground: {
> -            const CSSPropertyID properties[5] = { CSSPropertyBackgroundColor, CSSPropertyBackgroundImage,
> -                                        CSSPropertyBackgroundRepeat, CSSPropertyBackgroundAttachment,
> -                                        CSSPropertyBackgroundPosition };
> -            return getCSSPropertyValuesForShorthandProperties(StylePropertyShorthand(properties, WTF_ARRAY_LENGTH(properties)));
> +
> +            return getBackgroundShorthandValue();
>          }

Should get rid of the braces and blank line here.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2703
> +    RefPtr<CSSValueList> backgroundShorthandList = CSSValueList::createSlashSeparated();

I’d just call this local variable “list”. I don’t think the longer name helps readability.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2705
> +    // Now append CSSPropertyBackgroundSize value to the backgroundShorthandList.

This is not a helpful comment. It says the same thing as the next line.

> Source/WebCore/css/CSSParser.cpp:2313
> +        return parseFillShorthand(propId, properties, 8, important);

It would be better to not have the number 8 hard-coded here. We should use WTF_ARRAY_LENGTH.

[Joe Thomas]

Created attachment 140821 [details]
Patch

Incorporated Darin's review comments.

[Joe Thomas]

(In reply to comment #35)
> (From update of attachment 140774 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140774&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2349
> >          case CSSPropertyBackground: {
> > -            const CSSPropertyID properties[5] = { CSSPropertyBackgroundColor, CSSPropertyBackgroundImage,
> > -                                        CSSPropertyBackgroundRepeat, CSSPropertyBackgroundAttachment,
> > -                                        CSSPropertyBackgroundPosition };
> > -            return getCSSPropertyValuesForShorthandProperties(StylePropertyShorthand(properties, WTF_ARRAY_LENGTH(properties)));
> > +
> > +            return getBackgroundShorthandValue();
> >          }
> 
> Should get rid of the braces and blank line here.
Done
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2703
> > +    RefPtr<CSSValueList> backgroundShorthandList = CSSValueList::createSlashSeparated();
> 
> I’d just call this local variable “list”. I don’t think the longer name helps readability.
> 
Changed.
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2705
> > +    // Now append CSSPropertyBackgroundSize value to the backgroundShorthandList.
> 
> This is not a helpful comment. It says the same thing as the next line.
> 
Removed
> > Source/WebCore/css/CSSParser.cpp:2313
> > +        return parseFillShorthand(propId, properties, 8, important);
> 
> It would be better to not have the number 8 hard-coded here. We should use WTF_ARRAY_LENGTH.
Done

[Alexis Menard (darktears)]

Comment on attachment 140821 [details]
Patch

r=me. It is very unlikely that code would rely on the internal representation of the shorthand. In addition, the support of the shorthand property in getComputedStyle was added by myself 3-4 months ago (before NULL was returned) so the behavior change happening in this patch would mean that the breakage would be for people relying on this internal representation (which is not recommended by the spec) in the last 3-4 months. Only the last version of Chrome (18) ships the support of the background shorthand in general so it is fine. I agree with kling that the internal representation is a bit cumbersome, and it could be fixed in another patch by supporting creating CSSValueList with mixed separators (without increasing the number of items of the list) which is probably possible today. But what this patch fix and is very welcome is the support of the background-size in the shorthand which is a long standing bug (Opera and FF are supporting this feature).

[WebKit Review Bot]

Comment on attachment 140821 [details]
Patch

Clearing flags on attachment: 140821

Committed r116645: <http://trac.webkit.org/changeset/116645>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This caused a regression, bug 86880.

[Ryosuke Niwa]

As I commented on the other bug, we should make sure shorthand notations we generate can be parsed properly by old UAs like IE6. If not, we have to use a different function for editing.

[Alexis Menard (darktears)]

(In reply to comment #42)
> As I commented on the other bug, we should make sure shorthand notations we generate can be parsed properly by old UAs like IE6. If not, we have to use a different function for editing.

It's not the problem on #86880, it simply rely on undefined behavior.

[Alexey Proskuryakov]

This is questioned again in bug 97761