1206516 - Notification bar info text is hard to read on Linux Mint

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Christian Ascheberg]

Attachment: screenshot

As can be seen in the attached screenshot, the Firefox notification bar info text is hard to read with current Nightly on Linux Mint 17.2.
The screenshot shows the "testcase 1" and the compiled test program code that are both attached to bug 1198063. The difference to that bug is that the text color of the gtk3-info-bar-test program does not match the color of the text shown in Firefox, so that could be a Firefox bug.

Comment 1

[Andrew Comminos [:acomminos]]

Looks like we could probably just add GtkLabel to the widget style path when computing the text color to fix this according to the styling in the Mint-X GTK theme;

> .info GtkLabel {
>     color: @theme_text_color;
> }

Comment 2

[Andrew Comminos [:acomminos]]

Attachment: Properly build widget hierarchy for -moz-gtk-info-bar-text, fixing hard to read text on some GTK themes.

This patch correctly builds the widget hierarchy for a GtkLabel in a GtkInfoBar exactly like GTK does it (verified with the inspector), fixing the issue.

It appears that built widget paths do not inherit parents' style classes, which is why we have to add GTK_STYLE_CLASS_INFO twice.

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8665656 [details] [diff] [review]
Properly build widget hierarchy for -moz-gtk-info-bar-text, fixing hard to read text on some GTK themes.

(In reply to Andrew Comminos [:acomminos] from comment #1)
> > .info GtkLabel {
> >     color: @theme_text_color;
> > }

(In reply to Andrew Comminos [:acomminos] from comment #2)
> This patch correctly builds the widget hierarchy for a GtkLabel in a
> GtkInfoBar exactly like GTK does it (verified with the inspector), fixing
> the issue.

I'm happy the widget path looks correct, thanks.

> It appears that built widget paths do not inherit parents' style classes,
> which is why we have to add GTK_STYLE_CLASS_INFO twice.

I don't follow this.  GtkInfoBar doesn't add "info" to the label, and Mint-X
is expecting "info" on an ancestor of a label, not the label itself.

If widget paths are not matching the correct selectors (I don't know why that
would be), then I think we need to stop using widget paths and instead create
temporary widgets.

Comment 4

[Andrew Comminos [:acomminos]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #3)
> I don't follow this.  GtkInfoBar doesn't add "info" to the label, and Mint-X
> is expecting "info" on an ancestor of a label, not the label itself.

Sorry, I should've mentioned that the "info" styling for the label is to ensure we don't regress with theming on Ambiance (which sets the 'color' property in the .info selector). Setting the class on the GtkInfoBar in the widget path doesn't appear to propagate it down to the label otherwise.

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to Andrew Comminos [:acomminos] from comment #4)
> Sorry, I should've mentioned that the "info" styling for the label is to
> ensure we don't regress with theming on Ambiance (which sets the 'color'
> property in the .info selector). Setting the class on the GtkInfoBar in the
> widget path doesn't appear to propagate it down to the label otherwise.

OK, thanks, but the same applies there too.  If widget paths are not behaving the same as GTK widgets (perhaps because properties are not inherited when set on ancestors?), then we should not be using widget paths.

Comment 6

[Martin Giger [:freaktechnik]]

This also happens for me on elementary OS with the light theme variant. Note that it also affects basic UI components like the URL bar or the search bar input.

Comment 7

[Andrew Comminos [:acomminos]]

Attachment: Use temporary widget to construct style context for GtkInfoBar.

I agree that using temporary widgets will likely be more reliable; attached is a patch that yanks the style context from a GtkLabel in a GtkInfoBar. Thanks!

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8667671 [details] [diff] [review]
Use temporary widget to construct style context for GtkInfoBar.

Thanks!

>-    gtk_widget_path_append_type(path, GTK_TYPE_INFO_BAR);
>-    style = create_context(path);

>+    gtk_container_add(GTK_CONTAINER(infoBarContent), infoBarLabel);
>+    style = gtk_widget_get_style_context(infoBarLabel);
>     gtk_style_context_add_class(style, GTK_STYLE_CLASS_INFO);
>     gtk_style_context_get_color(style, GTK_STATE_FLAG_NORMAL, &color);

There's no need to add GTK_STYLE_CLASS_INFO to the label, right?

Comment 9

[Andrew Comminos [:acomminos]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #8)
> There's no need to add GTK_STYLE_CLASS_INFO to the label, right?

Right, inheritance works as expected when we let GTK manage the widget hierarchy. GTK_STYLE_CLASS_INFO gets set on the info bar when the message type of the info bar is set:

https://git.gnome.org/browse/gtk+/tree/gtk/gtkinfobar.c?id=3.18.0#n1270

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60d1c59f129f

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60d1c59f129f

Comment 12

[Christian Ascheberg]

Should the patch be uplifted to aurora/43?

Comment 13

[Christian Ascheberg]

Comment on attachment 8667671 [details] [diff] [review]
Use temporary widget to construct style context for GtkInfoBar.

Approval Request Comment
[Feature/regressing bug #]: GTK3 / bug 1187203
[User impact if declined]: notification bar is unreadable on Linux Mint
[Describe test coverage new/current, TreeHerder]: has been on m-c for a while
[Risks and why]: I guess low risk
[String/UUID change made/needed]: none

Comment 14

[Karl Tomlinson (:karlt)]

Yes, we should put this on 43, thanks.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8667671 [details] [diff] [review]
Use temporary widget to construct style context for GtkInfoBar.

Minor tweak for UI readability. OK to uplift to aurora.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/21faa7b5534a

Comment 17

[Karl Tomlinson (:karlt)]

(In reply to Andrew Comminos [:acomminos] from comment #2)
> It appears that built widget paths do not inherit parents' style classes,

I suspect gtk_style_context_set_parent() would be required with a chain of style contexts.

That would probably make it more complicated than creating the widget, and creating the widget has the possible advantage of automatically collecting some of the right style classes etc added to the context by the widget.