Robert Kaiser Assignee
	Description • 14 years ago

Neil brought up some good possibilities for cleaning up the places library code when commenting on the SeaMonkey version in bug 585601. I have worked up a patch for making all those cleanups on the Firefox side, but this probably needs some good testing and UI-review on the various platforms supported with Firefox default themes.

	Robert Kaiser Assignee
	Comment 1 • 14 years ago

This patch applies the cleanups from bug 585601 that seem fitting for the places library as well.
All in all, this should be removing some historic cruft (spurious additional boxes in the markup), and also make the search scope bar fit in better with OS native theming.

Marco, please mark the needed ui-reviews and such, I'm not 100% sure what we need there.

	Stefan [:stefanh]
	Comment 2 • 14 years ago

At least on mac, the toolbarbuttons in the scope bar are styled as "Scope bar buttons" (see the css you remove), so if you want to switch to regular push buttons, you'll need to re-style them so they fit in the scope bar.

	Marco Bonardo [:mak]
	Comment 3 • 14 years ago

Regarding the scope bar, I don't like your new styling on Windows 7, and I guess Mac is bad too based on comment 2. Id you drop current styling, then you'll have to figure out a similar scope bar ui in the various OSes and restyle them accordingly.

	Marco Bonardo [:mak]
	Comment 4 • 14 years ago

other changes looks fine, so you could split patch leaving organizer scope bar styling (including small button) alone.

	Robert Kaiser Assignee
	Comment 5 • 14 years ago

(In reply to comment #4)
> other changes looks fine, so you could split patch leaving organizer scope bar
> styling (including small button) alone.

Probably not soon, and the scope bar stuff probably dies with your comments here, as I have no possibility to do any styling for anything but GTK2 with a very skewed theme I have running for that GNOME crap here.

	Marco Bonardo [:mak]
	Comment 6 • 14 years ago

so, file a new bug to correctly make the scope styling stuff native on tier platforms

	Marco Bonardo [:mak]
	Comment 7 • 14 years ago

Comment on attachment 467026 [details] [diff] [review]
v1: apply cleanups brought up in bug 585601

forgot to set the flag

	Robert Kaiser Assignee
	Comment 8 • 14 years ago

OK, here's a patch that should revert Mac and Windows scope bar theming to what it was before, and still apply all the other cleanup, including that on the scope bar itself.

	Marco Bonardo [:mak]
	Comment 9 • 14 years ago

why toolbarbuttons in the scopebar toolbar have to become buttons?

	Robert Kaiser Assignee
	Comment 10 • 14 years ago

(In reply to comment #9)
> why toolbarbuttons in the scopebar toolbar have to become buttons?

Neil, that was your proposal, why exactly did you want that?

	neil@parkwaycc.co.uk
	Comment 11 • 14 years ago

Ordinary buttons blend in better with communicator themes (Modern, winstripe, gnomestripe) which conveniently meant we could get rid of a bunch of theme CSS.

	Marco Bonardo [:mak]
	Comment 12 • 14 years ago

hm, but toolbarbuttons are buttons in toolbars, and these are buttons in a toolbar. So, i'm not sure I understand the need for the change, if the theme is wrongly managing toolbarbuttons changing them to simple buttons does not look like a solution.

	Stefan [:stefanh]
	Comment 13 • 14 years ago

For pinstripe, see the css in my patch in bug 586026 - that should make the scope bar buttons look the same as before (just switching from toolbarbutton to button in the css file wasn't enough iirc).

	Marco Bonardo [:mak]
	Comment 14 • 14 years ago

Comment on attachment 471594 [details] [diff] [review]
v1.1: revert Mac and Windows scope bar theming

Mano, what do you think of the toolbabutton->button change in the scope toolbar? the rest of the patch looks fine to me.

	Marco Bonardo [:mak]
	Comment 15 • 14 years ago

Comment on attachment 471594 [details] [diff] [review]
v1.1: revert Mac and Windows scope bar theming

I'm directly moving review to mano, I have an hard time evaluating this subtle differences in xul widgets, so I prefer moving to his experience.
Apart the toolbarbutton->button change the rest of the patch is fine for me.

	Dão Gottwald [:dao]
	Comment 16 • 14 years ago

Comment on attachment 471594 [details] [diff] [review]
v1.1: revert Mac and Windows scope bar theming

>-#organizerScopeBar > toolbarbutton > .toolbarbutton-icon {
>+#organizerScopeBar > button > .toolbarbutton-icon {
>   padding: 0;
>   margin: 0;
> }
> 
>-#organizerScopeBar > toolbarbutton > .toolbarbutton-text {
>+#organizerScopeBar > button > .toolbarbutton-text {
>    margin: 0;
>    padding: 2px 5px;
> }

This doesn't work.

I'd suggest avoiding the toolbarbutton->button change. I don't think we want this for Firefox.

	Robert Kaiser Assignee
	Comment 17 • 14 years ago

(In reply to comment #16)
> I'd suggest avoiding the toolbarbutton->button change. I don't think we want
> this for Firefox.

In that case, we'll need a good part of CSS added for gnomestripe, where IMHO we want them to look like a <button> actually.

	Robert Kaiser Assignee
	Comment 18 • 14 years ago

OK, here's a patch that just leaves those buttons as toolbarbuttons. I decided I don't care if gnomestripe would look more fitting with actual buttons or not, but I really care about Firefox getting this cleanup (though I start to wonder if it would even be allowed for FF4 still - if not, it's still good to have after branching).

	Robert Kaiser Assignee
	Comment 19 • 13 years ago

Here's an unbitrotted version. Can anyone else than mano review this or was it forgotten? I'd love to get this off my local queue and into the tree...

	Marco Bonardo [:mak]
	Comment 20 • 13 years ago

I should have gone back to this after our talk at the all-hands, but looks like I forgot, sorry.  Will look at it soon.

	Marco Bonardo [:mak]
	Comment 21 • 13 years ago

Comment on attachment 534222 [details] [diff] [review]
v1.3: unbitrotted

Review of attachment 534222 [details] [diff] [review]:
-----------------------------------------------------------------

I tested on Win7 and WinXP, results seem consistent, the scope bar save button is larger but that's not a big deal (all styling of that bar is fancy, this only makes it less fancy).
Menus and trees are fine afaict, so let's proceed while we are early in the cycle :)

	Dão Gottwald [:dao]
	Comment 22 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/87ce91536b59

	Jorge Villalobos [:jorgev] (he/him)
	Comment 23 • 13 years ago

This is probably worth documenting. It breaks a couple of extensions and maybe some themes as well. See bug 677417.

	Florian Scholz (Open Web Docs)
	Comment 24 • 12 years ago

Added a note to
https://developer.mozilla.org/en/Firefox_7_for_developers#Other_Changes