Bug 186244

Summary:	REGRESSION(r232338): [GTK] Broke a few layout tests
Product:	WebKit	Reporter:	Michael Catanzaro <mcatanzaro>
Component:	WebKitGTK	Assignee:	Nobody <webkit-unassigned>
Status:	RESOLVED FIXED
Severity:	Normal	CC:	bugs-noreply, cadubentzen, calvaris, mcatanzaro
Priority:	P2
Version:	WebKit Nightly Build
Hardware:	PC
OS:	Linux
See Also:	https://bugs.webkit.org/show_bug.cgi?id=126907

Description Michael Catanzaro 2018-06-03 11:34:04 PDT

r232338 broke a bunch of layout tests, which in retrospect was probably predictable. The vast majority of the tests just needed updated expectations to handle the new text colors, which have in many places changed from black to dark grays. I'm handling that now.

But a couple of the tests appear genuinely problematic. I'm going to mark these as expected fails:

fast/css/read-only-read-write-webkit-user-modify.html
fast/dom/HTMLInputElement/checked-pseudo-selector.html

Both tests appear to be showcasing the same problem. I would focus on fast/dom/HTMLInputElement/checked-pseudo-selector.html, since it seems to be much simpler. In this test, the text is supposed to be green, but instead it is black.

Normally, I would roll out (revert) the offending commit, but in this case, since it's only two tests and it fixed a pretty serious bug, I'm not going to do that, at least not until I've found more problematic tests.

--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/fast/css/read-only-read-write-webkit-user-modify-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/fast/css/read-only-read-write-webkit-user-modify-actual.txt
@@ -13,19 +13,19 @@
 PASS getComputedStyle(document.querySelectorAll("#test-block *")[3]).backgroundColor is "rgb(255, 0, 0)"
 PASS getComputedStyle(document.querySelectorAll("#test-block *")[4]).color is "rgb(0, 255, 0)"
 PASS getComputedStyle(document.querySelectorAll("#test-block *")[4]).backgroundColor is "rgb(255, 255, 255)"
-PASS getComputedStyle(document.querySelectorAll("#test-block *")[5]).color is "rgb(0, 255, 0)"
+FAIL getComputedStyle(document.querySelectorAll("#test-block *")[5]).color should be rgb(0, 255, 0). Was rgb(0, 0, 0).
 PASS getComputedStyle(document.querySelectorAll("#test-block *")[5]).backgroundColor is "rgb(255, 255, 255)"
-PASS getComputedStyle(document.querySelectorAll("#test-block *")[6]).color is "rgb(0, 0, 0)"
+FAIL getComputedStyle(document.querySelectorAll("#test-block *")[6]).color should be rgb(0, 0, 0). Was rgb(139, 142, 143).
 PASS getComputedStyle(document.querySelectorAll("#test-block *")[6]).backgroundColor is "rgb(255, 0, 0)"
 PASS getComputedStyle(document.querySelectorAll("#test-block *")[7]).color is "rgb(0, 0, 0)"
 PASS getComputedStyle(document.querySelectorAll("#test-block *")[7]).backgroundColor is "rgb(255, 0, 0)"
-PASS getComputedStyle(document.querySelectorAll("#test-block *")[8]).color is "rgb(0, 0, 0)"
+FAIL getComputedStyle(document.querySelectorAll("#test-block *")[8]).color should be rgb(0, 0, 0). Was rgb(139, 142, 143).
 PASS getComputedStyle(document.querySelectorAll("#test-block *")[8]).backgroundColor is "rgb(255, 0, 0)"
 PASS getComputedStyle(document.querySelectorAll("#test-block *")[9]).color is "rgb(0, 0, 0)"
 PASS getComputedStyle(document.querySelectorAll("#test-block *")[9]).backgroundColor is "rgb(255, 0, 0)"
 PASS getComputedStyle(document.querySelectorAll("#test-block *")[10]).color is "rgb(0, 255, 0)"
 PASS getComputedStyle(document.querySelectorAll("#test-block *")[10]).backgroundColor is "rgb(255, 255, 255)"
