Closed Bug 659993 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Widget: Gtk, defect)

All
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla7

People

(Reporter: dao, Assigned: kliu)

References

Details

Attachments

(4 files, 8 obsolete files)

50.70 KB, image/png
Details
3.97 KB, patch
Details | Diff | Splinter Review
9.12 KB, patch
Details | Diff | Splinter Review
3.01 KB, patch
Details | Diff | Splinter Review
Attached image screenshot
No description provided.
Hmm, I'm not sure how to fix that.
Shall I just remove the styling for gnomestripe?
Yes, I think the gnomestripe part needs to be backed out.
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.
Filed bug 661846 for the Windows variant of this bug.
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).
Component: Theme → Widget: Gtk
Product: Firefox → Core
QA Contact: theme → gtk
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)...
Assignee: nobody → kliu
Status: NEW → ASSIGNED
Attachment #538779 - Flags: review?(bzbarsky)
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.
Attachment #538781 - Flags: review?(roc)
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 && ...;"
Attachment #538781 - Flags: review?(ventnor.bugzilla)
Attachment #538781 - Flags: review?(roc)
Attachment #538781 - Flags: review+
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.
Attachment #538785 - Flags: review?(roc)
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
Attachment #538785 - Flags: review?(roc) → review+
(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.)
Addressed comments.
Attachment #538781 - Attachment is obsolete: true
Attachment #538781 - Flags: review?(ventnor.bugzilla)
Attachment #538786 - Flags: review?(ventnor.bugzilla)
I went ahead and made the same brace fix for the same code under the GetWidgetPadding default case.
Attachment #538785 - Attachment is obsolete: true
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.
Attachment #538786 - Flags: review?(ventnor.bugzilla) → review+
With comments addressed.
Attachment #538786 - Attachment is obsolete: true
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 }?
Attachment #538779 - Flags: review?(bzbarsky) → review-
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?)
Attachment #538779 - Attachment is obsolete: true
Attachment #540211 - Flags: review?(bzbarsky)
Synchronizing with the part-1 v2 patch.
Attachment #538798 - Attachment is obsolete: true
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 on attachment 540211 [details] [diff] [review]
Part 1 (v2): Expose a means to identify menuitems in a menulist

Looks good!
Attachment #540211 - Flags: review?(bzbarsky) → review+
Updated commit message and addressed comment 19.
Attachment #540211 - Attachment is obsolete: true
Updated commit message and sync'ed.
Attachment #540212 - Attachment is obsolete: true
Keywords: checkin-needed
Summary: Highlight of on-screen tabs in the List All Tabs menu looks like visual glitch → Highlight of on-screen tabs in the List All Tabs menu looks like visual glitch [Linux]
http://hg.mozilla.org/mozilla-central/rev/810cfb27a823
http://hg.mozilla.org/mozilla-central/rev/510560dbd1be
http://hg.mozilla.org/mozilla-central/rev/1e2444688bd1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Whiteboard: [inbound]
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?
Status: RESOLVED → VERIFIED
(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. :)
You need to log in before you can comment on or make changes to this bug.