1315668 - menu highlight background color is missing in themes that animate menu item highlights (e.g. BlueMenta)

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P1)

Description

[vummiess]

Attachment: Firefox 50 (menu background highlight color missing (causes hovered text to be unreadable))

starting in Firefox 50, the menu highlight background color is missing in some themes (e.g. BlueMenta).

Comment 1

[vummiess]

Attachment: correct menu highlight background from gtk3-widget-factory

Comment 2

[Karl Tomlinson (:karlt)]

I see with even with Menta from mate-themes-3.18.3.

Comment 3

[Karl Tomlinson (:karlt)]

The first regression I see is between 2016-05-10/043082cb7bd8490c60815f67fbd1f33323ad7663 and 2016-05-11/674a552743785c28c75866969aad513bd8eaf6ae
suggesting changes for bug 1248974 as the trigger.

Between 2016-05-30-03/cad514ad49c199e823a92e8c8d27e16c22c3cac7 and 2016-05-30-07/3435dd7ad71fe9003bdeee18fd38d815e033beef
the problem started to remedy itself on the second menuitem paint.
Bug 1274745 changes were in that interval though relationship is somewhat obscure.

Before 2016-08-01, the problem started to recur after each hover over and inactive menuitem, still remedying on the second subsequent active menuitem paint.

Between 
2016-09-19-03/f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc and 2016-09-19-06/fd0564234eca242b7fb753a110312679020f8059
the problem stopped correcting, suggesting changes for bug 1301194 as that trigger.

Comment 4

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