-PASS getComputedStyle(document.querySelectorAll("#test-block *")[11]).color is "rgb(0, 255, 0)"
+FAIL getComputedStyle(document.querySelectorAll("#test-block *")[11]).color should be rgb(0, 255, 0). Was rgb(0, 0, 0).
 PASS getComputedStyle(document.querySelectorAll("#test-block *")[11]).backgroundColor is "rgb(255, 255, 255)"
 PASS getComputedStyle(document.querySelectorAll("#test-block *")[12]).color is "rgb(0, 0, 0)"
 PASS getComputedStyle(document.querySelectorAll("#test-block *")[12]).backgroundColor is "rgb(255, 0, 0)"


--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/fast/dom/HTMLInputElement/checked-pseudo-selector-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/fast/dom/HTMLInputElement/checked-pseudo-selector-actual.txt
@@ -1,4 +1,4 @@
-PASS view.getComputedStyle(input1, '').getPropertyValue('color') is "rgb(0, 128, 0)"
+FAIL view.getComputedStyle(input1, '').getPropertyValue('color') should be rgb(0, 128, 0). Was rgb(0, 0, 0).
 PASS input1.checked is true
 PASS successfullyParsed is true

Comment 1 Michael Catanzaro 2018-06-03 15:10:09 PDT

I found enough additional broken tests that I'm no longer comfortable with keeping the change. E.g. fast/forms/button-generated-content.html, the button text should be blue but is now black after this change:

Before: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r232449%20(6864)/retries/fast/forms/button-generated-content-expected.png

After: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r232449%20(6864)/retries/fast/forms/button-generated-content-actual.png


Another example is fast/forms/input-disabled-color.html:

Before: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r232449%20(6864)/retries/fast/forms/input-disabled-color-expected.png

After: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r232449%20(6864)/retries/fast/forms/input-disabled-color-actual.png


I don't think there is any bug in the patch itself, just that it's exposing some existing bug with color computation. There ought to be logic somewhere to not use the theme color if the page has specified its own color.

So I'm going to do a rollout. Sorry, Carlos Eduardo. This is actually really common, when making changes that affect rendering or layout, it often takes a while to get everything completely right....

Comment 2 Carlos Bentzen 2018-06-03 15:14:41 PDT

Yeah it seems pretty reasonable to rollout after those failures you found.

Let's make bug 126907 depend on bug 186219?

This problem is not even with dark themes at all, it is about the logic for when to use the theme colors or not.

Comment 3 Michael Catanzaro 2018-06-03 15:15:26 PDT

Committed r232454: <https://trac.webkit.org/changeset/232454>

Comment 4 Michael Catanzaro 2018-06-03 15:17:38 PDT

(In reply to Carlos Eduardo Ramalho from comment #2)
> Yeah it seems pretty reasonable to rollout after those failures you found.
> 
> Let's make bug 126907 depend on bug 186219?
> 
> This problem is not even with dark themes at all, it is about the logic for
> when to use the theme colors or not.

Your change in bug #165072 also broke at least one test, but I forget which, and it was only one affected test that I found so far, whereas I found many affected by this one. So I'll leave that change be for now.

Comment 5 Michael Catanzaro 2018-06-03 15:19:24 PDT

(In reply to Michael Catanzaro from comment #4)
> Your change in bug #165072 also broke at least one test, but I forget which,
> and it was only one affected test that I found so far, whereas I found many
> affected by this one. So I'll leave that change be for now.

Eh, just kidding, it depends on the patch I just rolled out and the build is failing now... whoops.

Comment 6 Michael Catanzaro 2018-06-03 15:24:24 PDT

Committed r232455: <https://trac.webkit.org/changeset/232455>