Bug 151533 – [GTK] RenderThemeGtk::platformActiveSelectionBackgroundColor, et. al. should not clobber state of cached GtkStyleContexts

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

platformActiveSelectionBackgroundColor(), platformInactiveSelectionBackgroundColor(), etc. are const functions intended only to return a color used for painting, but they also change the state of the cached GtkStyleContexts we use for GTK_TYPE_ENTRY and GTK_TYPE_TREE_VIEW. This could cause theme colors returned by those GtkStyleContexts to change unexpectedly.

At first I thought this was a regression I introduced in r192723, but it was actually introduced about a year ago.

[Michael Catanzaro]

(In reply to comment #0)
> This could cause theme colors returned by those GtkStyleContexts to change unexpectedly.

This didn't cause any regression only because every place using these style contexts explicitly sets the state of the style contexts before use. In fact, the GtkTreeView style context is not used anywhere else, and the GtkEntry style context is only used in paintTextField, which does set the state before use (and then reverts it using save/restore), so this cannot have broken anything in practice. But it's a landmine waiting for the next programmer to trip it.

[Michael Catanzaro]

Created attachment 266029 [details]
Patch

[Michael Catanzaro]

Created attachment 266031 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 266031 [details]
Patch

Clearing flags on attachment: 266031

Committed r192727: <http://trac.webkit.org/changeset/192727>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

Reverted r192727 for reason:

It made the selections transparent again and broke /webkit2/WebKitWebView/snapshot

Committed r192731: <http://trac.webkit.org/changeset/192731>

[Michael Catanzaro]

(In reply to comment #1)
> (In reply to comment #0)
> > This could cause theme colors returned by those GtkStyleContexts to change unexpectedly.
> 
> This didn't cause any regression only because every place using these style
> contexts explicitly sets the state of the style contexts before use. In
> fact, the GtkTreeView style context is not used anywhere else, and the
> GtkEntry style context is only used in paintTextField, which does set the
> state before use (and then reverts it using save/restore), so this cannot
> have broken anything in practice.

I missed that this function is sometimes called with the GtkButton style context, so I see that this patch could affect the results of RenderThemeGtk::paintButton and RenderThemeGtk::paintMenuList (presumably for the better). But I frankly have no clue how it broke selections. I need to investigate closer.

[Michael Catanzaro]

(In reply to comment #7)
> I missed that this function is sometimes called with the GtkButton style
> context, so I see that this patch could affect the results of
> RenderThemeGtk::paintButton

This might be causing GNOME #758968

[Michael Catanzaro]

Created attachment 267022 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 267022 [details]
Patch

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

Ok, let's try it out.

> Source/WebCore/rendering/RenderThemeGtk.cpp:398
> -    GtkStyleContext* context = 0;
> +    GRefPtr<GtkStyleContext> context = nullptr;

GRefPtr is already nullptr initialized.

[Michael Catanzaro]

Created attachment 267078 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 267078 [details]
Patch

Clearing flags on attachment: 267078

Committed r193896: <http://trac.webkit.org/changeset/193896>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.