Notification bar info text is hard to read on Linux Mint

VERIFIED FIXED in Firefox 43

Status

()

Core
Widget: Gtk
VERIFIED FIXED
2 years ago
9 months ago

People

(Reporter: Christian Ascheberg, Assigned: acomminos)

Tracking

(Blocks: 1 bug)

Trunk
mozilla44
Unspecified
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed, firefox44 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8663424 [details]
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.
Blocks: 1193807
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;
> }
Assignee: nobody → andrew
Status: NEW → ASSIGNED
Created attachment 8665656 [details] [diff] [review]
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.
Attachment #8665656 - Flags: review?(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.
Attachment #8665656 - Flags: review?(karlt)
(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.
(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.
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.
Created attachment 8667671 [details] [diff] [review]
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!
Attachment #8665656 - Attachment is obsolete: true
Attachment #8667671 - Flags: review?(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?
Attachment #8667671 - Flags: review?(karlt) → review+
(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
Keywords: checkin-needed

Comment 10

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/60d1c59f129f
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/60d1c59f129f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Reporter)

Comment 12

2 years ago
Should the patch be uplifted to aurora/43?
Status: RESOLVED → VERIFIED
(Reporter)

Comment 13

2 years ago
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
Attachment #8667671 - Flags: approval-mozilla-aurora?
Yes, we should put this on 43, thanks.
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.
Attachment #8667671 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/21faa7b5534a
status-firefox43: affected → fixed
(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.

Updated

9 months ago
Blocks: 1320883
You need to log in before you can comment on or make changes to this bug.