(In reply to Karl Tomlinson (:karlt) from comment #3)
> the problem stopped correcting, suggesting changes for bug 1301194 as that
> trigger.

Yes that's really the point. From my testing (gtk 3.22.2) for the correct rendering:

- direction has to be set explicitly, by gtk_style_context_set_direction()
- STATE_FLAG_DIR_LTR can't be set by gtk_style_context_set_state()

If any of those two does not comply the rendering is broken.

Comment 5

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

Seems to be a bug in animated styles in Gtk. Gtk stores (internally) wrong style state which is also handled to us. When we set the prelight state Gtk does not invalide the style. A simple workaround is to call something like:

gtk_style_context_set_state(style, static_cast<GtkStateFlags>(0));
gtk_style_context_set_state(style, static_cast<GtkStateFlags>(newState));

but the optimization where we check the style state (oldState != newState) does not work because returned oldState is invalid.

Comment 6

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

Filled as https://bugzilla.gnome.org/show_bug.cgi?id=774431

Comment 7

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

Karl, see the comment from Benjamin at https://bugzilla.gnome.org/show_bug.cgi?id=774431#c2. I guess we should create the style from path or duplicate the widgets we get style from for different states (normal/prelight, etc.) IMHO the first option seems to be easier for us.

Comment 8

[Karl Tomlinson (:karlt)]

Yes, I agree styles from path is the best solution here.
I have some patches for that.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: bug 1315668 remove use of gtk_container_get_border_width from menuitems

The GtkContainer border-width property defaults to zero.  It is not influenced
by theme CSS.  While theme engines can theoretically modify default values for
properties of any class, I don't think that is something that needs to be
supported.

Removing this code is necessary to switch from caching GtkWidgets to caching
GtkStyleContexts only.

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

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: bug 1315668 use style context instead of widget for menuitem dimensions

This is necessary to switch from caching GtkWidgets to caching
GtkStyleContexts only.

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

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: bug 1315668 CreateStyleForWidget: store classes on context instead of path

CreateStyleForWidget() then provides the same behavior with
g_style_context_save() as contexts from widget root style nodes.

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

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: bug 1315668 construct menuitem style contexts from paths

instead of using the context belonging to a widget.
Only the style context is cached, instead of the whole widget.

Using the style context from a widget meant that rendering displayed the
initial appearance of animations after state changes, but there was no
invalidation to trigger the final rendering in the animations.

Style contexts constructed from paths do not incorporate animations.
(See gtk_css_path_node_update_style() in GTK.)  Therefore they provide the
appropriate rendering for Gecko's model, which is not expecting animations.

There is no mechanism available to display animations when using style
contexts constructed from paths, but the GtkWidget animation design is also
not suitable for rendering potentially multiple elements each in a different
state of their animation.

This contexts-from-paths approach can be extended also to other widget types,
but this is a smaller change intended for uplift to other branches to address
a regression in menuitem rendering.

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

Comment 13

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

Comment on attachment 8810705 [details]
bug 1315668 remove use of gtk_container_get_border_width from menuitems

https://reviewboard.mozilla.org/r/92986/#review93036

Sounds reasonable.

Comment 14

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

Comment on attachment 8810706 [details]
bug 1315668 use style context instead of widget for menuitem dimensions

https://reviewboard.mozilla.org/r/92988/#review93044

Looks okay.

Comment 15

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

Comment on attachment 8810708 [details]
bug 1315668 construct menuitem style contexts from paths

https://reviewboard.mozilla.org/r/92992/#review93094

Fine here.

Comment 16

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

Comment on attachment 8810707 [details]
bug 1315668 CreateStyleForWidget: store classes on context instead of path

https://reviewboard.mozilla.org/r/92990/#review93100

I've a limited knowledge of the Gtk+ internals but I've got a confirmation from Benjamin Otte that this should work here so virtually carry r+ from him.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0ca38de294a
remove use of gtk_container_get_border_width from menuitems r=stransky+263117
https://hg.mozilla.org/integration/autoland/rev/749bc3f069a8
use style context instead of widget for menuitem dimensions r=stransky+263117
https://hg.mozilla.org/integration/autoland/rev/1693f10ca33e
CreateStyleForWidget: store classes on context instead of path r=stransky+263117
https://hg.mozilla.org/integration/autoland/rev/d09e8a3b7875
construct menuitem style contexts from paths r=stransky+263117

Comment 18

[Karl Tomlinson (:karlt)]

moz_gtk_images_in_menus() will need to use a style context if applying these patches to a version which still has this function, removed in version 52 for bug 1302157.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0ca38de294a
https://hg.mozilla.org/mozilla-central/rev/749bc3f069a8
https://hg.mozilla.org/mozilla-central/rev/1693f10ca33e
https://hg.mozilla.org/mozilla-central/rev/d09e8a3b7875

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Karl, should we uplift this fix to Aurora52/Beta51?

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I am assuming this affects only one or two themes and is therefore not a commonly hit scenario. Given the size of code change (4 patches!) I'd rather this ride the 51+ train. Wontfix for 50. Please let me know if you have concerns.

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 8810705 [details]
bug 1315668 remove use of gtk_container_get_border_width from menuitems

Please consider this a request for all 4 patches on this bug.

Yes, this is too risky for 50, and I don't know of any distributions having default themes affected by this.

I think we should uplift to Aurora 52.

Approval Request Comment
[Feature/regressing bug #]:
This was first observed after changes from bug 1248974.
That caused Firefox 49 to not show the correct background on the first paint or two.
Firefox 50 continues to show the incorrect background.
[User impact if declined]:
Funny looking menubar and context menus.  The highlighted item will likely be hard to read.
This happens only with some themes. 
[Describe test coverage new/current, TreeHerder]:
Tested manually with problem theme.
[Risks and why]: 
Although I'm reasonably confident that what is changed here should work in general, there are many factors involved and it is hard to consider them all.  I've been surprised before.
[String/UUID change made/needed]:
None.

We could uplift for Beta 51 also, but I would need to extend the patch.
We have time to fix any problems that show up, but if it is only a few rarely used themes, then it is not worth it.  At this stage, I won't request Beta approval.

Others please speak up and explain the situation if this affects widely used themes.  We can then reconsider.

Comment 23

[Ryan]

(In reply to Karl Tomlinson (:karlt) from comment #22)
> Comment on attachment 8810705 [details]
> bug 1315668 remove use of gtk_container_get_border_width from menuitems
> 
> Please consider this a request for all 4 patches on this bug.
> 
> Yes, this is too risky for 50, and I don't know of any distributions having
> default themes affected by this.
> 
> I think we should uplift to Aurora 52.
> 
> Approval Request Comment
> [Feature/regressing bug #]:
> This was first observed after changes from bug 1248974.
> That caused Firefox 49 to not show the correct background on the first paint
> or two.
> Firefox 50 continues to show the incorrect background.
> [User impact if declined]:
> Funny looking menubar and context menus.  The highlighted item will likely
> be hard to read.
> This happens only with some themes. 
> [Describe test coverage new/current, TreeHerder]:
> Tested manually with problem theme.
> [Risks and why]: 
> Although I'm reasonably confident that what is changed here should work in
> general, there are many factors involved and it is hard to consider them
> all.  I've been surprised before.
> [String/UUID change made/needed]:
> None.
> 
> We could uplift for Beta 51 also, but I would need to extend the patch.
> We have time to fix any problems that show up, but if it is only a few
> rarely used themes, then it is not worth it.  At this stage, I won't request
> Beta approval.
> 
> Others please speak up and explain the situation if this affects widely used
> themes.  We can then reconsider.

Any chance for this to make it into the Beta build then? Between this bug and the black borders in input boxes on GTK 3.20+ I am stuck using 49 for the time being.

Comment 24

[Karl Tomlinson (:karlt)]

(In reply to Ryan from comment #23)
> Any chance for this to make it into the Beta build then? Between this bug
> and the black borders in input boxes on GTK 3.20+ I am stuck using 49 for
> the time being.

Which theme are you seeing this in?
How did you come to be using that theme?

Comment 25

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

Comment on attachment 8810705 [details]
bug 1315668 remove use of gtk_container_get_border_width from menuitems

GTK theme issue, let's try it in aurora.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e0666d53b25a
https://hg.mozilla.org/releases/mozilla-aurora/rev/333f3821396e
https://hg.mozilla.org/releases/mozilla-aurora/rev/10c574e6c343
https://hg.mozilla.org/releases/mozilla-aurora/rev/b77fb8918a6d

Comment 27

[Ryan]

(In reply to Karl Tomlinson (:karlt) from comment #24)
> (In reply to Ryan from comment #23)
> > Any chance for this to make it into the Beta build then? Between this bug
> > and the black borders in input boxes on GTK 3.20+ I am stuck using 49 for
> > the time being.
> 
> Which theme are you seeing this in?
> How did you come to be using that theme?

The Menta set themes. The Menta Theme as been the default theme for Mate Desktop out of the box for three years and one of the best looking IMO.

Comment 28

[Karl Tomlinson (:karlt)]

Attachment: 51beta part 4 - construct menuitem style contexts from paths

This is https://hg.mozilla.org/mozilla-central/rev/d09e8a3b7875 without the
MOZ_GTK_IMAGEMENUITEM changes.  They were unnecessary (and
MOZ_GTK_IMAGEMENUITEM is no longer used since 52 - bug 1319355), but will
conflict with the code on beta that was removed for Bug 1302157.

MozReview-Commit-ID: EFV7swWQtm4

Comment 29

[Karl Tomlinson (:karlt)]

Comment on attachment 8813485 [details] [diff] [review]
51beta part 4 - construct menuitem style contexts from paths

Approval Request Comment
[Feature/regressing bug #]:
This was first observed after changes for bug 1248974.
That caused Firefox 49 to not show the correct background on the first paint or two.
Firefox 50 continues to show the incorrect background.
[User impact if declined]:
Funny looking menus from menubar and context click.  The highlighted item will likely be hard to read.
This happens only with some themes, most notably Menta, which is the default theme for Mate Desktop.  I don't know the numbers of people using Mate.
It is popular enough to have its own flavor of Ubuntu.
https://distrowatch.com/table.php?distribution=ubuntumate
http://popcon.ubuntu.com/ would indicate it is installed in only about 0.5% of Ubuntu installations.  I wonder whether that includes Ubuntu MATE installations.
http://popcon.debian.org/ indicates 6-7%.
[Describe test coverage new/current, TreeHerder]:
Tested manually with problem theme.
[Risks and why]: 
Although I'm reasonably confident that what is changed here should work in general, there are many factors involved and it is hard to consider them all.  I've been surprised before.  I would only suggest taking these changes early in the cycle, so that there is time to adjust if issues do come up.
[String/UUID change made/needed]:
None.

Comment 30

[Karl Tomlinson (:karlt)]

Please consider comment 29 a request uplift of 4 patches: the first three that landed on central/aurora and attachment 8813485 [details] [diff] [review].

Comment 31

[Gerry Chang [:gchang]]

Comment on attachment 8813485 [details] [diff] [review]
51beta part 4 - construct menuitem style contexts from paths

Fix a regression related to themes. Beta51+. Should be in 51 beta 3.

Comment 32

[Carsten Book [:Tomcat]]

has problems to apply to beta when applying the first 4 patches and the one single as requested

applying https://bug1315668.bmoattachments.org/attachment.cgi?id=8813485
patching file widget/gtk/WidgetStyleCache.cpp
Hunk #1 FAILED at 19
Hunk #2 FAILED at 87
Hunk #3 FAILED at 315
Hunk #4 FAILED at 372
Hunk #5 FAILED at 416
Hunk #6 FAILED at 487
Hunk #7 FAILED at 512
Hunk #8 FAILED at 683
Hunk #9 FAILED at 755
Hunk #10 FAILED at 805
10 out of 10 hunks FAILED -- saving rejects to file widget/gtk/WidgetStyleCache.cpp.rej

Comment 33

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/releases/mozilla-beta/rev/427a01f3d6a4
https://hg.mozilla.org/releases/mozilla-beta/rev/9961ffa21ba6
https://hg.mozilla.org/releases/mozilla-beta/rev/c0681f9b8c05
https://hg.mozilla.org/releases/mozilla-beta/rev/3a4718648de7

Comment 34

[Gerry Chang [:gchang]]

This will be in 51 beta 4.

Comment 35

[Paul Silaghi, QA [:pauly]]

I didn't manage to install Mate Desktop properly on my Ubuntu 14.04.
Could you please confirm the problem is gone in the latest beta/nightly?
https://archive.mozilla.org/pub/firefox/candidates/51.0b11-candidates/build1/
https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-central/
Thanks.

Comment 36

[Andrei Vaida [:avaida]]

Removing qe-verify+, as there's nothing else we can do here to help (see Comment 35).