Bug 211672

Summary: CSS Variables: Color on specific `border` properties does not work.
Product: WebKit Reporter: ed7-aspire4925
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, koivisto, macpherson, menard, simon.fraser, twilco.o, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Other   
OS: Other   
Attachments:
Description Flags
sample
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description ed7-aspire4925 2020-05-09 13:27:27 PDT 
Created attachment 398937 [details]
sample

Color CSS variables set in the following `border` properties do not work:
- border-block-start
- border-block-end
- border-inline-start
- border-inline-end

See attachment or [https://jsfiddle.net/f2u4w9g6/1]

WebKitGTK 2.28.2-2~deb10u1

Tested on Midori and GNOME Web, both use the same engine.
Comment 1 Radar WebKit Bug Importer 2020-05-12 14:07:53 PDT 
<rdar://problem/63153128>
Comment 2 Tyler Wilcock 2020-06-03 22:15:01 PDT 
Created attachment 400997 [details]
Patch
Comment 3 Simon Fraser (smfr) 2020-06-04 11:02:29 PDT 
Comment on attachment 400997 [details]
Patch

Patch looks OK,but can you make a reference test (foo.html, foo-expected.html that render the same way). We don't add new -expected.png files any more.
Comment 4 Tyler Wilcock 2020-06-04 16:21:23 PDT 
Created attachment 401089 [details]
Patch
Comment 5 Darin Adler 2020-06-04 16:23:33 PDT 
Comment on attachment 401089 [details]
Patch

Need to create a logical-border-props-with-variables-expected.html file that generates the same appearance, but without using variables. That will make the test a "reftest". Should not have an expected.txt or an expected.png.
Comment 6 Tyler Wilcock 2020-06-04 16:27:08 PDT 
Created attachment 401091 [details]
Patch
Comment 7 Tyler Wilcock 2020-06-04 16:29:13 PDT 
Oops, thanks Darin, misread Simon's comment.  Will do that now.  I removed the review request on this latest patch.
Comment 8 Tyler Wilcock 2020-06-04 17:04:50 PDT 
Created attachment 401096 [details]
Patch
Comment 9 Tyler Wilcock 2020-06-04 17:22:23 PDT 
Created attachment 401101 [details]
Patch
Comment 10 Simon Fraser (smfr) 2020-06-04 19:59:27 PDT 
Comment on attachment 401101 [details]
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.
Comment 11 Tyler Wilcock 2020-06-04 20:26:51 PDT 
Created attachment 401114 [details]
Patch
Comment 12 Simon Fraser (smfr) 2020-06-04 20:59:29 PDT 
Comment on attachment 401114 [details]
Patch

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

> LayoutTests/fast/borders/logical-border-props-with-variables-expected.html:2
> +<html xmlns="http://www.w3.org/1999/xhtml">

No xmlns.

> LayoutTests/fast/borders/logical-border-props-with-variables.html:25
> +    <div>
> +    </div>

I guess this now ends up as a 6x6px div. I think it would be better to give it a size (like 100x100px) and make the border super obvious (50px width). Don't be shy making things big!
Comment 13 Tyler Wilcock 2020-06-04 21:13:31 PDT 
Created attachment 401116 [details]
Patch
Comment 14 Simon Fraser (smfr) 2020-06-04 21:26:06 PDT 
Comment on attachment 401116 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        Add test verifying CSS variables work as values in the `border-block-start`, `border-block-end`, `border-inline-start`, and `border-inline-end` properties.
> +
> +        Reviewed by NOBODY (OOPS!).

The Reviewed by should go above, but that's fine.
Comment 15 EWS 2020-06-04 21:26:32 PDT 
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
Comment 16 Tyler Wilcock 2020-06-04 21:32:18 PDT 
Created attachment 401118 [details]
Patch
Comment 17 Tyler Wilcock 2020-06-04 21:34:12 PDT 
I added a new patch with the "Reviewed by" in the correct spot.  Should this line be removed, or will EWS accept it now that it's in the right place?
Comment 18 EWS 2020-06-05 10:22:11 PDT 
Committed r262627: <https://trac.webkit.org/changeset/262627>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401118 [details].
Comment 19 Tyler Wilcock 2020-06-05 10:33:49 PDT 
Thanks for sticking with me on this one -- learned a lot!