REGRESSION(r244635): [GTK] Wrong background color used in non-dark mode

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 368224 [details]
Patch

[Michael Catanzaro]

Comment on attachment 368224 [details]
Patch

Makes sense.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 368233 [details]
Patch

[Michael Catanzaro]

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?

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 368310 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Carlos Garcia Campos]

(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.

[Michael Catanzaro]

Maybe Timothy could propose a name; this becomes CSS API forever, so we want to make sure the name makes sense.

[Timothy Hatcher]

(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.

[Michael Catanzaro]

This is equivalent to -apple-system-control-background, though.

[Timothy Hatcher]

(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.

[Michael Catanzaro]

I don't understand the difference between -apple-system and -apple, but OK.

So would -webkit-control-background be appropriate, then?

[Timothy Hatcher]

(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.

[Carlos Garcia Campos]

Created attachment 368445 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r244731: <https://trac.webkit.org/changeset/244731>