[GTK3] Regression: highlighted menu items are invisible

VERIFIED FIXED in Firefox 49

Status

()

Core
Widget: Gtk
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: Stephen Moehle, Assigned: karlt)

Tracking

({regression})

49 Branch
mozilla49
x86_64
Linux
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 unaffected, firefox49 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Reporter)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
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
(Assignee)

Updated

2 years ago
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.

Comment 3

2 years ago
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

2 years ago
Created attachment 8751855 [details]
Ubuntu 15.10 (64 bit).mp4

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

Updated

2 years ago
Status: UNCONFIRMED → NEW
status-firefox49: --- → affected
Ever confirmed: true
(Reporter)

Comment 5

2 years ago
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

2 years ago
(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).
(Assignee)

Comment 7

2 years ago
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.

Updated

2 years ago
Duplicate of this bug: 1273277
(Assignee)

Comment 9

2 years ago
Created attachment 8753219 [details]
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/
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)
(Assignee)

Comment 10

2 years ago
Created attachment 8753220 [details]
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/
(Assignee)

Comment 11

2 years ago
Created attachment 8753221 [details]
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/
(Assignee)

Comment 12

2 years ago
Created attachment 8753222 [details]
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/
(Assignee)

Comment 13

2 years ago
Created attachment 8753223 [details]
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/
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.
(Assignee)

Comment 24

2 years ago
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)
(Assignee)

Comment 25

2 years ago
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/
(Assignee)

Comment 26

2 years ago
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/
(Assignee)

Comment 27

2 years ago
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/
(Assignee)

Comment 28

2 years ago
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/
(Assignee)

Comment 29

2 years ago
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.
(Assignee)

Comment 30

2 years ago
https://reviewboard.mozilla.org/r/53098/#review50228

> We may use only MOZ_GTK_MENUITEM for now.

Changed to use MOZ_GTK_MENUITEM.
(Assignee)

Comment 31

2 years ago
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.
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1274298
(Assignee)

Comment 36

2 years ago
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
(Assignee)

Comment 37

2 years ago
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.
(Assignee)

Comment 38

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

Comment 39

2 years ago
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

Updated

2 years ago
Duplicate of this bug: 1274735

Comment 41

2 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee: nobody → karlt
(Assignee)

Updated

2 years ago
Depends on: 1274745
(Assignee)

Updated

a year ago
status-firefox48: --- → unaffected
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
status-firefox49: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.