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
Created attachment 142539 [details] ProposedPatch
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.
(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
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.
Created attachment 142744 [details] Patch-Updated Patch updated after refactoring the layout test case in bug 86782
Comment on attachment 142744 [details] Patch-Updated Clearing flags on attachment: 142744 Committed r117622: <http://trac.webkit.org/changeset/117622>
All reviewed patches have been landed. Closing bug.