Bug 27577 - [CSS3 Backgrounds and Borders] Add background-size to the background shorthand
Summary: [CSS3 Backgrounds and Borders] Add background-size to the background shorthand
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Joe Thomas
URL:
Keywords: InRadar
: 8464 (view as bug list)
Depends on:
Blocks: 27569
  Show dependency treegraph
 
Reported: 2009-07-22 15:48 PDT by Dave Hyatt
Modified: 2012-09-27 10:42 PDT (History)
17 users (show)

See Also:


Attachments
ProposedPatch (14.97 KB, patch)
2012-05-01 12:45 PDT, Joe Thomas
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (5.67 MB, application/zip)
2012-05-01 13:25 PDT, WebKit Review Bot
no flags Details
Patch-Updated (11.42 KB, patch)
2012-05-01 13:43 PDT, Joe Thomas
no flags Details | Formatted Diff | Diff
Patch-Updated (11.28 KB, patch)
2012-05-01 14:28 PDT, Joe Thomas
menard: review-
Details | Formatted Diff | Diff
Patch-updated (18.14 KB, patch)
2012-05-03 14:54 PDT, Joe Thomas
no flags Details | Formatted Diff | Diff
Patch-Updated (68.35 KB, patch)
2012-05-03 17:20 PDT, Joe Thomas
menard: review-
menard: commit-queue-
Details | Formatted Diff | Diff
Patch-with-More-testCases (70.86 KB, patch)
2012-05-04 07:57 PDT, Joe Thomas
menard: review-
menard: commit-queue-
Details | Formatted Diff | Diff
patch (69.92 KB, patch)
2012-05-07 14:13 PDT, Joe Thomas
no flags Details | Formatted Diff | Diff
Patch-Updated (69.72 KB, patch)
2012-05-08 13:55 PDT, Joe Thomas
no flags Details | Formatted Diff | Diff
Patch (69.76 KB, patch)
2012-05-08 17:00 PDT, Joe Thomas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2009-07-22 15:48:26 PDT
background-size is supposed to be part of the background shorthand.  It appears after background-position followed by a '/'.
Comment 1 Boris Zbarsky 2011-09-30 13:55:46 PDT
Note bug 8464.
Comment 2 Boris Zbarsky 2011-09-30 13:57:02 PDT
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...
Comment 3 Radar WebKit Bug Importer 2011-09-30 16:18:31 PDT
<rdar://problem/10218264>
Comment 4 Joe Thomas 2012-05-01 12:45:11 PDT
Created attachment 139659 [details]
ProposedPatch
Comment 5 Joe Thomas 2012-05-01 12:52:35 PDT
*** Bug 8464 has been marked as a duplicate of this bug. ***
Comment 6 Eric Seidel (no email) 2012-05-01 13:17:33 PDT
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 7 WebKit Review Bot 2012-05-01 13:25:38 PDT
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
Comment 8 WebKit Review Bot 2012-05-01 13:25:45 PDT
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
Comment 9 Joe Thomas 2012-05-01 13:43:41 PDT
Created attachment 139667 [details]
Patch-Updated

Fixed Layout test failures.
Incorporated Eric's review comments
Comment 10 Joe Thomas 2012-05-01 13:44:50 PDT
(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 11 Eric Seidel (no email) 2012-05-01 13:52:43 PDT
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. :)
Comment 12 Joe Thomas 2012-05-01 14:28:22 PDT
Created attachment 139681 [details]
Patch-Updated
Comment 13 Joe Thomas 2012-05-01 14:32:23 PDT
(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 14 Alexis Menard (darktears) 2012-05-02 04:14:45 PDT
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.
Comment 15 Joe Thomas 2012-05-02 04:50:33 PDT
(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?
Comment 16 Alexis Menard (darktears) 2012-05-02 05:14:01 PDT
(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.
Comment 17 Joe Thomas 2012-05-03 14:54:03 PDT
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?
Comment 18 Alexis Menard (darktears) 2012-05-03 14:58:56 PDT
(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.
Comment 19 Joe Thomas 2012-05-03 17:20:56 PDT
Created attachment 140130 [details]
Patch-Updated
Comment 20 Joe Thomas 2012-05-03 17:22:47 PDT
(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 21 Alexis Menard (darktears) 2012-05-04 05:41:37 PDT
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 22 Alexis Menard (darktears) 2012-05-04 05:52:47 PDT
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.
Comment 23 Joe Thomas 2012-05-04 07:57:12 PDT
Created attachment 140226 [details]
Patch-with-More-testCases
Comment 24 Joe Thomas 2012-05-04 08:04:13 PDT
(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.
Comment 25 Joe Thomas 2012-05-04 08:06:06 PDT
(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 26 Alexis Menard (darktears) 2012-05-07 11:07:30 PDT
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 27 Alexis Menard (darktears) 2012-05-07 11:10:32 PDT
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.
Comment 28 Joe Thomas 2012-05-07 14:13:55 PDT
Created attachment 140591 [details]
patch

Review comments from Alexis incorporated.
Comment 29 Joe Thomas 2012-05-07 14:20:31 PDT
(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 30 Alexis Menard (darktears) 2012-05-08 07:30:10 PDT
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 31 Andreas Kling 2012-05-08 07:46:02 PDT
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.
Comment 32 Joe Thomas 2012-05-08 08:12:51 PDT
(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.
Comment 33 Joe Thomas 2012-05-08 13:39:06 PDT
(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."
Comment 34 Joe Thomas 2012-05-08 13:55:58 PDT
Created attachment 140774 [details]
Patch-Updated

Review comments from Alexis and Kling incorporated.
Comment 35 Darin Adler 2012-05-08 16:44:08 PDT
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.
Comment 36 Joe Thomas 2012-05-08 17:00:00 PDT
Created attachment 140821 [details]
Patch

Incorporated Darin's review comments.
Comment 37 Joe Thomas 2012-05-08 17:03:40 PDT
(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 38 Alexis Menard (darktears) 2012-05-10 07:02:48 PDT
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 39 WebKit Review Bot 2012-05-10 08:09:28 PDT
Comment on attachment 140821 [details]
Patch

Clearing flags on attachment: 140821

Committed r116645: <http://trac.webkit.org/changeset/116645>
Comment 40 WebKit Review Bot 2012-05-10 08:09:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 41 Alexey Proskuryakov 2012-05-21 13:14:39 PDT
This caused a regression, bug 86880.
Comment 42 Ryosuke Niwa 2012-05-21 13:49:47 PDT
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.
Comment 43 Alexis Menard (darktears) 2012-05-21 13:57:47 PDT
(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.
Comment 44 Alexey Proskuryakov 2012-09-27 10:42:09 PDT
This is questioned again in bug 97761