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

VERIFIED FIXED in mozilla7

Status

()

Core
Widget: Gtk
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: dao, Assigned: Kai Liu)

Tracking

Trunk
mozilla7
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 8 obsolete attachments)

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
(Reporter)

Description

6 years ago
Created attachment 535376 [details]
screenshot

Comment 1

6 years ago
Hmm, I'm not sure how to fix that.
Shall I just remove the styling for gnomestripe?
(Reporter)

Comment 2

6 years ago
Yes, I think the gnomestripe part needs to be backed out.
(Assignee)

Comment 3

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

Comment 4

6 years ago
Filed bug 661846 for the Windows variant of this bug.
(Assignee)

Comment 5

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

Comment 6

6 years ago
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)...
Assignee: nobody → kliu
Status: NEW → ASSIGNED
Attachment #538779 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

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

Comment 9

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

Comment 11

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

Comment 12

6 years ago
Created attachment 538786 [details] [diff] [review]
Part 2 (v2): A better fix for bug 406129

Addressed comments.
Attachment #538781 - Attachment is obsolete: true
Attachment #538781 - Flags: review?(ventnor.bugzilla)
Attachment #538786 - Flags: review?(ventnor.bugzilla)
(Assignee)

Comment 13

6 years ago
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.
Attachment #538785 - Attachment is obsolete: true

Comment 14

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

Comment 15

6 years ago
Created attachment 538798 [details] [diff] [review]
Part 2 (v3): A better fix for bug 406129 [r=roc+ventnor]

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-
(Assignee)

Comment 17

6 years ago
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?)
Attachment #538779 - Attachment is obsolete: true
Attachment #540211 - Flags: review?(bzbarsky)
(Assignee)

Comment 18

6 years ago
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.
Attachment #538798 - Attachment is obsolete: true
(Assignee)

Comment 19

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

Comment 21

6 years ago
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.
Attachment #540211 - Attachment is obsolete: true
(Assignee)

Comment 22

6 years ago
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.
Attachment #540212 - Attachment is obsolete: true
(Assignee)

Comment 23

6 years ago
Created attachment 541243 [details] [diff] [review]
[for checkin] Part 3: Consolidate the internal padding for regular menuitems in GetWidgetPadding [r=roc]

Updated commit message.
Attachment #538789 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
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/integration/mozilla-inbound/rev/810cfb27a823
http://hg.mozilla.org/integration/mozilla-inbound/rev/510560dbd1be
http://hg.mozilla.org/integration/mozilla-inbound/rev/1e2444688bd1
Keywords: checkin-needed
Whiteboard: [inbound]
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]

Comment 26

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

Comment 27

6 years ago
(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.