406075 - Firefox does not use native toolbar separator

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Ian Spence]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007112916 Firefox/3.0b2pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007112916 Firefox/3.0b2pre

Firefox does not use the native toolbar separator. It instead creates a box with borders on the left and right, simulating a toolbar separator.

Reproducible: Always

Steps to Reproduce:
1. Add a separator to the toolbar
Actual Results:  
Firefox uses a comparatively large toolbar separator that does not use the correct colors.

Expected Results:  
Firefox uses native toolbar separator

Comment 1

[Ian Spence]

Attachment: Shows 3 different toolbars

The top bar is how the toolbar separator currently appears in Firefox.
The bottom bar is another gtk app, showing the correct separator.
The middle bar is how the toolbar separator appears when using the soon to be attached patch.

Comment 2

[Ian Spence]

Attachment: Patch to support native GTK toolbar separators

Adds support for native GTK toolbar separators.
This is my first bug so I'm not sure if the formatting of the code is correct. This code works with GTK+ 2.10 and higher.

Comment 3

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

Thanks for the patch!

What happens if separator_width is greater than 2px? Looks like this code will just draw outside the frame. So there are a couple of options:
-- make sure that nsNativeThemeGTK::GetMinimumWidgetSize returns the true width
-- limit separator_width to no more than widget's rectangle width

Comment 4

[Ian Spence]

Attachment: Updated patch that I believe addresses the comments

I'm not exactly sure if this is what you meant. I'm not 100% sure if I handled the case where wide separators are not used, since in that case, separator_width is not used at all.

This patch does fix a really stupid typo (was actually looking up the height instead of the width)

Comment 5

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

You probably want PR_MIN(style->xthickness, rect->width) for the vline call. Otherwise it looks good to me. Michael Ventnor had some comments, I'll let him post them here.

Comment 6

[Benjamin Berg]

Just thought to point this out. There is one difference to what GTK+ does though. GTK passes the GtkSeparatorToolItem to the engine and its style, and not the toolbar. (This shouldn't create many problems though, I expect.)

Comment 7

[Ian Spence]

Attachment: More closely emulated GTK+

Updated to address the comments I've received.  The only issue I see is a carry over from the old way of doing these. For some reason, the separator is closer to the toolitem to its left than the toolitem to its right. I believe this has to do with the css that spaces toolitems, although I'm not sure.

Comment 8

[Ian Spence]

This is probably a separate issue, but once this is applied, the separators will have a height of 60% of its parent, while the resizer between the url bar and the search bar will have roughly 100% of its parent's height, making it look large.  It'd probably be best to give it a top and bottom margin of 20% so that it matches.

Comment 9

[Ian Spence]

Attachment: Fixes the centering of the separator

This should handle the centering of the separator

Comment 10

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

You didn't address comment #5?

Comment 11

[Ian Spence]

Using

+    gtk_widget_style_get(gToolbarWidget,
+                         "space-size", size,
+                         NULL);

Instead of 

+    gtk_widget_style_get(gToolbarWidget,
+                         "wide-separators",  &wide_separators,
+                         "separator-width", &separator_width,
+                         NULL);  

handles both cases from the code I've read

Comment 12

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

Are you going to post a new patch?

Comment 13

[Ian Spence]

That was used in attachment 290944 [details] [diff] [review]

Comment 14

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

+  case NS_THEME_TOOLBAR_SEPARATOR:
+    gint separator_width;
+    
+    moz_gtk_get_toolbar_separator_width(&separator_width);
+    
+    aResult->width = separator_width;
+    break;

Need {} here to scope the declaration of separator_width properly.

I'd still like the PR_MIN for the xthickness just in case the frame is unexpectedly small or something.

Comment 15

[Ian Spence]

Before I attach a patch, I want to make sure I know exactly what you are asking.

How I handle it in the new patch:

width = style->space_size;
if (wide)
    width = MAX(separator_width, width);
else
    width = MAX(xthickness, width);

This way the box is always at least as wide as 2px, but will expand to the expected paint size.

The reason I use MAX instead of MIN for xthickness is that space_size is what gtk gives me that includes the padding on each side, which is likely greater than (is never less than) xthickness.

Comment 16

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

Right, so that's how you should compute the width. But when *painting* you should treat the input rectangle width as an upper bound on what you should paint.

Comment 17

[Ian Spence]

OK, I was thinking of the wrong piece of code.  I should have a patch soonish.

Comment 18

[Ian Spence]

Attachment: Native Toolbar Separators Version 5

Is more careful when decided on the bounds to begin with, and makes sure that we never paint anything larger than the box.

(I'm not sure if I messed up the request review thing, so if I did, sorry)

Comment 19

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

Comment on attachment 291142 [details] [diff] [review]
Native Toolbar Separators Version 5

this is just right, thanks!!!

Comment 20

[Reed Loden [:reed]]

Comment on attachment 291142 [details] [diff] [review]
Native Toolbar Separators Version 5

Use a native toolbar separator on Linux.

Comment 21

[Reed Loden [:reed]]

Checking in toolkit/themes/gnomestripe/global/toolbar.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/toolbar.css,v  <--  toolbar.css
new revision: 1.11; previous revision: 1.10
done
Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.44; previous revision: 1.43
done
Checking in widget/src/gtk2/gtkdrawing.h;
/cvsroot/mozilla/widget/src/gtk2/gtkdrawing.h,v  <--  gtkdrawing.h
new revision: 1.38; previous revision: 1.37
done
Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.115; previous revision: 1.114
done