Last Comment Bug 583510 - Toolbar separators are invisible
: Toolbar separators are invisible
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: Firefox 11
Assigned To: Markus Amalthea Magnuson
:
Mentors:
Depends on: 726132
Blocks: 544821 559033
  Show dependency treegraph
 
Reported: 2010-07-31 13:37 PDT by RobertJ
Modified: 2012-02-18 09:11 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Show toolbar separators correctly on Mac. (695 bytes, patch)
2011-11-16 04:58 PST, Markus Amalthea Magnuson
dao+bmo: review-
mstange: review+
Details | Diff | Splinter Review
Show toolbar separators correctly on Mac. (761 bytes, patch)
2011-11-26 12:01 PST, Markus Amalthea Magnuson
dao+bmo: review+
Details | Diff | Splinter Review
Show toolbar separators correctly on Mac. (908 bytes, patch)
2011-11-26 12:26 PST, Markus Amalthea Magnuson
dao+bmo: review+
Details | Diff | Splinter Review

Description RobertJ 2010-07-31 13:37:11 PDT
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b2) Gecko/20100720 Firefox/4.0b2
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b2) Gecko/20100720 Firefox/4.0b2

The toolbar separators on Mac are invisible on the toolbar. I also tried redefining them as a solid wide line in the userchrome.css since the dotted separator is a little hard to see on Mac. It was changed in the customize window to a solid wide line; however, when I dragged and dropped on the toolbar it disappeared.

Reproducible: Always

Actual Results:  
See description.

Expected Results:  
See the separator on the toolbar.

Using default theme.
Comment 1 RobertJ 2010-08-11 15:09:11 PDT
This is still broken in FF4 beta3
Comment 2 RobertJ 2010-08-24 13:37:01 PDT
This is still broken in FF4 beta4
Comment 3 RobertJ 2010-09-08 07:52:45 PDT
This is still broken in FF4 beta5
Comment 4 Jean-Yves Perrier [:teoli] 2010-09-09 10:09:12 PDT
I see the same with the latest trunk on Mac OS X 10.5.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b6pre) Gecko/20100909 Firefox/4.0b6pre
Comment 5 RobertJ 2010-09-15 09:43:04 PDT
This is still broken in FF4 beta6
Comment 6 RobertJ 2010-11-11 08:01:31 PST
This is still broken in FF4 beta7
Comment 7 RobertJ 2010-12-21 08:56:55 PST
This is still broken in FF4 beta8
Comment 8 Dão Gottwald [:dao] 2010-12-21 09:05:55 PST
These updates aren't really useful.
What would be more useful is a regression range, assuming this is a regression.
Comment 9 RobertJ 2010-12-21 09:25:49 PST
Sorry if I broke protocol. 

All I know is I first saw this on FF4b2 on OSX6. I don't test nightly builds so anything prior to FF4b2 is an unknown for me. As you can see from comment 4, teoli2003 confirmed it on 4.0b6pre on OSX5.

This is not and has never been a bug on 3.6.x Other than that I have nothing else to add.
Comment 10 Dão Gottwald [:dao] 2010-12-21 09:33:26 PST
This is probably a regression from bug 559033.
Comment 11 Markus Stange [:mstange] 2010-12-21 09:37:33 PST
Hmm, this is a very convincing argument for changing the box align back to stretch.
Comment 12 RobertJ 2011-01-02 12:51:01 PST
As an experiment I installed this theme nuvola-1.9.7 on FF4b8 on OSX 10.6.5. 

The toolbar separators work just fine. I may poke around the omni.jar to see if I can find it. Will start by searching for "box align"
Comment 13 Markus Stange [:mstange] 2011-01-02 13:55:36 PST
You don't really have to do anything here, except if you want to provide a patch.

The problem is in pinstripe's browser.css. We'll either need to give toolbar separators a min-height, or we'll have to revert the toolbars to -moz-box-align: stretch. The rationale for -moz-box-align: center is in bug 559033 starting at bug 559033 comment 11.
Comment 14 RobertJ 2011-01-03 06:57:23 PST
I added this to my userChrome.css. It solved the problem for me.

toolbarseparator {
  min-height: 17px !important;
}

Note that I checked this out for both the "normal" OSX dotted separator which I find hard to see and a beveled one which is easier to see that I add through the userChrome.css and they seem to work just fine.

My beveled userChrome.css

toolbarseparator {
  margin : 6px  !important;
  border-right : 1px solid #FFFFFF !important;
  border-left : 1px solid #808080 !important;
  width : 2px !important;
  opacity: 1.0 !important;
}
Comment 15 RobertJ 2011-01-03 13:42:05 PST
After playing around a bit I found this combination works best

toolbarseparator {
min-height: 20px !important;
margin: 0px 6px  !important;
<other properties>
}

The separator is vertically the same size as the buttons and the toolbar is not enlarged.
Comment 16 Markus Amalthea Magnuson 2011-11-16 04:58:26 PST
Created attachment 574876 [details] [diff] [review]
Show toolbar separators correctly on Mac.

Here's a patch that fixes the separators on Mac.

I've set a default min-height of 22px that will be used for text-only and icon-only toolbars, and another min-height of 36px for icon+text toolbars.
Comment 17 Markus Stange [:mstange] 2011-11-16 05:44:56 PST
Comment on attachment 574876 [details] [diff] [review]
Show toolbar separators correctly on Mac.

Sounds good to me, let's see what Dão thinks.
Comment 18 Dão Gottwald [:dao] 2011-11-26 04:38:38 PST
Comment on attachment 574876 [details] [diff] [review]
Show toolbar separators correctly on Mac.

I think I'd prefer this to be in browser.css, since this is where we (unfortunately) set -moz-box-align: center.
Comment 19 Markus Amalthea Magnuson 2011-11-26 12:01:16 PST
Created attachment 577092 [details] [diff] [review]
Show toolbar separators correctly on Mac.

Here's the same change but applied in browser.css instead.
Comment 20 Dão Gottwald [:dao] 2011-11-26 12:05:51 PST
Comment on attachment 577092 [details] [diff] [review]
Show toolbar separators correctly on Mac.

Can you add a comment, saying that this is needed because of "-moz-box-align: center" a couple of lines above?

Thanks!
Comment 21 Markus Amalthea Magnuson 2011-11-26 12:26:31 PST
Created attachment 577093 [details] [diff] [review]
Show toolbar separators correctly on Mac.

Same patch as the last one but with the requested comment added. Thanks for the feedback!
Comment 22 Markus Amalthea Magnuson 2011-11-26 17:03:43 PST
What additional reviews do I need for this?
Comment 23 Dão Gottwald [:dao] 2011-11-27 02:21:34 PST
(In reply to Markus Amalthea Magnuson from comment #22)
> What additional reviews do I need for this?

none
Comment 25 Marco Bonardo [::mak] 2011-11-28 05:25:39 PST
https://hg.mozilla.org/mozilla-central/rev/58394a391fd2

Note You need to log in before you can comment on or make changes to this bug.