Firefox does not use native toolbar separator

RESOLVED FIXED in mozilla1.9beta2

Status

()

Core
Widget: Gtk
RESOLVED FIXED
10 years ago
2 years ago

People

(Reporter: Ian Spence, Assigned: Ian Spence)

Tracking

Trunk
mozilla1.9beta2
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

10 years ago
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

10 years ago
Created attachment 290757 [details]
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.
(Assignee)

Comment 2

10 years ago
Created attachment 290758 [details] [diff] [review]
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.
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

10 years ago
Created attachment 290778 [details] [diff] [review]
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)
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

10 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

10 years ago
Created attachment 290915 [details] [diff] [review]
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.
Attachment #290758 - Attachment is obsolete: true
Attachment #290778 - Attachment is obsolete: true
(Assignee)

Comment 8

10 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

10 years ago
Created attachment 290944 [details] [diff] [review]
Fixes the centering of the separator

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
(Assignee)

Comment 11

10 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

10 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

10 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

10 years ago
OK, I was thinking of the wrong piece of code.  I should have a patch soonish.
(Assignee)

Comment 18

10 years ago
Created attachment 291142 [details] [diff] [review]
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)
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?

Updated

10 years ago
Attachment #291142 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
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
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10

Updated

2 years ago
Depends on: 1294136
You need to log in before you can comment on or make changes to this bug.