Closed Bug 406075 Opened 13 years ago Closed 13 years ago

Firefox does not use native toolbar separator

Categories

(Core :: Widget: Gtk, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: ispence, Assigned: ispence)

References

Details

Attachments

(2 files, 4 obsolete files)

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
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.
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.
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
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)
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.
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.)
Attached patch More closely emulated GTK+ (obsolete) — Splinter Review
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.
Attachment #290758 - Attachment is obsolete: true
Attachment #290778 - Attachment is obsolete: true
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.
This should handle the centering of the separator
Attachment #290915 - Attachment is obsolete: true
Assignee: nobody → ispence
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #290944 - Flags: superreview?(roc)
Attachment #290944 - Flags: review?(roc)
Status: NEW → ASSIGNED
Summary: Firefox does not use native toolbar separator. → Firefox does not use native toolbar separator
Version: unspecified → Trunk
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
Are you going to post a new patch?
That was used in attachment 290944 [details] [diff] [review]
+  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.
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.
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.
OK, I was thinking of the wrong piece of code.  I should have a patch soonish.
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)
Attachment #290944 - Attachment is obsolete: true
Attachment #291142 - Flags: superreview?(roc)
Attachment #291142 - Flags: review?(roc)
Attachment #290944 - Flags: superreview?(roc)
Attachment #290944 - Flags: review?(roc)
Comment on attachment 291142 [details] [diff] [review]
Native Toolbar Separators Version 5

this is just right, thanks!!!
Attachment #291142 - Flags: superreview?(roc)
Attachment #291142 - Flags: superreview+
Attachment #291142 - Flags: review?(roc)
Attachment #291142 - Flags: review+
Comment on attachment 291142 [details] [diff] [review]
Native Toolbar Separators Version 5

Use a native toolbar separator on Linux.
Attachment #291142 - Flags: approval1.9?
Attachment #291142 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10
Depends on: 411032
Depends on: 1294136
You need to log in before you can comment on or make changes to this bug.