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

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

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.

(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.

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.

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

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.

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

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 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 on attachment 8810708 [details] bug 1315668 construct menuitem style contexts from paths https://reviewboard.mozilla.org/r/92992/#review93094 Fine here.

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.

Pushed by ktomlinson@mozilla.com: 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

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.

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

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

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 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.

(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.

(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 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.

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

(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.

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 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.

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 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.

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

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

This will be in 51 beta 4.

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.

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