Closed Bug 1315668 Opened 3 years ago Closed 3 years ago

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

Categories

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

49 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: vummiess, Assigned: karlt, NeedInfo)

References

()

Details

(Keywords: regression, Whiteboard: tpi:+)

Attachments

(7 files)

starting in Firefox 50, the menu highlight background color is missing in some themes (e.g. BlueMenta).
Blocks: gtk-3.20
Priority: -- → P2
Whiteboard: tpi:+
I see with even with Menta from mate-themes-3.18.3.
Assignee: nobody → karlt
No longer blocks: gtk-3.20
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: regression
Priority: P2 → P1
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.
Blocks: 1274745, 1301194
(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.
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.
Flags: needinfo?(karlt)
Yes, I agree styles from path is the best solution here.
I have some patches for that.
Flags: needinfo?(karlt)
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.
Attachment #8810705 - Flags: review?(stransky) → review+
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.
Attachment #8810706 - Flags: review?(stransky) → review+
Comment on attachment 8810708 [details]
bug 1315668 construct menuitem style contexts from paths

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

Fine here.
Attachment #8810708 - Flags: review?(stransky) → review+
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.
Attachment #8810707 - Flags: review?(stransky) → review+
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.
Blocks: 1287036
Hi Karl, should we uplift this fix to Aurora52/Beta51?
Flags: needinfo?(karlt)
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.
Flags: needinfo?(karlt)
Attachment #8810705 - Flags: approval-mozilla-aurora?
Summary: menu highlight background color is missing in some themes (e.g. BlueMenta) → menu highlight background color is missing in themes that animate menu item highlights (e.g. BlueMenta)
(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?
Flags: needinfo?(yixxt)
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.
Attachment #8810705 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Blocks: 1319355
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.
Flags: needinfo?(yixxt)
Attachment #8813485 - Flags: approval-mozilla-beta?
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.
Attachment #8813485 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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
Flags: needinfo?(karlt)
This will be in 51 beta 4.
Flags: needinfo?(karlt)
Version: 50 Branch → 49 Branch
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.
Flags: needinfo?(yixxt)
Flags: needinfo?(vummiess)
Depends on: 1333746
Removing qe-verify+, as there's nothing else we can do here to help (see Comment 35).
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.