[webkit-reviews] review granted: [Bug 211672] CSS Variables: Color on specific `border` properties does not work. : [Attachment 401101] Patch

Thu Jun 4 19:59:27 PDT 2020

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Tyler Wilcock
<twilco.o at protonmail.com>'s request for review:
Bug 211672: CSS Variables: Color on specific `border` properties does not work.
https://bugs.webkit.org/show_bug.cgi?id=211672

Attachment 401101: Patch

https://bugs.webkit.org/attachment.cgi?id=401101&action=review

--- Comment #10 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 401101
  --> https://bugs.webkit.org/attachment.cgi?id=401101
Patch

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

> LayoutTests/fast/borders/logical-border-props-with-variables-expected.html:2
> +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
> +<html xmlns="http://www.w3.org/1999/xhtml">

Don't need XHTML here. This can just be <!DOCTYPE html><html>

> LayoutTests/fast/borders/logical-border-props-with-variables-expected.html:8
> +    <title>
> +	   Test for https://bugs.webkit.org/show_bug.cgi?id=211672.  The
intention of this test is to ensure variables are
> +	   usable as values in the `border-block-start`, `border-block-end`,
`border-inline-start`, and `border-inline-end`
> +	   properties.
> +    </title>

I would not bother with this in the expected result.

> LayoutTests/fast/borders/logical-border-props-with-variables-expected.html:11
> +    <style type="text/css">

Just <style>

> LayoutTests/fast/borders/logical-border-props-with-variables-expected.html:22
> +    Test for https://bugs.webkit.org/show_bug.cgi?id=211672.  Passes if
there is a 3px solid green border around all four sides of this text.

I would remove this text. Having visible text in a test makes it more likely to
fail in future because of small antialiasing differences.

> LayoutTests/fast/borders/logical-border-props-with-variables.html:25
> +    Test for https://bugs.webkit.org/show_bug.cgi?id=211672.  Passes if
there is a 3px solid green border around all four sides of this text.

Same comments apply to this file.