659993 - Highlight of on-screen tabs in the List All Tabs menu looks like visual glitch [Linux]

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Dão Gottwald [:dao]]

Attachment: screenshot

Comment 1

[Frank Yan (:fryn)]

Hmm, I'm not sure how to fix that.
Shall I just remove the styling for gnomestripe?

Comment 2

[Dão Gottwald [:dao]]

Yes, I think the gnomestripe part needs to be backed out.

Comment 3

[Kai Liu]

This problem isn't limited to gnomestripe; it also affects Windows when using classically-rendered menus.  Basically, anything that is not NT6.x with Aero looks broken.  So Luna, Classic, et al. are all affected.

Comment 4

[Kai Liu]

Filed bug 661846 for the Windows variant of this bug.

Comment 5

[Kai Liu]

The cause of the Linux variant of this bug is similar to that of the Windows variant (i.e., the use of a border width to internally pad up the widget).

Comment 6

[Kai Liu]

Attachment: Part 1: Expose a means to identify menuitems in a menulist

This patch exposes a function to let theme code in widget/ determine if a menuitem is a part of a regular, traditional menu, or if it's a part of a menulist.  In most platforms, regular and menulist menuitems are rendered differently (sometimes drastically so)...

Comment 7

[Kai Liu]

Attachment: Part 2: A better fix for bug 406129

Patch v4 of bug 404751 called GTK APIs to determine the horizontal padding of menuitems, but specifying a padding in GetWidgetPadding overrode the padding specified in menu.css.  Since menulist menuitems and regular menuitems all shared the same -moz-appearance, this meant that the padding set in GetWidgetPadding that was intended for regular menuitems was overriding the custom padding in menu.css for menulist menuitems.

An alternate fix would be to have GetWidgetPadding detect whether the menuitem is a part of a menulist or a regular menu and set the padding only in the latter case.  Much of this patch was copied from the v4 patch in bug 404751.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 538781 [details] [diff] [review]
Part 2: A better fix for bug 406129

Review of attachment 538781 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/src/gtk2/gtkdrawing.h
@@ +335,5 @@
> + *
> + * returns:    MOZ_GTK_SUCCESS if there was no error, an error code otherwise
> + */
> +gint
> +moz_gtk_menuitem_get_padding(gint* horizontal_padding);

get_horizontal_padding

@@ +338,5 @@
> +gint
> +moz_gtk_menuitem_get_padding(gint* horizontal_padding);
> +
> +gint
> +moz_gtk_checkmenuitem_get_padding(gint* horizontal_padding);

get_horizontal_padding

