background-size specified by a single value in background shorthand fails to parse

• Component: CSS
• Status: RESOLVED FIXED

[Joe Thomas]

Created attachment 142479 [details]
reduction

If the background-size is specified by a single value in background shorthand property other than cover or contain, it fails to parse.

Examples:

background: url(dummy://test.png) center / auto padding-box no-repeat
background: url(dummy://test.png) center / 20px padding-box no-repeat
background: url(dummy://test.png) center / 30% padding-box no-repeat

[Joe Thomas]

Created attachment 142539 [details]
ProposedPatch

[Tony Chang]

Comment on attachment 142539 [details]
ProposedPatch

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

> Source/WebCore/css/CSSParser.cpp:3385
> +                    // If we're not parsing a shorthand then we are invalid.

I would remove this comment.  It's just restating what the code is doing.

> Source/WebCore/css/CSSParser.cpp:3394
>      } else if (!parsedValue2 && propId == CSSPropertyWebkitBackgroundSize) {
>          // For backwards compatibility we set the second value to the first if it is omitted.

It looks like the comment here is trying to handle this case.  Should we be hitting this code instead?

> LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.html:-7
> -<div id="test-pixels" style="background: red url(dummy://test.png) repeat top left / 100px 200px border-box padding-box"></div>

This test case appears to have changed to border-box border-box.  Was that intentional?

> LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.html:-8
> -<div id="test-auto" style="background: red url(dummy://test.png) repeat 50% / auto auto border-box content-box"></div>

This too.  FWIW, it was hard to tell which test cases you added from the diff.  If you wanted to change the test, you could have done that in a separate refactor change before this bug fix.

[Joe Thomas]

(In reply to comment #2)
> (From update of attachment 142539 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142539&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:3385
> > +                    // If we're not parsing a shorthand then we are invalid.
> 
> I would remove this comment.  It's just restating what the code is doing.
> 
Sure. I will remove it.

> > Source/WebCore/css/CSSParser.cpp:3394
> >      } else if (!parsedValue2 && propId == CSSPropertyWebkitBackgroundSize) {
> >          // For backwards compatibility we set the second value to the first if it is omitted.
> 
> It looks like the comment here is trying to handle this case.  Should we be hitting this code instead?
> 
There is another comment down which says that we just need to do it for -webkit-background-size and not for background-size. So I thought it's better to keep it as it is. Please let me know if I need to change this.

> > LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.html:-7
> > -<div id="test-pixels" style="background: red url(dummy://test.png) repeat top left / 100px 200px border-box padding-box"></div>
> 
> This test case appears to have changed to border-box border-box.  Was that intentional?
> 
> > LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.html:-8
> > -<div id="test-auto" style="background: red url(dummy://test.png) repeat 50% / auto auto border-box content-box"></div>
> 
> This too.  FWIW, it was hard to tell which test cases you added from the diff.  If you wanted to change the test, you could have done that in a separate refactor change before this bug fix.

Yes, above 2 changes were intentional. Currently the order of background-origin and background-clip is returned wrongly in StylePropertySet::getPropertyValue. Bug 86152 is created to handle this. This makes the newly introduced checkStyle() function for round-trip testing fails if the background-origin and background-clip values are different.

I will create another bug to refactor the layout testcase to include round-trip testing. I will keep the same values for background-origin and background-clip in that. And later I will change them to different values as part of 86152

[Tony Chang]

Comment on attachment 142539 [details]
ProposedPatch

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

>>> Source/WebCore/css/CSSParser.cpp:3394
>>>          // For backwards compatibility we set the second value to the first if it is omitted.
>> 
>> It looks like the comment here is trying to handle this case.  Should we be hitting this code instead?
> 
> There is another comment down which says that we just need to do it for -webkit-background-size and not for background-size. So I thought it's better to keep it as it is. Please let me know if I need to change this.

Ah, you're right, that's just for -webkit-background-size.  Your code looks correct.

[Joe Thomas]

Created attachment 142744 [details]
Patch-Updated

Patch updated after refactoring the layout test case in bug 86782

[WebKit Review Bot]

Comment on attachment 142744 [details]
Patch-Updated

Clearing flags on attachment: 142744

Committed r117622: <http://trac.webkit.org/changeset/117622>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.