255911 - native theme button cleanups for gtk2

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[Brian Ryner (not reading)]

There are a number of things that need to be cleaned up for gtk2 button,
toolbarbutton, and menubutton.  In particular:

- buttons need to honor the interior-focus, focus-line-width, and focus-padding
style properties
- buttons need to include focus padding in their border
- CSS padding for buttons' button-box, and for toolbarbutton, needs to _not_
apply when native theming is enabled since all of the padding is included in the
native theme border
- gnomestripe theme needs the dualbutton and dualbutton-dropdown moz-appearance
restored for menubuttons

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Rather than adding more interior boxes, as I did for checkboxes and
radiobuttons awhile back, I decided to take a better approach to supporting
having CSS style apply when native theming is disabled.  The main problem
currently is that GetWidgetBorder() is expected to include the padding as well.
 This means that you can't use CSS padding on that element for the case when
native theming is disabled, since it will be added in on top of the native
theme border when native theming _is_ enabled.

To try to alleviate this, I added nsITheme::GetWidgetPadding().  Eventually the
goal is to actually separate out the border and padding values that nsITheme
returns.  But in the meantime, I'm just using it as a way to say "when native
theming is enabled for this widget, force the CSS padding to 0."

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 156363 [details] [diff] [review]
patch

>+// An interior box in a button, used for painting a focus outline.
>+#define NS_THEME_BUTTON_INTERIOR                           154

Why not just call this NS_THEME_BUTTON_FOCUS ?

r=me for the rest.  Make sure you have a followup bug filed.

Comment 3

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

Comment on attachment 156363 [details] [diff] [review]
patch

+	     x += gButtonWidget->style->xthickness + focus_pad;
+	     y += gButtonWidget->style->ythickness + focus_pad;

why gButtonWidget and not widget?

+    nsIContent* content = frame->GetContent();
+    if (content) {
+      nsCOMPtr<nsIDocument> doc = content->GetDocument();
+      if (doc) {
+	 nsIPresShell *shell = doc->GetShellAt(0);
+	 nsPresContext *context = shell->GetPresContext();

Why not just frame->GetPresContext()?

Comment 4

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

Comment on attachment 156363 [details] [diff] [review]
patch

bryner agreed with me on IRC

Comment 5

[Brian Ryner (not reading)]

I made one more change to the way NS_THEME_TOOLBAR_DUAL_BUTTON is handled.  When
you hover over the toolbar button, I want a border to draw all the way around
the entire button, then a border around the inner button if you're over that
(which will overlap with the outer border on the top and left).  This is how it
looks on Windows, and is really the only way I see to get good hover feedback
without making these things be too tall.  Here's what I added to achieve this:

@@ -337,6 +331,7 @@ nsNativeThemeGTK::GetGtkWidgetAndState(P
   switch (aWidgetType) {
   case NS_THEME_BUTTON:
   case NS_THEME_TOOLBAR_BUTTON:
+  case NS_THEME_TOOLBAR_DUAL_BUTTON:
     if (aWidgetFlags)
       *aWidgetFlags = (aWidgetType == NS_THEME_BUTTON) ? GTK_RELIEF_NORMAL :
GTK_RELIEF_NONE;
     aGtkWidgetType = MOZ_GTK_BUTTON;
@@ -543,6 +538,14 @@ nsNativeThemeGTK::GetWidgetBorder(nsIDev
     // gtk's 'toolbar' for purposes of painting the widget background,
     // we don't use the toolbar border for toolbox.
     break;
+  case NS_THEME_TOOLBAR_DUAL_BUTTON:
+    // TOOLBAR_DUAL_BUTTON is an interesting case.  We want a border to draw
+    // around the entire button + dropdown, and also an inner border if you're
+    // over the button part.  But, we want the inner button to be right up
+    // against the edge of the outer button so that the borders overlap.
+    // To make this happen, we draw a button border for the outer button,
+    // but don't reserve any space for it.
+    break;
   default:
     {
       GtkThemeWidgetType gtkWidgetType;

Comment 6

[Brian Ryner (not reading)]

(In reply to comment #5)
> (which will overlap with the outer border on the top and left)
                                               ^^^^^^^^^^^^^^^^^

and bottom

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 156363 [details] [diff] [review]
patch

a=[e-mail]

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 156363 [details] [diff] [review]
patch

oops, wrong flag.

Comment 9

[Brian Ryner (not reading)]

Attachment: patch merged to aviary branch

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 157987 [details] [diff] [review]
patch merged to aviary branch

requesting branch approval

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 157987 [details] [diff] [review]
patch merged to aviary branch

requesting 1.7 approval as well

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 157987 [details] [diff] [review]
patch merged to aviary branch

a=asa for aviary checkin.

Comment 13

[Brian Ryner (not reading)]

checked in on the trunk, aviary, and 1.7 branches.