1272194 - [GTK3] Regression: highlighted menu items are invisible

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Stephen Moehle]

Highlighted menu items on the drop down menus from the menu bar and context menus are invisible. This is a recent regression within the last day. It looks like the highlight, as one mouses over a menu item, changes the text color to be the background color.

The highlighted item on a drop down list is invisible as well.

Mozregression gives me:

 5:47.48 INFO: Last good revision: a95651b07928723aaac20f74e9504528ef4d44ee
 5:47.48 INFO: First bad revision: 0504bc4398f643b6b7c4a6f3cbf9e435b732b384
 5:47.48 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a95651b07928723aaac20f74e9504528ef4d44ee&tochange=0504bc4398f643b6b7c4a6f3cbf9e435b732b384

The culprit seems to be bug 1248974.

Firefox nightly build 2016-05-11
Fedora Linux 22 64-bit
GTK3 3.16.7

Comment 1

[Karl Tomlinson (:karlt)]

Thanks for the report and the regression range.

I'm not reproducing here (Adwaita 3.18.7).
Can you tell me the GTK theme, please?

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Also reproducing on Debian stable (8/Jessie) with KDE desktop. GTK3 theme is "default" which I think is oxygen-gtk in this configuration.

Comment 3

[rsx11m]

Also reproducible with SeaMonkey 2.46a1 Linux x86_64 nightly build on OpenSUSE 13.2 (default KDE 4 desktop [oxygen] and default application theme).

Comment 4

[magicp]

Attachment: Ubuntu 15.10 (64 bit).mp4

I confirmed this issue on Ubuntu 15.10 (64 bit) with Nightly build 20160512030253

Comment 5

[Stephen Moehle]

I am using Clearlooks-Phenix under Xfce4. Other gtk3 apps, such as gnome-terminal, do not have any problems with their menus. One both gnome-terminal and Firefox, the menus are black text on a light grey background. The menu item highlight for gnome-terminal is white text on a blue background, the same as Firefox before the regression.

And on the subject of bug 1248974, the menubar highlighting of Firefox now matches that of gnome-terminal, with one very important exception. The highlighted menubar item in gnome-terminal has a thick (3 pixels maybe) underline which Firefox is missing. Without the underline, the difference between highlighted and not menubar items is much too subtle.

Comment 6

[rsx11m]

