Bug 27577

Note bug 8464.

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...

<rdar://problem/10218264>

Created attachment 139659 [details] ProposedPatch

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

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?

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

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

Created attachment 139667 [details] Patch-Updated Fixed Layout test failures. Incorporated Eric's review comments

(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.

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. :)

Created attachment 139681 [details] Patch-Updated

(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.

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.

(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?

(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.

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?

(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.

Created attachment 140130 [details] Patch-Updated

(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 :)

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).

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.

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

(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.

(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.

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?

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.

Created attachment 140591 [details] patch Review comments from Alexis incorporated.

(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.

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

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.

(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.

(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."

Created attachment 140774 [details] Patch-Updated Review comments from Alexis and Kling incorporated.

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.

Created attachment 140821 [details] Patch Incorporated Darin's review comments.

(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

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).

Comment on attachment 140821 [details] Patch Clearing flags on attachment: 140821 Committed r116645: <http://trac.webkit.org/changeset/116645>

All reviewed patches have been landed. Closing bug.

This caused a regression, bug 86880.

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.

(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.

This is questioned again in bug 97761