1266914 - [GTK 3.20 Adwaita] Borders and separators too dark and thick

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[carlosjosepita]

Attachment: 2016-04-22-214307_1920x1080_scrot.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160412225428

Steps to reproduce:

Just run firefox (45 or 46b11). My gtk theme is adwaita. My os is arch.

 local/gtk2 2.24.30-1
    GObject-based multi-platform GUI toolkit (legacy)
local/gtk3 3.20.3-1
    GObject-based multi-platform GUI toolkit



Actual results:

Navigation bar, menu, find bar, etc. borders are too dark and thick. See the attached screenshot.

Comment 1

[carlosjosepita]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1211892 for a related issue.

Comment 2

[carlosjosepita]

One main offender seems to be ThreeDShadow. For example, setting:

#nav-bar {
    --urlbar-border-color: #a2a2a1 !important;
}

makes the navigation bar looks good again.

Comment 3

[carlosjosepita]

This started to happen after a recent update (gtk or firefox update, I can't tell for sure). I've no firefox addons nor themes installed. My gtk configuration is pretty standard (just out-of-the-box adwaita).

Comment 4

[Daniel Holbert [:dholbert]]

Thanks for the bug report! CC'ing the folks who've been working on GTK stuff recently.

In the meantime:

(In reply to carlosjosepita from comment #3)
> This started to happen after a recent update (gtk or firefox update, I can't
> tell for sure).

To figure out what needs fixing, it would be helpful if you could narrow down whether it was a Firefox update or a GTK update that introduced the issue.  e.g. could you download an older version of Firefox, like v43 and/or v44 from here:
  https://ftp.mozilla.org/pub/firefox/releases/43.0/
  https://ftp.mozilla.org/pub/firefox/releases/44.0/
...and report back about whether it's affected?

(Note that before starting one of these, you'll need to quit out of your current Firefox sesion -- otherwise, when you try to start the old version, it'll just open a new window in the already-running Firefox process.  (The "-no-remote" command-line flag is an alternative way around this, if you're familiar with that.))

(You may also want to create a fresh Firefox profile when testing older versions, in case something breaks from the newer-->older-->newer version changes -- though that's pretty unlikely.)

Comment 5

[carlosjosepita]

Ok, Daniel. Here is what I have:

1) Downgrading firefox to 44 has no effect regarding the issue.

2) Downgrading gtk3 from gtk3-3.18.9-1-x86_64.pkg.tar.xz to gtk3-3.18.8-1-x86_64.pkg.tar.xz fixes the problem (but I had to simultaneously downgrade firefox to 44 because of dependency problems. Nevertheless, both firefox 44 and 45 show thick borders against a newer gtk3).

So my conclusion is that the gtk3 upgrade was the culprit. I don't know what changed between 3.18.8 and 3.18.9 but it may give a hint to you.

Comment 6

[carlosjosepita]

I found out that the first arch package for ff 45 works against the older gtk and it doesn't show dark borders. So I can confirm that even some version of ff 45 is free of this problem, reinforcing the idea of a gtk3 upgrade being the cause.

Comment 7

[carlosjosepita]

Regarding the gtk3 packages above, sorry I made a mistake and I'm not able to edit the comment. The problematic jump is from:
  gtk3-3.18.9
to
  gtk3-3.19.12
at least with:
  firefox 45.0.1.

This make more sense because there were many adwaita changes:

https://git.gnome.org/browse/gtk+/log/?h=3.19.12&qt=grep&q=adwaita

Comment 9

[Karl Tomlinson (:karlt)]

Not sure that 1234158 is related, but it may turn out necessary.

Comment 10

[Andrew Comminos [:acomminos]]

Looks like GTK 3.20 removed the addition of GTK_STYLE_CLASS_FRAME in gtk_frame_init as part of the transition to CSS nodes. We could add it back, the rules are still there- what do you think, Karl? It might be a useful stopgap, considering fetching the border colour is technically deprecated anyway.

Comment 11

[Karl Tomlinson (:karlt)]

Interesting.
https://git.gnome.org/browse/gtk+/commit/?id=576028bdec90b27a42e8a65755dc0b2e235cdf5a
looks like it may have been trying to maintain support for foreign drawing.
I wonder whether that is intentional and widespread.

https://git.gnome.org/browse/gtk+/commit/?id=aa5dc38b0df3719a3407f8471c50b0355c113b5f
had already broken support for themes, settings, and Gecko.

(In reply to Andrew Comminos [:acomminos] from comment #10)
> It might be a useful stopgap,

I'm not so keen on adding a stop-gap, which will likely not be effective in other themes (if anyone would still dare to write one).

"GtkFrame has a main CSS node with name frame and a subnode with name border. The border node is used to render the visible border."

Seems we should get colors from the "border" subnode.

WidgetStyleCache.cpp has started providing that kind of thing.
I suspect it doesn't have support for detecting theme changes on 3.20 subnodes yet, but that's not a prerequisite for using it.

> considering fetching the border colour is technically deprecated anyway.

What exactly do you mean by "technically deprecated"?
Is there a replacement?

Comment 12

[Andrew Comminos [:acomminos]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #11)
> > considering fetching the border colour is technically deprecated anyway.
> 
> What exactly do you mean by "technically deprecated"?
> Is there a replacement?

The replacement for gtk_style_context_get_border_color (according to the GTK guys) would be use of the gtk_render APIs, which isn't possible in our case for nsLookAndFeel. It's in our best interest to stick with it for now, with our workaround for unico.

https://developer.gnome.org/gtk3/stable/GtkStyleContext.html#gtk-style-context-get-border-color

(In reply to Karl Tomlinson (ni?:karlt) from comment #11)
> Seems we should get colors from the "border" subnode.

https://blogs.gnome.org/mclasen/2015/11/20/a-gtk-update/ recommends construction a widget path for the subnode using gtk_widget_path_iter_set_object_name. That's probably the best solution, we just need to link against gtk_widget_path_iter_set_object_name at runtime for GTK 3.20 and then fetch the border colour from the border subnode.

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to Andrew Comminos [:acomminos] from comment #12)
> https://developer.gnome.org/gtk3/stable/GtkStyleContext.html#gtk-style-
> context-get-border-color

"gtk_style_context_get_border_color has been deprecated since version 3.16 and should not be used in newly-written code.

Use gtk_render_frame() instead."

Ah, thanks.  Perhaps some day, we may just need to render to a small image and read pixels, but I don't know how to deal with border radius.

> (In reply to Karl Tomlinson (ni?:karlt) from comment #11)
> > Seems we should get colors from the "border" subnode.
> 
> https://blogs.gnome.org/mclasen/2015/11/20/a-gtk-update/ recommends
> construction a widget path for the subnode using
> gtk_widget_path_iter_set_object_name. That's probably the best solution, we
> just need to link against gtk_widget_path_iter_set_object_name at runtime
> for GTK 3.20 and then fetch the border colour from the border subnode.

https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/widget/gtk/WidgetStyleCache.cpp#90

Comment 15

[Clément Lefèvre]

Ok, so as explained in bug 1269121, for the color part it could be reversed and white borders with dark themed Gnome. I'll keep an eye on the problem for dark themed Gnome and just add to take care to:
- the line between UI and webpage because it's "cutting" it and create a clear separation between UI zone and scrollbar zone.
- the border that appeared around the menu when clicking on the sandwich menu, being very strong (more than before)
- separation lines in menus (both contextual ones on right click and on menus from menubar) which are very strongs too.

Comment 16

[Eevee (Evelyn Woods) [:eevee]]

I just upgraded to Firefox 47 and started seeing this problem, with both gnome-breeze and breeze-gtk.  It also affects every single tab in Tree Style Tab's sidebar, making for an aesthetic catastrophe.

It didn't affect Firefox 46, even with the same version of GTK3.  Did something change in 47?

Nightly is the same, with the additional wrinkle that the scroll handle in scrollbars is completely invisible.

Comment 17

[Eevee (Evelyn Woods) [:eevee]]

Workaround was to edit my GTK theme (which for me is /usr/share/themes/Breeze/gtk-3.0/gtk.css) and add:

    frame { border-color: #c0c2c4; }

That color's already set on various children and pseudoelements of a frame (including a border element, as mentioned above), but not on the frame itself, which is where Firefox looks to get a value for ThreeDShadow.  I'm guessing this is why gtk_style_context_get_border_color is deprecated.  :)

Comment 18

[Bill Gianopoulos [:WG9s]]

(In reply to Eevee (Lexy Munroe) [:eevee] from comment #16)
> I just upgraded to Firefox 47 and started seeing this problem, with both
> gnome-breeze and breeze-gtk.  It also affects every single tab in Tree Style
> Tab's sidebar, making for an aesthetic catastrophe.
> 
> It didn't affect Firefox 46, even with the same version of GTK3.  Did
> something change in 47?
> 
> Nightly is the same, with the additional wrinkle that the scroll handle in
> scrollbars is completely invisible.

I just installed the breeze theme on my fedora24 system and cannot duplicate the scroll handle issue using a currently nightly build.

Comment 19

[Bill Gianopoulos [:WG9s]]

(In reply to Eevee (Lexy Munroe) [:eevee] from comment #17)
> Workaround was to edit my GTK theme (which for me is
> /usr/share/themes/Breeze/gtk-3.0/gtk.css) and add:
> 
>     frame { border-color: #c0c2c4; }
> 
This hack is trickier to do for Adwaita, because it is the default theme and he gtk.css file is ignored.  There is a css file that according to it's comments only contains key-bindings, but it works fine if you add this line there.  Under fedora24 the file to change is:

/usr/share/themes/Default/gtk-3.0/gtk-keys.css

Comment 20

[carlosjosepita]

> there.  Under fedora24 the file to change is:
> 
> /usr/share/themes/Default/gtk-3.0/gtk-keys.css

It seems to me that the correct place to put this workaround is ~/.config/gtk-3.0/gtk.css

That said, I would like to also fix the too dark menu separators. Do you know what the appropriate css selector is?

Comment 21

[Eevee (Evelyn Woods) [:eevee]]

Fixing the "frame" color also fixes Firefox's menu separators for me.  I don't have nearly-black lines anywhere now.

Comment 22

[carlosjosepita]

I found it: separator (https://developer.gnome.org/gtk3/stable/GtkSeparatorMenuItem.html)

This almost does the trick for me in Adwaita:

frame { border-color: #a2a2a1; }
separator { color: #a2a2a1; }

AFAICS there is one remaining detail to fix: the find bar (ctrl-f) upper border looks to thick. Any idea how to solve that?

Comment 23

[carlosjosepita]

I think the selector is something like findbar and the property maybe is clientTop (currently set to 2), but I'm not sure how to change it. Nevertheless, as I'm unable to go back to my previous installation after a system upgrade, and since I can't remember how the findbar top border looked like before that upgrade, I'm attaching a screenshot and asking you if yours looks the same as mine and if it's indeed intended to look like that.

https://postimg.org/image/6sw9wj4xd/

Comment 24

[carlosjosepita]

Attachment: findbar top border

Comment 25

[Andrew Comminos [:acomminos]]

Attachment: Bug 1266914 - Use the border subnode for GtkFrame to fetch border colours.

Review commit: https://reviewboard.mozilla.org/r/66836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66836/

Comment 26

[Karl Tomlinson (:karlt)]

Comment on attachment 8774356 [details]
Bug 1266914 - Use the border subnode for GtkFrame to fetch border colours.

https://reviewboard.mozilla.org/r/66836/#review64384

Comment 27

[Andrew Comminos [:acomminos]]

Comment on attachment 8774356 [details]
Bug 1266914 - Use the border subnode for GtkFrame to fetch border colours.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66836/diff/1-2/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7afc8055e8fb
Use the border subnode for GtkFrame to fetch border colours. r=karlt

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7afc8055e8fb

Comment 30

[Martin Stránský [:stransky] (ni? me)]

Attachment: cache style patch

Folks, there's a minor issue with this - the newly created style is not stored at widget cache as other css node based styles. It's no so big deal because it's used only once but better to fix that IMHO.

Comment 31

[Karl Tomlinson (:karlt)]

Comment on attachment 8775934 [details] [diff] [review]
cache style patch

Yes, that is what this function should do, thanks Martin.

I expect we might need MOZ_GTK_FRAME_BORDER for moz_gtk_frame_paint too, so keeping the style will be useful.

Comment 32

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8775934 [details] [diff] [review]
cache style patch

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69cff517689d

Comment 33

[Martin Stránský [:stransky] (ni? me)]

Attachment: style cache patch for check-in

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3803c63afd7e
cache newly created style, r=karlt

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3803c63afd7e