[CSS3 Backgrounds and Borders] Allow the CSS3 background position offset for background shorthand.
Created attachment 177510 [details] Patch
Comment on attachment 177510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177510&action=review > Source/WebCore/css/CSSParser.cpp:3859 > +bool CSSParser::isPositionValue(CSSParserValue* value) can you make this an inline function? Or just put this in the header? > Source/WebCore/css/CSSParser.cpp:4139 > + // parseFillBackgroundPosition advances the m_valueList pointer Make it a sentence please. > LayoutTests/fast/backgrounds/background-position-parsing-2.html:454 > +style.background = "center center center / 10em gray round fixed border-box"; > +shouldBe("style.background", "'left 30% top 10% round fixed border-box border-box gray'"); > +shouldBe("computedStyle.background", "'rgb(128, 128, 128) none round fixed left 30% top 10% / auto border-box border-box'"); > + Do we have negative tests that are supposed to fail? If not, can you add them please?
Comment on attachment 177510 [details] Patch Looking at it again, it looks like they are missing, since no test needs to be adapted. Please add negative tests as well, where the position is partly between background-position and -size, where you have 1, 2, 3, 4 or fife values. I just can see tests with 4 values. That is not enough. I assume we have tests for 1 and 2 values, but 3 and 5 are missing.
Comment on attachment 177510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177510&action=review >> LayoutTests/fast/backgrounds/background-position-parsing-2.html:454 >> + > > Do we have negative tests that are supposed to fail? If not, can you add them please? These on top were. Maybe it was not clear but after debug("background-position inside the background shorthand, some invalid cases, no changes expected"); in fact the style didn't change because the background values were incorrect. I made it more clear in the next version.
Created attachment 177718 [details] Patch
Comment on attachment 177718 [details] Patch LGTM. r=me.
Comment on attachment 177718 [details] Patch Clearing flags on attachment: 177718 Committed r136683: <http://trac.webkit.org/changeset/136683>
All reviewed patches have been landed. Closing bug.