::: widget/src/gtk2/nsNativeThemeGTK.cpp
@@ +181,5 @@
> +// Returns true if it's not a menubar item or menulist item
> +static PRBool IsRegularMenuItem(nsIFrame *aFrame)
> +{
> +  nsIMenuFrame *menuFrame = do_QueryFrame(aFrame);
> +  if (menuFrame && (menuFrame->IsOnMenuBar() || menuFrame->IsInMenuList()))

Just write "return menuFrame && ...;"

Comment 9

[Kai Liu]

Attachment: Part 3: Consolidate the internal padding for regular menuitems in GetWidgetPadding

The internal spacing used by menuitems comes from the horizontal-padding style property and from the x and ythickness padding values of the GtkStyle, which we currently are adding to the widget via GetWidgetBorder.  By consolidating these padding values in GetWidgetPadding, this resolves the problem specified in this bug.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 538785 [details] [diff] [review]
Part 3: Consolidate the internal padding for regular menuitems in GetWidgetPadding

Review of attachment 538785 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/src/gtk2/nsNativeThemeGTK.cpp
@@ +945,5 @@
> +  case NS_THEME_RADIOMENUITEM:
> +    // For regular menuitems, use GetWidgetPadding instead of GetWidgetBorder
> +    // to pad up the widget's internals.
> +    if (IsRegularMenuItem(aFrame))
> +      break;

Add explicit comment that you're falling through

@@ +997,5 @@
> +        if (GetGtkWidgetAndState(aWidgetType, aFrame, gtkWidgetType, nsnull,
> +                                 nsnull))
> +          moz_gtk_get_widget_border(gtkWidgetType, &aResult->left, &aResult->top,
> +                                    &aResult->right, &aResult->bottom, GetTextDirection(aFrame),
> +                                    IsFrameContentNodeInNamespace(aFrame, kNameSpaceID_XHTML));

{} around the body

Comment 11

[Kai Liu]

(In reply to comment #10)
> {} around the body

Shall I make the same change under the GetWidgetBorder default case?  (I had imitated the style that was used there.)

Comment 12

[Kai Liu]

Attachment: Part 2 (v2): A better fix for bug 406129

Addressed comments.

Comment 13

[Kai Liu]

Attachment: Part 3 (v2): Consolidate the internal padding for regular menuitems in GetWidgetPadding [r=roc]

I went ahead and made the same brace fix for the same code under the GetWidgetPadding default case.

Comment 14

[Michael Ventnor]

Comment on attachment 538786 [details] [diff] [review]
Part 2 (v2): A better fix for bug 406129

Review of attachment 538786 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/src/gtk2/nsNativeThemeGTK.cpp
@@ +178,5 @@
>    return GTK_TEXT_DIR_NONE;
>  }
>  
> +// Returns true if it's not a menubar item or menulist item
> +static PRBool IsRegularMenuItem(nsIFrame *aFrame)

This should go in xpwidgets nsNativeTheme.

With that fixed, r=me.

Comment 15

[Kai Liu]

Attachment: Part 2 (v3): A better fix for bug 406129 [r=roc+ventnor]

With comments addressed.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 538779 [details] [diff] [review]
Part 1: Expose a means to identify menuitems in a menulist

IsInMenuList strongly suggests a boolean return.

How about calling it GetMenuParentType and having it return an enum?  Something like { eNotMenu, eReadonlyMenulist, eEditableMenuList }?

Comment 17

[Kai Liu]

Attachment: Part 1 (v2): Expose a means to identify menuitems in a menulist

I wasn't sure what the best name for the enum type would be.  Looking through MXR, there's eFoo, EFoo, and nsFoo.  Since there is already a nsMenuType in nsMenuFrame.h (for different types of menuitems), I decided to use the "ns" prefix here.

As for the name itself, you suggested MenuParentType, but of the four types of parent types--editable menulist, readonly menulist, menu, and not-a-menu (e.g., context menus)--we don't really care about the difference between menu and not-a-menu (since nsIMenuFrame is intended for use by theme code, and AFAICT, there is never a distinction made between the renderings of menubar menus and context menus).  So I instead opted to name it MenuListType and have it return only 3 types: editable menulist, readonly menulist, and not-a-menulist.  I'm not sure which approach you prefer.

(Another option would be to keep IsInMenuList and make that return a boolean, since, strictly speaking, theme code only cares about the menu/menulist distinction and doesn't care about editable/readonly.  The reason I added the editable/readonly distinction was so that nsMenuFrame::ShouldBlink could use this function and thus avoid code duplication.  But maybe avoiding the code duplication is not worth the slightly more complicated public interface?)

Comment 18

[Kai Liu]

Attachment: Part 2 (v3.1): A better fix for bug 406129 [r=roc+ventnor]

Synchronizing with the part-1 v2 patch.

Comment 19

[Kai Liu]

Hmm, I guess if we keep the trinary function, I should probably s/GetMenuListType/GetParentMenuListType/ so that it's clearer that the input is a menuitem and not a menulist...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 540211 [details] [diff] [review]
Part 1 (v2): Expose a means to identify menuitems in a menulist

Looks good!

Comment 21

[Kai Liu]

Attachment: [for checkin] Part 1: Expose a means to identify menuitems in a menulist [r=bz]

Updated commit message and addressed comment 19.

Comment 22

[Kai Liu]

Attachment: [for checkin] Part 2: A alternative fix for bug 406129 [r=roc+ventnor]

Updated commit message and sync'ed.

Comment 23

[Kai Liu]

Attachment: [for checkin] Part 3: Consolidate the internal padding for regular menuitems in GetWidgetPadding [r=roc]

Updated commit message.

Comment 24

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/810cfb27a823
http://hg.mozilla.org/integration/mozilla-inbound/rev/510560dbd1be
http://hg.mozilla.org/integration/mozilla-inbound/rev/1e2444688bd1

Comment 25

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/810cfb27a823
http://hg.mozilla.org/mozilla-central/rev/510560dbd1be
http://hg.mozilla.org/mozilla-central/rev/1e2444688bd1

Comment 26

[Vlad [QA]]

Setting resolution to Verified Fixed on Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0 beta 3

One issue that I've found on Linux is that you cannot scroll the tabbar and have in the same time the Tab Groups menu in focus.
Is this an issue?

Comment 27

[Frank Yan (:fryn)]

(In reply to Vlad [QA] from comment #26)
> Setting resolution to Verified Fixed on Mozilla/5.0 (X11; Linux x86_64;
> rv:7.0) Gecko/20100101 Firefox/7.0 beta 3
> 
> One issue that I've found on Linux is that you cannot scroll the tabbar and
> have in the same time the Tab Groups menu in focus.
> Is this an issue?

No. Thanks for testing it though. :)