Closed Bug 1272194 Opened 4 years ago Closed 4 years ago

[GTK3] Regression: highlighted menu items are invisible

Categories

(Core :: Widget: Gtk, defect)

49 Branch
x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox48 --- unaffected
firefox49 --- verified

People

(Reporter: stephen.moehle, Assigned: karlt)

References

Details

(Keywords: regression)

Attachments

(6 files)

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?
Blocks: 1248974
Keywords: regression
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
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/
Attachment #8753219 - Flags: review?(stransky)
Attachment #8753220 - Flags: review?(stransky)
Attachment #8753221 - Flags: review?(stransky)
Attachment #8753222 - Flags: review?(stransky)
Attachment #8753223 - Flags: review?(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/
Attachment #8753219 - Flags: review?(stransky) → review+
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?
Attachment #8753219 - Flags: review+
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.
Attachment #8753220 - Flags: review?(stransky) → review+
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.
Attachment #8753221 - Flags: review?(stransky) → review+
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.
Attachment #8753222 - Flags: review?(stransky) → review+
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.
Attachment #8753223 - Flags: review?(stransky)
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/
Attachment #8753219 - Attachment description: MozReview Request: bug 1272194 replace MOZ_TOPLEVEL_MENU_ITEM flag with MOZ_GTK_MENUBARITEM node r?stransky → MozReview Request: bug 1272194 replace MOZ_TOPLEVEL_MENU_ITEM flag with MOZ_GTK_MENUBARITEM node r=stransky
Attachment #8753220 - Attachment description: MozReview Request: bug 1272194 don't create a label for menu items r?stransky → MozReview Request: bug 1272194 don't create a label for menu items r=stransky
Attachment #8753221 - Attachment description: MozReview Request: bug 1272194 only add menubar class to menu items with GTK 3.4 r?stransky → MozReview Request: bug 1272194 only add menubar class to menu items with GTK 3.4 r=stransky
Attachment #8753222 - Attachment description: MozReview Request: bug 1272194 use WidgetStyleCache for menus r?stransky → MozReview Request: bug 1272194 use WidgetStyleCache for menus r=stransky
Attachment #8753219 - Flags: review?(stransky)
Attachment #8753223 - Flags: review?(stransky)
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.
Attachment #8753219 - Flags: review?(stransky) → review+
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.
Attachment #8753223 - Flags: review?(stransky) → review+
Okay, Thanks. I don't have requested permissions so please land those patches.
Duplicate of this bug: 1274298
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/
Attachment #8753223 - Attachment description: MozReview Request: bug 1272194 explicitly invalidate after change style contexts belonging to widgets r?stransky → MozReview Request: bug 1272194 explicitly invalidate after change style contexts belonging to widgets r=stransky
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.
Duplicate of this bug: 1274735
Assignee: nobody → karlt
Depends on: 1274745
Version: Trunk → 49 Branch
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.