Bug 101245 – Add an extra test for background-position parsing.

• Component: CSS
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

Add an extra test for background-position parsing.

[Alexis Menard (darktears)]

Created attachment 172382 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 172382 [details]
Patch

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

> LayoutTests/fast/backgrounds/background-position-parsing-2.html:4
> +<meta charset="utf-8">

Do we normally specify this?

> LayoutTests/fast/backgrounds/background-position-parsing-2.html:10
> +<script>
> +
> +description("Test to make sure background-position is correctly parsed.")

why newline here before description.

is parsed correctly.

> LayoutTests/fast/backgrounds/background-position-parsing-2.html:46
> +// One value.
> +style.backgroundPosition = "70%";
> +// Second value is assuming to be 'center'
> +shouldBe("style.backgroundPosition", "'70% 50%'");
> +shouldBe("computedStyle.backgroundPosition", "'70% 50%'");
> +style.backgroundPosition = "84px";
> +shouldBe("style.backgroundPosition", "'84px 50%'");
> +shouldBe("computedStyle.backgroundPosition", "'84px 50%'");
> +style.backgroundPosition = "left";
> +shouldBe("style.backgroundPosition", "'0% 50%'");
> +shouldBe("computedStyle.backgroundPosition", "'0% 50%'");
> +style.backgroundPosition = "right";
> +shouldBe("style.backgroundPosition", "'100% 50%'");
> +shouldBe("computedStyle.backgroundPosition", "'100% 50%'");
> +style.backgroundPosition = "bottom";
> +shouldBe("style.backgroundPosition", "'50% 100%'");
> +shouldBe("computedStyle.backgroundPosition", "'50% 100%'");
> +style.backgroundPosition = "top";
> +shouldBe("style.backgroundPosition", "'50% 0%'");
> +shouldBe("computedStyle.backgroundPosition", "'50% 0%'");
> +style.backgroundPosition = "center";
> +shouldBe("style.backgroundPosition", "'50% 50%'");
> +shouldBe("computedStyle.backgroundPosition", "'50% 50%'");

Adding a bit of newlines between tests might make this more readable

> LayoutTests/fast/backgrounds/background-position-parsing-2.html:172
> +document.body.removeChild(testContainer);
> +
> +</script>

I would remove that newline

[Eric Seidel (no email)]

Comment on attachment 172382 [details]
Patch

LGTM.

[Alexis Menard (darktears)]

Created attachment 172541 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 172541 [details]
Patch for landing

Clearing flags on attachment: 172541

Committed r133591: <http://trac.webkit.org/changeset/133591>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.