Martin Stránský [:stransky] (ni? me) Reporter
	Description • 9 years ago

The Gtk3 Firefox has way too much padding (space) between icons on the personal toolbar (and maybe elsewhere, in other toolbars), compared to the gtk2 Firefox. The buttons used for bookmark toolbar elements has more border/padding than the Gtk2 ones.

	storrgie
	Comment 1 • 9 years ago

Wanted to also chime in, this is a big bummer to see in Fedora 22. Here is an image example: http://i.imgur.com/zNNqd5X.png

	Martin Stránský [:stransky] (ni? me) Reporter
	Comment 2 • 9 years ago

Which setup for the bookmark tab do you use? I looks like you have only icons there, no text.

	Martin Stránský [:stransky] (ni? me) Reporter
	Comment 3 • 9 years ago

Do you use any bookmark extension?

	Martin Stránský [:stransky] (ni? me) Reporter
	Comment 4 • 9 years ago

Well, the problem is that padding returned from Gtk3 is just huge. The Gtk2 Firefox gets left/right padding 6 pixels, but Gtk3 one gets 12. That's 12 extra pixels per button. Unfortunately we can't distinguish between buttons on bookmark bar and other XUL buttons. 

Possible solution may be to restrict maximal padding to some reasonable value (like the 6 pixels we have in Gtk2). Karl, what do you think?

	Guillaume Majbe
	Comment 6 • 9 years ago

Hello, same bug here.

For the record : no extension needed to have no text for bookmarks - just empty the "Name" attribute when saving bookmark.

Thanks for your patch !

	storrgie
	Comment 7 • 9 years ago

@Martin: I don't use any extension. I have a default configuration and I have all of my website icons with no information in the 'name' field. I have some folder icons, which you can't see, that have names.

I did find a work around temporarily in this extension: https://addons.mozilla.org/en-US/firefox/addon/roomy-bookmarks-toolbar/

I would prefer that I don't have to use extensions though, anything added to the base install can create issues.

I would also like to report that the top right buttons are _huge_ as well as the scroll bar. It's highly disruptive to the eyes when a page (such as gmail) has a scroll bar in the middle (such as hangouts on the bottom left). Here is a screen shot of me adding to this issue, you can see the scroll bar to the right of the text field as well as to the far right of the screen: http://i.imgur.com/YQHyGrI.jpg

	Karl Tomlinson (:karlt)
	Comment 8 • 9 years ago