(In reply to Stephen Moehle from comment #5)
> the menus are black text on a light grey background.

I see white text on white background with KDE 4 and oxygen, if that helps.
It used to be a dark blue background (still seeing that on aurora).

Comment 7

[Karl Tomlinson (:karlt)]

Although
https://git.gnome.org/browse/gtk+/commit/?id=0e6a9858e1801846d9fd5fc5bcb59eee6e65827f
says "Style contexts are invalidated automatically." since at least 3.12,
the automatic invalidation of contexts for widgets was performed off an event.
That model was introduced for 3.6 in
https://git.gnome.org/browse/gtk+/commit/?id=585a1fae4f347d979c55f2848cd3afba7dc32c2e
https://git.gnome.org/browse/gtk+/commit/?id=40982eabbb6bc7515cf045c4c85eb18df42c125b
It did not apply to saved style contexts or contexts without widgets
https://git.gnome.org/browse/gtk+/tree/gtk/gtkstylecontext.c?h=3.6.0#n1093
https://git.gnome.org/browse/gtk+/tree/gtk/gtkstylecontext.c?h=3.6.0#n3240
The model seems to have changed to on-demand invalidation from 3.18, as this
bug does not exist there.

gtk_style_context_invalidate() in moz_gtk_menu_item_paint() after set_state() is necessary resolves, but I still need to see what else might be affected.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1272194 replace MOZ_TOPLEVEL_MENU_ITEM flag with MOZ_GTK_MENUBARITEM node r=stransky

This provides a better mapping between WidgetNodeType and GtkWidgets.

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

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1272194 don't create a label for menu items r=stransky

The label dates back to GTK2 code and is not currently used.
Removing will allow sharing code with menubar item creation.

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

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1272194 only add menubar class to menu items with GTK 3.4 r=stransky

and save/restore context consistent with GTK 3.4.

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

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1272194 use WidgetStyleCache for menus r=stransky

Unnecessary widget realization is also removed.

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

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1272194 explicitly invalidate after change style contexts belonging to widgets r=stransky

This fixes menu item rendering during hover.

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

Comment 14

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

Comment on attachment 8753219 [details]
MozReview Request: bug 1272194 replace MOZ_TOPLEVEL_MENU_ITEM flag with MOZ_GTK_MENUBARITEM node r=stransky

https://reviewboard.mozilla.org/r/53092/#review49988

Looks fine.

Comment 15

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

Comment on attachment 8753219 [details]
MozReview Request: bug 1272194 replace MOZ_TOPLEVEL_MENU_ITEM flag with MOZ_GTK_MENUBARITEM node r=stransky

https://reviewboard.mozilla.org/r/53092/#review50000

Actually it causes a regression in menu bar rendering. The border for MOZ_GTK_MENUBAR returned by moz_gtk_get_widget_border() is 8px for left/right which does not look correct and it's extra wide, compared to other gtk3 apps.

I see the previous version does not calculate border for MOZ_GTK_MENUBAR at all. I suggest to use that approach. Or shall we calculate the border as a container borders from gMenuBarWidget, not gMenuBarItemWidget?

Comment 16

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

Comment on attachment 8753220 [details]
MozReview Request: bug 1272194 don't create a label for menu items r=stransky

https://reviewboard.mozilla.org/r/53094/#review50002

Looks OK when the border size is fixed.

Comment 17

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

https://reviewboard.mozilla.org/r/53092/#review50000

Sorry, I mean the MOZ_GTK_MENUBARITEM from moz_gtk_get_widget_border().

Comment 18

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

Comment on attachment 8753221 [details]
MozReview Request: bug 1272194 only add menubar class to menu items with GTK 3.4 r=stransky

https://reviewboard.mozilla.org/r/53096/#review50010

Fine.

Comment 19

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

https://reviewboard.mozilla.org/r/53092/#review50000

I see that we use the gMenuItemWidget for the toolbar menu items recently, which is not correct and the border is slightly bigger than the one in gtk3-widget-factory. Maybe we can keep the gMenuItemWidget for the toolbar menu items border for now and address that in another patch.

Comment 20

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

https://reviewboard.mozilla.org/r/53092/#review50226

::: widget/gtk/gtk3drawing.cpp:2817
(Diff revision 1)
>      case MOZ_GTK_RADIOMENUITEM:
>          {
> -            if (widget == MOZ_GTK_MENUITEM) {
> +            if (widget == MOZ_GTK_MENUBARITEM) {
> -                ensure_menu_item_widget();
>                  ensure_menu_bar_item_widget();
> +                w = gMenuBarItemWidget;

Actually it causes a regression in menu bar rendering. With this change the MOZ_GTK_MENUBARITEM border is 8px for left/right which does extra space in menus.

I see the previous version calculate that border from gMenuItemWidget. It is not correct, 
but we can keep it for now and address that in another patch.

Comment 21

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

Comment on attachment 8753222 [details]
MozReview Request: bug 1272194 use WidgetStyleCache for menus r=stransky

https://reviewboard.mozilla.org/r/53098/#review50228

r+ with the MOZ_GTK_MENUITEM border change.

::: widget/gtk/gtk3drawing.cpp
(Diff revision 1)
>      case MOZ_GTK_CHECKMENUITEM:
>      case MOZ_GTK_RADIOMENUITEM:
>          {
> -            if (widget == MOZ_GTK_MENUBARITEM) {
> -                ensure_menu_bar_item_widget();
> +            if (widget == MOZ_GTK_MENUBARITEM || widget == MOZ_GTK_MENUITEM) {
> +                w = GetWidget(widget);
> -                w = gMenuBarItemWidget;

We may use only MOZ_GTK_MENUITEM for now.

Comment 22

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

Comment on attachment 8753223 [details]
MozReview Request: bug 1272194 explicitly invalidate after change style contexts belonging to widgets r=stransky

https://reviewboard.mozilla.org/r/53100/#review50238

::: widget/gtk/WidgetStyleCache.cpp:296
(Diff revision 1)
> +  if (oldState != aStateFlags || oldDirection != aDirection) {
> +    // From GTK 3.8, set_state() will overwrite the direction, so set
> +    // direction after state.
> -  gtk_style_context_set_state(style, aStateFlags);
> +    gtk_style_context_set_state(style, aStateFlags);
> -  gtk_style_context_set_direction(style, aDirection);
> +    gtk_style_context_set_direction(style, aDirection);
> +

If we do this, can we fix that correcly for > 3.8?
Don't use gtk_style_context_set_direction() but add the GTK_STATE_FLAG_DIR_LTR/GTK_STATE_FLAG_DIR_RTL states.

::: widget/gtk/WidgetStyleCache.cpp:305
(Diff revision 1)
> +    //
> +    // https://bugzilla.mozilla.org/show_bug.cgi?id=1272194#c7
> +    //
> +    // Avoid calling invalidate on saved contexts to avoid performing
> +    // build_properties() (in 3.16 stylecontext.c) unnecessarily early.
> +    if (sWidgetStorage[aNodeType]) {

Don't you mean !sWidgetStorage[aNodeType] here? sWidgetStorage[aNodeType] is true for base widgets which styles are not saved.

But I think it's better to check sStyleContextNeedsRestore directly here.

Comment 23

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

https://reviewboard.mozilla.org/r/53100/#review50238

> Don't you mean !sWidgetStorage[aNodeType] here? sWidgetStorage[aNodeType] is true for base widgets which styles are not saved.
> 
> But I think it's better to check sStyleContextNeedsRestore directly here.

Ahh, sorry, I miread the commend, the check is correct. But I think is still better to check the sStyleContextNeedsRestore instead of sWidgetStorage[aNodeType.

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 8753219 [details]
MozReview Request: bug 1272194 replace MOZ_TOPLEVEL_MENU_ITEM flag with MOZ_GTK_MENUBARITEM node r=stransky

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

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 8753220 [details]
MozReview Request: bug 1272194 don't create a label for menu items r=stransky

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

Comment 26

[Karl Tomlinson (:karlt)]

Comment on attachment 8753221 [details]
MozReview Request: bug 1272194 only add menubar class to menu items with GTK 3.4 r=stransky

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

Comment 27

[Karl Tomlinson (:karlt)]

Comment on attachment 8753222 [details]
MozReview Request: bug 1272194 use WidgetStyleCache for menus r=stransky

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

Comment 28

[Karl Tomlinson (:karlt)]

Comment on attachment 8753223 [details]
MozReview Request: bug 1272194 explicitly invalidate after change style contexts belonging to widgets r=stransky

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

Comment 29

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/53092/#review50226

> Actually it causes a regression in menu bar rendering. With this change the MOZ_GTK_MENUBARITEM border is 8px for left/right which does extra space in menus.
> 
> I see the previous version calculate that border from gMenuItemWidget. It is not correct, 
> but we can keep it for now and address that in another patch.

Thanks for catching that.  Keeping gMenuItemWidget for now and filed bug 1274143 for MOZ_GTK_MENUBARITEM.

Comment 30

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/53098/#review50228

> We may use only MOZ_GTK_MENUITEM for now.

Changed to use MOZ_GTK_MENUITEM.

Comment 31

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/53100/#review50238

> If we do this, can we fix that correcly for > 3.8?
> Don't use gtk_style_context_set_direction() but add the GTK_STATE_FLAG_DIR_LTR/GTK_STATE_FLAG_DIR_RTL states.

That would be separate change, but I'd prefer not to have separate code paths
for different versions when a single code path works.  The current code
produces the correct style.  The only advantage of having a separate path for
3.8 would be that the context is changed only once instead of twice.  GTK's
separate phases for marking the context dirty and rebuilding should mean that
rebuilding happens only once, and the cost of marking dirty twice should be
small relative to the cost of rebuilding.

> Ahh, sorry, I miread the commend, the check is correct. But I think is still better to check the sStyleContextNeedsRestore instead of sWidgetStorage[aNodeType.

I changed to !sStyleContextNeedsRestore.  It's not exactly the same code flow as sWidgetStorage[aNodeType] because 3.20+ has some unsaved contexts that do not belong to widgets, but the extra invalidate calls should be harmless.

Comment 32

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

Comment on attachment 8753219 [details]
MozReview Request: bug 1272194 replace MOZ_TOPLEVEL_MENU_ITEM flag with MOZ_GTK_MENUBARITEM node r=stransky

https://reviewboard.mozilla.org/r/53092/#review50632

Great, Thanks.

Comment 33

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

Comment on attachment 8753223 [details]
MozReview Request: bug 1272194 explicitly invalidate after change style contexts belonging to widgets r=stransky

https://reviewboard.mozilla.org/r/53100/#review50634

Thanks.

Comment 34

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

Okay, Thanks. I don't have requested permissions so please land those patches.

Comment 36

[Karl Tomlinson (:karlt)]

Comment on attachment 8753223 [details]
MozReview Request: bug 1272194 explicitly invalidate after change style contexts belonging to widgets r=stransky

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

Comment 37

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/53100/#review50810

::: widget/gtk/WidgetStyleCache.cpp:283
(Diff revisions 2 - 3)
> +  MOZ_ASSERT(!sStyleContextNeedsRestore);
>    GtkStyleContext* style = GetStyleInternal(aNodeType);
>  #ifdef DEBUG
> -  MOZ_ASSERT(!sStyleContextNeedsRestore);

I moved this new assert to the right place.

Comment 38

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b40354c864f9027b8c1b540f49f3200e3244a4f5

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/169369e8ef8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/5299d183263b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3909bcf5501b
https://hg.mozilla.org/integration/mozilla-inbound/rev/764650604afd
https://hg.mozilla.org/integration/mozilla-inbound/rev/53e791a65fa2

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/169369e8ef8a
https://hg.mozilla.org/mozilla-central/rev/5299d183263b
https://hg.mozilla.org/mozilla-central/rev/3909bcf5501b
https://hg.mozilla.org/mozilla-central/rev/764650604afd
https://hg.mozilla.org/mozilla-central/rev/53e791a65fa2

Comment 42

[Bogdan Maris, Desktop QA]

Reproduced the initial issue using old Nightly (2016-05-12) on Ubuntu 15.10 x64, verified that the issue does not reproduce anymore using Firefox 49 beta 9 on Ubuntu 15.10 x64.