Bug 75523 – Unify and modernize fast/css/{outline,background}-currentcolor.html

• Component: CSS
• Status: RESOLVED FIXED

[David Barr]

Unify and modernize fast/css/{outline,background}-currentcolor.html
These tests are nearly identical bar small style and javascript differences.
They also duplicate functionality of js-test-pre.js.

[David Barr]

Created attachment 135027 [details]
Patch

[Daniel Bates]

Comment on attachment 135027 [details]
Patch

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

> LayoutTests/ChangeLog:7
> +        Unify and modernize fast/css/{outline,background}-currentcolor.html
> +        https://bugs.webkit.org/show_bug.cgi?id=75523
> +
> +        Reviewed by NOBODY (OOPS!).
> +

This bug title isn't sufficiently descriptive of this change. Can you elaborate on how your proposed change unifies and modernizes these tests?

Maybe adding a description can help clarify the bug title and explain the motivation behind making these changes. You may want to say something like:

Use js-test-pre utility functions instead of hardcoded testing logic in fast/css/{outline, background}-currentcolor.html so as to simplify the test code and make the test more closely conform to the visual appearance of other PASS/FAIL tests.

> LayoutTests/fast/css/background-currentcolor.html:3
> +#test div { height: 5em; width: 10em; margin-bottom: 1em; }

This is OK as-as. That being said, we're underutilizing the class="test" in the markup (below). We can a class selector here instead of a descendent selector to match the same set of elements. If you choose to use a descendent selector then we should remove the 'class="test"' from the markup since it isn't being used.

> LayoutTests/fast/css/background-currentcolor.html:6
> +<div id="test">

Can we come up with a better name for this? Maybe, test-container?

> LayoutTests/fast/css/background-currentcolor.html:9
> +<div class="test" id="one" style="color:green; background: currentColor" ></div>
> +<div class="test" id="two" style="color:red; background: currentColor" ></div>
> +<div class="test" style="color:green">

We're underutilizing the class "test" here. Currently we match these elements using the descendent selector, #test div. We should either match these element using a class selector or remove the class attribute. See my remark on #test div (above) for more details.

> LayoutTests/fast/css/background-currentcolor.html:13
> +<p id="description">Test that background-color is non-inherit and currentColor is handled correctly. The test passes if 3 boxes with green backgrounds are displayed.</p>

As can be seen in the expected results, LayoutTests/fast/css/background-currentcolor-expected.txt, the text "TEST COMPLETE" is above the test description because there isn't an explicit <div id="console"></div> in the markup; => js-test-pre.js will insert such an element above <div id="test">. This makes both the test case and its expected results look weird and inconsistent with the output of other PASS/FAIL tests. I suggest that either we use description() (*) or explicitly declare <div id="console"></div> after <div id="test"> in the markup such that the text "TEST COMPLETE" is the last line of text in the expected results file.

(*) <http://trac.webkit.org/browser/trunk/LayoutTests/fast/js/resources/js-test-pre.js?rev=110650#L28>

> LayoutTests/fast/css/outline-currentcolor.html:13
> +<p id="description">Test that outline-color is non-inherit and currentColor is handled correctly. The test passes if 3 boxes with green outlines are displayed.</p>

As can be seen in the expected results, LayoutTests/fast/css/outline-currentcolor-expected.txt, the text "TEST COMPLETE" is above the test description. This looks weird. See my remark for the descriptive text in file background-currentcolor.html for more details.

> LayoutTests/fast/css/outline-currentcolor.html:21
> +var two = document.getElementById("two");
> +var three = document.getElementById("three");
> +two.style.color = "green";
> +shouldBeEqualToString('window.getComputedStyle(two)["' + property + '"]', 'rgb(0, 128, 0)');
> +shouldBeEqualToString('window.getComputedStyle(three).color', 'rgb(0, 128, 0)');

This code is identical to code in file LayoutTests/fast/css/background-currentcolor.html. Maybe we should consider extracting this code into a common

[Daniel Bates]

(In reply to comment #2)
> > LayoutTests/fast/css/outline-currentcolor.html:21
> > +var two = document.getElementById("two");
> > +var three = document.getElementById("three");
> > +two.style.color = "green";
> > +shouldBeEqualToString('window.getComputedStyle(two)["' + property + '"]', 'rgb(0, 128, 0)');
> > +shouldBeEqualToString('window.getComputedStyle(three).color', 'rgb(0, 128, 0)');
> 
> This code is identical to code in file LayoutTests/fast/css/background-currentcolor.html. Maybe we should consider extracting this code into a common

*into a common function. Although, this could be overkill given the length of this code.

[Daniel Bates]

(In reply to comment #2)
> > LayoutTests/fast/css/background-currentcolor.html:3
> > +#test div { height: 5em; width: 10em; margin-bottom: 1em; }
> 
> This is OK as-as. That being said, we're underutilizing the class="test" in the markup (below). We can a class selector here...

*use a

[David Barr]

Created attachment 135513 [details]
Patch

[David Barr]

(In reply to comment #3)
> This code is identical to code in file LayoutTests/fast/css/background-currentcolor.html.
> Maybe we should consider extracting this code into a common function.
> Although, this could be overkill given the length of this code.

After refining this part of the tests, the resulting code is short and structured
enough that extracting the common code does seem to be overkill.

(In reply to comment #4)
> > LayoutTests/fast/css/background-currentcolor.html:3
> > +#test div { height: 5em; width: 10em; margin-bottom: 1em; }
> 
> This is OK as-is. That being said, we're underutilizing the class="test"
> in the markup (below). We can use a class selector here...

This styling was for presentation not critical to the test.
After s/#test/&-container/, this is unnecessary as the
div is removed on completion.

[Daniel Bates]

Comment on attachment 135513 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

Nit: The Reviewed by line should be after the bug URL.

> LayoutTests/fast/css/background-currentcolor.html:9
> +  <div id="test1" style="color:green; background: currentColor"/>
> +  <div id="test2" style="color:red; background: currentColor"/>
>    <div style="color:green">
> -    <div id="three" style="color:currentColor; background: currentColor" ></div>
> +    <div id="test3" style="color:currentColor; background: currentColor"/>

This HTML5 markup is invalid as a <div> doesn't self-close. That is, you must explicitly specify a </div>.

> LayoutTests/fast/css/outline-currentcolor.html:9
> +  <div id="test1" id="one" style="color:green; outline: solid 1em currentColor"/>
> +  <div id="test2" id="two" style="color:red; outline: solid 1em currentColor"/>
>    <div style="color:green">
> -    <div id="three" style="color:currentColor; outline: solid 1em currentColor" ></div>
> +    <div id="test3" style="color:currentColor; outline: solid 1em currentColor"/>

Ditto.

[David Barr]

Created attachment 135523 [details]
Patch

[Daniel Bates]

Comment on attachment 135523 [details]
Patch

Thanks for updating the patch. Looks good to me.

[WebKit Review Bot]

Comment on attachment 135523 [details]
Patch

Clearing flags on attachment: 135523

Committed r113265: <http://trac.webkit.org/changeset/113265>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.