Closed Bug 1160154 Opened 5 years ago Closed 4 years ago

[gtk3] Firefox: too much padding between icons in the personal toolbar

Categories

(Core :: Widget: Gtk, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 --- affected
firefox42 --- fixed

People

(Reporter: stransky, Assigned: pjasicek)

References

()

Details

Attachments

(1 file, 3 obsolete files)

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.
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
Which setup for the bookmark tab do you use? I looks like you have only icons there, no text.
Flags: needinfo?(andrew.g.dunn)
Do you use any bookmark extension?
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?
Flags: needinfo?(andrew.g.dunn) → needinfo?(karlt)
Attached patch max padding hack (obsolete) — Splinter Review
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 !
@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
(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.
Flags: needinfo?(karlt)
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.
(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.
(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?
Flags: needinfo?(karlt)
(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.
Flags: needinfo?(karlt)
Duplicate of this bug: 1187208
Attached patch 001_gtk3_toolbar_padding.patch (obsolete) — Splinter Review
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.
Attachment #8643711 - Flags: feedback?(karlt)
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.
Attachment #8643711 - Flags: feedback?(karlt) → feedback+
Attached patch 001_gtk3_toolbar_padding.patch (obsolete) — Splinter Review
Thank you for the feedback, Karl.

I modified the patch as you suggested.
Attachment #8643711 - Attachment is obsolete: true
Attachment #8644306 - Flags: review?(karlt)
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
Attachment #8644306 - Flags: review?(karlt) → review+
Attachment #8612804 - Attachment is obsolete: true
Updated, thanks.
Attachment #8644306 - Attachment is obsolete: true
Attachment #8644306 - Attachment is obsolete: false
Keywords: checkin-needed
Attachment #8644306 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/b4fed4b50af5
Assignee: nobody → pjasicek
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1210086
See Also: → 1210308
You need to log in before you can comment on or make changes to this bug.