Clean up places library code

RESOLVED FIXED in Firefox 7

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Robert Kaiser, Assigned: Robert Kaiser)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

Trunk
Firefox 7
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

7 years ago
Created attachment 467026 [details] [diff] [review]
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.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #467026 - Flags: review?(mak77)

Comment 2

7 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.
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.
other changes looks fine, so you could split patch leaving organizer scope bar styling (including small button) alone.
(Assignee)

Comment 5

7 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.
so, file a new bug to correctly make the scope styling stuff native on tier platforms
Comment on attachment 467026 [details] [diff] [review]
v1: apply cleanups brought up in bug 585601

forgot to set the flag
Attachment #467026 - Flags: review?(mak77) → review-
(Assignee)

Comment 8

7 years ago
Created attachment 471594 [details] [diff] [review]
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.
Attachment #467026 - Attachment is obsolete: true
Attachment #471594 - Flags: review?(mak77)
why toolbarbuttons in the scopebar toolbar have to become buttons?
(Assignee)

Comment 10

7 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?
Ordinary buttons blend in better with communicator themes (Modern, winstripe, gnomestripe) which conveniently meant we could get rid of a bunch of theme CSS.
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

7 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).
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.
Attachment #471594 - Flags: feedback?(mano)
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.
Attachment #471594 - Flags: review?(mano)
Attachment #471594 - Flags: review?(mak77)
Attachment #471594 - Flags: feedback?(mano)
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.
Attachment #471594 - Flags: review-
(Assignee)

Comment 17

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

Comment 18

7 years ago
Created attachment 494581 [details] [diff] [review]
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).
Attachment #471594 - Attachment is obsolete: true
Attachment #494581 - Flags: review?(mano)
Attachment #471594 - Flags: review?(mano)
(Assignee)

Comment 19

6 years ago
Created attachment 534222 [details] [diff] [review]
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...
Attachment #494581 - Attachment is obsolete: true
Attachment #534222 - Flags: review?(mano)
Attachment #494581 - Flags: review?(mano)

Updated

6 years ago
Attachment #534222 - Flags: review?(mano) → review?(mak77)
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 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 :)
Attachment #534222 - Flags: review?(mak77) → review+
http://hg.mozilla.org/mozilla-central/rev/87ce91536b59
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7

Updated

6 years ago
Depends on: 677417
This is probably worth documenting. It breaks a couple of extensions and maybe some themes as well. See bug 677417.
Keywords: dev-doc-needed
Added a note to
https://developer.mozilla.org/en/Firefox_7_for_developers#Other_Changes
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.