Bug 104014 – [CSS3 Backgrounds and Borders] Allow the CSS3 background position offset for background shorthand.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexis Menard (darktears)]

[CSS3 Backgrounds and Borders] Allow the CSS3 background position offset for background shorthand.

[Alexis Menard (darktears)]

Created attachment 177510 [details]
Patch

[Dirk Schulze]

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?

[Dirk Schulze]

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.

[Alexis Menard (darktears)]

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.

[Alexis Menard (darktears)]

Created attachment 177718 [details]
Patch

[Dirk Schulze]

Comment on attachment 177718 [details]
Patch

LGTM. r=me.

[WebKit Review Bot]

Comment on attachment 177718 [details]
Patch

Clearing flags on attachment: 177718

Committed r136683: <http://trac.webkit.org/changeset/136683>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.