588027 - Clean up places library code

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Robert Kaiser]

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.

Comment 1

[Robert Kaiser]

Attachment: v1: apply cleanups brought up in bug 585601

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.

Comment 2

[Stefan [:stefanh]]

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.

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Marco Bonardo [:mak]]

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

Comment 5

[Robert Kaiser]

(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.

Comment 6

[Marco Bonardo [:mak]]

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

Comment 7

[Marco Bonardo [:mak]]

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

forgot to set the flag

Comment 8

[Robert Kaiser]

Attachment: v1.1: revert Mac and Windows scope bar theming

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.

Comment 9

[Marco Bonardo [:mak]]

why toolbarbuttons in the scopebar toolbar have to become buttons?

Comment 10

[Robert Kaiser]

(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?

Comment 11

[[e-mail]]

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

Comment 12

[Marco Bonardo [:mak]]

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.

Comment 13

[Stefan [:stefanh]]

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).

Comment 14

[Marco Bonardo [:mak]]

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.

Comment 15

[Marco Bonardo [:mak]]

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.

Comment 16

[Dão Gottwald [:dao]]

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.

Comment 17

[Robert Kaiser]

(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.

Comment 18

[Robert Kaiser]

Attachment: v1.2: leave toolbarbutton

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).

Comment 19

[Robert Kaiser]

Attachment: v1.3: unbitrotted

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...

Comment 20

[Marco Bonardo [:mak]]

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.

Comment 21

[Marco Bonardo [:mak]]

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 :)

Comment 22

[Dão Gottwald [:dao]]

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

Comment 23

[Jorge Villalobos [:jorgev] (he/him)]

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

Comment 24

[Florian Scholz (Open Web Docs)]

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