|Summary:||REGRESSION(r244635): [GTK] Wrong background color used in non-dark mode|
|Product:||WebKit||Reporter:||Carlos Garcia Campos <cgarcia>|
|Severity:||Normal||CC:||bugs-noreply, ews-watchlist, mcatanzaro, timothy|
|Priority:||P2||Keywords:||Gtk, LayoutTestFailure, Regression|
|Version:||WebKit Nightly Build|
Description Carlos Garcia Campos 2019-04-25 02:49:53 PDT
Since r244635, we are now getting the frame view background color from the theme. That's correct for dark mode, but in non-dark mode we still want to use white backgrounds by default. This made a lot of tests to fail.
Comment 2 Michael Catanzaro 2019-04-25 06:15:29 PDT
Comment on attachment 368224 [details] Patch Makes sense.
Comment 3 Carlos Garcia Campos 2019-04-25 06:31:22 PDT
The patch is not enough for other test failures, I'll submit an updated one soon. We will need to rebaseline others because of the change in the disabled text for text fields.
Comment 5 Michael Catanzaro 2019-04-25 07:27:56 PDT
Comment on attachment 368233 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368233&action=review > Source/WebCore/css/CSSValueKeywords.in:272 > +-webkit-text-control-background Can we think of a better name? -webkit-system-control-background? -webkit-text-system-background?
Comment 6 Carlos Garcia Campos 2019-04-25 23:05:26 PDT
(In reply to Michael Catanzaro from comment #5) > Comment on attachment 368233 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=368233&action=review > > > Source/WebCore/css/CSSValueKeywords.in:272 > > +-webkit-text-control-background > > Can we think of a better name? > > -webkit-system-control-background? This would match apple, I added text because this is only used for text fields and text areas, not all form controls have the same background color. > -webkit-text-system-background? This doesn't make any sense to me. I'll use -webkit-system-text-control-background then.
Comment 8 EWS Watchlist 2019-04-26 03:00:49 PDT Comment hidden (obsolete)
Comment on attachment 368310 [details] Patch Attachment 368310 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12003555 New failing tests: fast/css/first-letter-anonymous-block-crash.html
Comment 9 EWS Watchlist 2019-04-26 03:00:51 PDT Comment hidden (obsolete)
Created attachment 368314 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
Comment 10 Carlos Garcia Campos 2019-04-26 06:01:37 PDT
(In reply to Build Bot from comment #8) > Comment on attachment 368310 [details] > Patch > > Attachment 368310 [details] did not pass ios-sim-ews (ios-simulator-wk2): > Output: https://webkit-queues.webkit.org/results/12003555 > > New failing tests: > fast/css/first-letter-anonymous-block-crash.html This was not caused by this patch.
Comment 11 Michael Catanzaro 2019-04-26 07:25:29 PDT
Maybe Timothy could propose a name; this becomes CSS API forever, so we want to make sure the name makes sense.
Comment 12 Timothy Hatcher 2019-04-26 10:33:53 PDT
(In reply to Michael Catanzaro from comment #11) > Maybe Timothy could propose a name; this becomes CSS API forever, so we want > to make sure the name makes sense. I would go with -webkit-text-background or -webkit-view-background. Since we have similar with -apple-system-text-background.
Comment 13 Michael Catanzaro 2019-04-26 11:14:19 PDT
This is equivalent to -apple-system-control-background, though.
Comment 14 Timothy Hatcher 2019-04-26 12:31:21 PDT
(In reply to Michael Catanzaro from comment #13) > This is equivalent to -apple-system-control-background, though. -apple-system is a prefix, separate from -apple and -webkit. So dropping "system" seems more fitting, and we don't have -webkit-system for any other keywords.
Comment 15 Michael Catanzaro 2019-04-26 15:20:15 PDT
I don't understand the difference between -apple-system and -apple, but OK. So would -webkit-control-background be appropriate, then?
Comment 16 Timothy Hatcher 2019-04-26 16:22:57 PDT
(In reply to Michael Catanzaro from comment #15) > I don't understand the difference between -apple-system and -apple, but OK. -apple is basically an alias for -webkit and -apple-system is separate and not mapped to -webkit. > So would -webkit-control-background be appropriate, then? That is fine.
Comment 17 Carlos Garcia Campos 2019-04-29 00:57:18 PDT
Created attachment 368445 [details] Patch for landing