(In reply to Martin Stránský from comment #4)
> Well, the problem is that padding returned from Gtk3 is just huge. The Gtk2
> Firefox gets left/right padding 6 pixels, but Gtk3 one gets 12. That's 12
> extra pixels per button. Unfortunately we can't distinguish between buttons
> on bookmark bar and other XUL buttons. 
> 
> Possible solution may be to restrict maximal padding to some reasonable
> value (like the 6 pixels we have in Gtk2). Karl, what do you think?

Padding is applied between the button border outside rectangle and the contents of the button (icon, label).  The bookmark icons in the screenshots linked here don't have visible buttons, so I don't have any indication of how much button padding and inter-button spacing is involved.  DOM Inspector or similar can help here.

Just looking at evince here with Adwaita and gtk+ 3.14.9, which has visible buttons, there seems to be about 6px of padding in the toolbar buttons, perhaps 8 including the border.  There is also about 6 px between the borders of adjacent buttons.

In dialogs (e.g. print), there are wider buttons, but that seems like some kind of minimum button size rather than a padding around the labels.

I don't know why Firefox would be getting different values from evince, but I wonder whether they are doing something different?

Do evince and dialogs have visible buttons with your theme?
I wonder why the toolbar buttons are not visible?

I'm not keen on clamping padding of all widgets to 6 pixels.
We need to better understand what is going wrong.

	Martin Stránský [:stransky] (ni? me) Reporter
	Comment 9 • 9 years ago

It, looks like I was wrong about the size - it's even bigger:

https://git.gnome.org/browse/gtk+/tree/gtk/theme/Adwaita/_common.scss?h=3.16.0#n617

According to this code one would expect the left-right padding is 16 - and I get such values from moz_gtk_add_style_padding().

So the text button will have 17 extra pixels on both sides. Maybe the evince buttons have different attributes.

	Andrew Comminos [:acomminos]
	Comment 10 • 9 years ago

(In reply to Martin Stránský from comment #4)
> Unfortunately we can't distinguish between buttons
> on bookmark bar and other XUL buttons. 

NS_THEME_TOOLBAR_BUTTON appears to identify the bookmark bar buttons; a nicer solution might be to override the widget border for it in nsNativeThemeGTK::GetWidgetBorder.

	Martin Stránský [:stransky] (ni? me) Reporter
	Comment 11 • 9 years ago

(In reply to Andrew Comminos [:acomminos] from comment #10)
> NS_THEME_TOOLBAR_BUTTON appears to identify the bookmark bar buttons; a
> nicer solution might be to override the widget border for it in
> nsNativeThemeGTK::GetWidgetBorder.

That's interesting idea and works (I tested a simple hack) but we will need an extra simplified GTKbutton class in gtk3drawing.c assigned a small padding class from
https://git.gnome.org/browse/gtk+/tree/gtk/theme/Adwaita/_common.scss?h=3.16.0#n617

Karl, do you agree to add extra "small button" class to gtk3drawing.c?

	Karl Tomlinson (:karlt)
	Comment 12 • 9 years ago

(In reply to Martin Stránský from comment #11)
> (In reply to Andrew Comminos [:acomminos] from comment #10)
> > NS_THEME_TOOLBAR_BUTTON appears to identify the bookmark bar buttons; a
> > nicer solution might be to override the widget border for it in
> > nsNativeThemeGTK::GetWidgetBorder.
> 
> That's interesting idea and works (I tested a simple hack) but we will need
> an extra simplified GTKbutton class in gtk3drawing.c assigned a small
> padding class from
> https://git.gnome.org/browse/gtk+/tree/gtk/theme/Adwaita/_common.scss?h=3.16.
> 0#n617
> 
> Karl, do you agree to add extra "small button" class to gtk3drawing.c?

Thanks for the link.

Yes, we need to distinguish between buttons with labels and without.  GTK and Adwaita distinguish through either "text-button" or "image-button" passed to gtk_style_context_add_class().  I assume buttons without a label have an image.

NS_THEME_TOOLBAR_BUTTON and NS_THEME_BUTTON may be the appropriate distinction in nsThemeConstants.h.

	Petr Jašíček Assignee
	Comment 14 • 9 years ago

In this patch the toolbar buttons use new class, "image-button" - https://git.gnome.org/browse/gtk+/tree/gtk/theme/Adwaita/_common.scss?h=3.16.0#n615 which shrinks the left and right padding from 16px to 8px.

	Karl Tomlinson (:karlt)
	Comment 15 • 9 years ago

Comment on attachment 8643711 [details] [diff] [review]
001_gtk3_toolbar_padding.patch

>-  MOZ_GTK_WINDOW
>+  MOZ_GTK_WINDOW,
>+  /* Special toolbar button, see mozbz#1160154 */
>+  MOZ_GTK_TOOLBAR_BUTTON

Probably more helpful to insert this with the other buttons.  This is an
internal file, so no need for backward compat.

"Special" doesn't tell us much.  Indicate that this is a button with an image
and no text, and the bz reference is not required.

>+            if (widget == MOZ_GTK_TOOLBAR_BUTTON)
>+            {
>+                gtk_style_context_save(style);
>+                gtk_style_context_add_class(style, "image-button");
>+            }
>+
>             /* Don't add this padding in HTML, otherwise the buttons will
>                become too big and stuff the layout. */
>             if (!inhtml) {
>                 moz_gtk_add_style_padding(style, left, top, right, bottom);
>             }
> 
>+            if (widget == MOZ_GTK_TOOLBAR_BUTTON)
>+                gtk_style_context_restore(style);
>+
>             moz_gtk_add_style_border(style, left, top, right, bottom);

Might as well move the code inside the (!inhtml) block, because it
has no effect when (inhtml).

Either that or restore after moz_gtk_add_style_border and include similar
logic in moz_gtk_button_paint in case themes choose to make the border or
rendering depend on the "image-button" class.

>   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;
>     break;
>+    case NS_THEME_TOOLBAR_BUTTON:
>+      if (aWidgetFlags)
>+        *aWidgetFlags = GTK_RELIEF_NONE;
>+      aGtkWidgetType = MOZ_GTK_TOOLBAR_BUTTON;
>+      break;

I would guess that NS_THEME_TOOLBAR_DUAL_BUTTON should be rendered similarly
to NS_THEME_TOOLBAR_BUTTON.

	Petr Jašíček Assignee
	Comment 16 • 9 years ago

Thank you for the feedback, Karl.

I modified the patch as you suggested.

	Karl Tomlinson (:karlt)
	Comment 17 • 9 years ago

Comment on attachment 8644306 [details] [diff] [review]
001_gtk3_toolbar_padding.patch

Code looks good thanks.  Can you add author and commit message to the patch please?

See https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

	Petr Jašíček Assignee
	Comment 18 • 9 years ago

Updated, thanks.

	Martin Stránský [:stransky] (ni? me) Reporter
	Comment 19 • 9 years ago

Comment on attachment 8644843 [details] [diff] [review]
patch for check-in

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bf94f72b2ed

	Pulsebot
	Comment 20 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4fed4b50af5

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/b4fed4b50af5