Last Comment Bug 659993 - Highlight of on-screen tabs in the List All Tabs menu looks like visual glitch [Linux]
: Highlight of on-screen tabs in the List All Tabs menu looks like visual glitc...
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: mozilla7
Assigned To: Kai Liu
:
Mentors:
Depends on:
Blocks: 626903
  Show dependency treegraph
 
Reported: 2011-05-26 09:55 PDT by Dão Gottwald [:dao]
Modified: 2013-11-12 00:56 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screenshot (50.70 KB, image/png)
2011-05-26 09:55 PDT, Dão Gottwald [:dao]
no flags Details
Part 1: Expose a means to identify menuitems in a menulist (3.73 KB, patch)
2011-06-12 14:19 PDT, Kai Liu
bzbarsky: review-
Details | Diff | Review
Part 2: A better fix for bug 406129 (7.50 KB, patch)
2011-06-12 14:27 PDT, Kai Liu
roc: review+
Details | Diff | Review
Part 3: Consolidate the internal padding for regular menuitems in GetWidgetPadding (2.65 KB, patch)
2011-06-12 14:34 PDT, Kai Liu
roc: review+
Details | Diff | Review
Part 2 (v2): A better fix for bug 406129 (7.53 KB, patch)
2011-06-12 15:02 PDT, Kai Liu
ventnor.bugzilla: review+
Details | Diff | Review
Part 3 (v2): Consolidate the internal padding for regular menuitems in GetWidgetPadding [r=roc] (3.01 KB, patch)
2011-06-12 15:05 PDT, Kai Liu
no flags Details | Diff | Review
Part 2 (v3): A better fix for bug 406129 [r=roc+ventnor] (9.06 KB, patch)
2011-06-12 19:08 PDT, Kai Liu
no flags Details | Diff | Review
Part 1 (v2): Expose a means to identify menuitems in a menulist (3.92 KB, patch)
2011-06-17 23:03 PDT, Kai Liu
bzbarsky: review+
Details | Diff | Review
Part 2 (v3.1): A better fix for bug 406129 [r=roc+ventnor] (9.11 KB, patch)
2011-06-17 23:06 PDT, Kai Liu
no flags Details | Diff | Review
[for checkin] Part 1: Expose a means to identify menuitems in a menulist [r=bz] (3.97 KB, patch)
2011-06-22 17:43 PDT, Kai Liu
no flags Details | Diff | Review
[for checkin] Part 2: A alternative fix for bug 406129 [r=roc+ventnor] (9.12 KB, patch)
2011-06-22 17:44 PDT, Kai Liu
no flags Details | Diff | Review
[for checkin] Part 3: Consolidate the internal padding for regular menuitems in GetWidgetPadding [r=roc] (3.01 KB, patch)
2011-06-22 17:45 PDT, Kai Liu
no flags Details | Diff | Review

Description Dão Gottwald [:dao] 2011-05-26 09:55:53 PDT
Created attachment 535376 [details]
screenshot
Comment 1 Frank Yan (:fryn) 2011-05-26 16:17:05 PDT
Hmm, I'm not sure how to fix that.
Shall I just remove the styling for gnomestripe?
Comment 2 Dão Gottwald [:dao] 2011-05-27 00:45:11 PDT
Yes, I think the gnomestripe part needs to be backed out.
Comment 3 Kai Liu 2011-06-03 07:36:53 PDT
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 2011-06-03 09:02:06 PDT
Filed bug 661846 for the Windows variant of this bug.
Comment 5 Kai Liu 2011-06-12 14:15:50 PDT
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 2011-06-12 14:19:35 PDT
Created attachment 538779 [details] [diff] [review]
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 2011-06-12 14:27:36 PDT
Created attachment 538781 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2011-06-12 14:33:43 PDT
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 2011-06-12 14:34:10 PDT
Created attachment 538785 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2011-06-12 14:39:38 PDT
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 2011-06-12 14:47:33 PDT
(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 2011-06-12 15:02:47 PDT
Created attachment 538786 [details] [diff] [review]
Part 2 (v2): A better fix for bug 406129

Addressed comments.
Comment 13 Kai Liu 2011-06-12 15:05:32 PDT
Created attachment 538789 [details] [diff] [review]
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 2011-06-12 18:02:53 PDT
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 2011-06-12 19:08:13 PDT
Created attachment 538798 [details] [diff] [review]
Part 2 (v3): A better fix for bug 406129 [r=roc+ventnor]

With comments addressed.
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-17 14:20:20 PDT
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 2011-06-17 23:03:21 PDT
Created attachment 540211 [details] [diff] [review]
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 2011-06-17 23:06:01 PDT
Created attachment 540212 [details] [diff] [review]
Part 2 (v3.1): A better fix for bug 406129 [r=roc+ventnor]

Synchronizing with the part-1 v2 patch.
Comment 19 Kai Liu 2011-06-18 07:18:58 PDT
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 [:bz] (Out June 25-July 6) 2011-06-22 15:16:29 PDT
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 2011-06-22 17:43:46 PDT
Created attachment 541241 [details] [diff] [review]
[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 2011-06-22 17:44:59 PDT
Created attachment 541242 [details] [diff] [review]
[for checkin] Part 2: A alternative fix for bug 406129 [r=roc+ventnor]

Updated commit message and sync'ed.
Comment 23 Kai Liu 2011-06-22 17:45:43 PDT
Created attachment 541243 [details] [diff] [review]
[for checkin] Part 3: Consolidate the internal padding for regular menuitems in GetWidgetPadding [r=roc]

Updated commit message.
Comment 26 Vlad [QA] 2011-08-31 07:25:54 PDT
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) 2011-08-31 14:55:11 PDT
(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. :)

Note You need to log in before you can comment on or make changes to this bug.