Closed
Bug 406075
Opened 17 years ago
Closed 17 years ago
Firefox does not use native toolbar separator
Categories
(Core :: Widget: Gtk, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9beta2
People
(Reporter: ispence, Assigned: ispence)
References
Details
Attachments
(2 files, 4 obsolete files)
8.33 KB,
image/png
|
Details | |
12.08 KB,
patch
|
roc
:
review+
roc
:
superreview+
mtschrep
:
approval1.9+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•17 years ago
|
||
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.
Assignee | ||
Comment 2•17 years ago
|
||
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
Assignee | ||
Comment 4•17 years ago
|
||
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.
Comment 6•17 years ago
|
||
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.)
Assignee | ||
Comment 7•17 years ago
|
||
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
Assignee | ||
Comment 8•17 years ago
|
||
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.
Assignee | ||
Comment 9•17 years ago
|
||
This should handle the centering of the separator
Attachment #290915 -
Attachment is obsolete: true
Updated•17 years ago
|
Assignee: nobody → ispence
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•17 years ago
|
Attachment #290944 -
Flags: superreview?(roc)
Attachment #290944 -
Flags: review?(roc)
Updated•17 years ago
|
Status: NEW → ASSIGNED
Summary: Firefox does not use native toolbar separator. → Firefox does not use native toolbar separator
Version: unspecified → Trunk
You didn't address comment #5?
Assignee | ||
Comment 11•17 years ago
|
||
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?
Assignee | ||
Comment 13•17 years ago
|
||
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.
Assignee | ||
Comment 15•17 years ago
|
||
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.
Assignee | ||
Comment 17•17 years ago
|
||
OK, I was thinking of the wrong piece of code. I should have a patch soonish.
Assignee | ||
Comment 18•17 years ago
|
||
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 20•17 years ago
|
||
Comment on attachment 291142 [details] [diff] [review]
Native Toolbar Separators Version 5
Use a native toolbar separator on Linux.
Attachment #291142 -
Flags: approval1.9?
Updated•17 years ago
|
Attachment #291142 -
Flags: approval1.9? → approval1.9+
Updated•17 years ago
|
Keywords: checkin-needed
Comment 21•17 years ago
|
||
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: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10
You need to log in
before you can comment on or make changes to this bug.
Description
•