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

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?

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

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

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

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.

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

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.

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/

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/

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/

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/

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

https://reviewboard.mozilla.org/r/53092/#review50000 Sorry, I mean the MOZ_GTK_MENUBARITEM from moz_gtk_get_widget_border().

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.

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.

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

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

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.

https://reviewboard.mozilla.org/r/53098/#review50228 > We may use only MOZ_GTK_MENUITEM for now. Changed to use MOZ_GTK_MENUITEM.

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

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

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/

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.

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

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

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.