When places ui landed, css for win/nix was added. However, the files in classic/mac was not updated.

For the record, this has the bad

(In reply to comment #1) > For the record, this has the bad effect of the "Most visited" icon disappearing when you click it in the PT

I was actually wrong in comment #2, this is of course not because we lack css: suite/themes/classic/mac/communicator/bookmarks/bookmarks.css /* query-nodes should be styled even if they're not expandable */ .bookmark-item[container][query], treechildren::-moz-tree-image(title, query) { list-style-image: url("chrome://communicator/skin/bookmarks/query.png"); -moz-image-region: auto; } As you can see from the screenshot, the icon doesn't fit, so instead of creating a hover:active version I think the best thing to do is to not use it in the PT for now. All that needs to be done then is to remove the '.bookmark-item[container][query]'. I could probably create a new query icon that blends in with the existing ones.

To explain further what I mean with the missing css - it's the sidebar and certain elements in the main window that lacks some styling.

(In reply to comment #0) > However, the files in > classic/mac was not updated. For the record, this is not true, as http://hg.mozilla.org/comm-central/rev/05e7e38f82ea indeed did land the same changes for mac than it did for the normal default theme files. This also means that anything else is off my hands, as I can't test on a mac and so have no idea what else than on the normal platforms we need on Mac. We once again need someone on a Mac to go the last mile here, apparently.

(In reply to comment #5) > (In reply to comment #0) > > However, the files in > > classic/mac was not updated. > > For the record, this is not true, as > http://hg.mozilla.org/comm-central/rev/05e7e38f82ea indeed did land the same > changes for mac than it did for the normal default theme files. Yes, I was wrong, there was indeed an update of the mac css files - this was what I ment in comment #3. Some bits are missing, though (see comment #4)- but that's no big deal, no-one's expecting someone to theme an unknown platform. > This also means that anything else is off my hands, as I can't test on a mac > and so have no idea what else than on the normal platforms we need on Mac. Yeah, sure I agree except for the icon problem. See the last bit of comment #3 where I have told what can be done to temporary avoid the issue you see in the screenshot in attachment #464569 [details] (until someone fixes new icons).

Comment on attachment 464875 [details] [diff] [review] Fix the most obvious thing for a3 (pushed for a3) As long as you put it back in later with a more comprehensive fix, go for it.

Comment on attachment 464875 [details] [diff] [review] Fix the most obvious thing for a3 (pushed for a3) I pushed this as http://hg.mozilla.org/comm-central/rev/3caedf4e73cf

I now understand why the text-shadow rules on hover:active on the PT doesn't work: The xul structure have changed.

Just putting up a wip patch here, so I don't lose it.

Almost done, I need to double-check some things first, though. For the record, these are the icon changes compared to the existing ones: New icons: bookmarksMenu.png, bookmarksToolbar.png Ported icons: expander-closed.png, expander-closed-active.png, expander-open.png, expander-open-active.png and query.png. Changed icons: query-active.png (darkened query.png). The feed icons needs some changes as well and we should probably have a better "All Bookmarks" icon, but that can wait.

KaiRo, I have a question regarding the changes you landed recently: The detailsDeck-splitter (added in bug 585601), during what circumstances is it enabled?

I had to do some changes because of rev 64ad57c8da27

(In reply to comment #14) > I have a question regarding the changes you landed recently: The > detailsDeck-splitter (added in bug 585601), during what circumstances is it > enabled? It's always there, but never in an enabled state, because you can't resize the detailsDeck, you can only collapse it - that's why the splitter has a grippy (and that one works). Not sure about the design of the grippies on Mac, though...

(In reply to comment #16) > (In reply to comment #14) > > I have a question regarding the changes you landed recently: The > > detailsDeck-splitter (added in bug 585601), during what circumstances is it > > enabled? > > It's always there, but never in an enabled state, because you can't resize the > detailsDeck, you can only collapse it - that's why the splitter has a grippy > (and that one works). Not sure about the design of the grippies on Mac, > though... Ah, ok - got it. Yeah, no grippies in Mac classic. We'll see what we can do about this. I'm a bit surprised that you can have a parent node disabled and it's children enabled, though.

Almost there... Added some livemark icons (1 ported, 2 new). I'll file a new bug regarding the grippie, since I have no idea atm on how to solve that. Looks kind of funny, since the default mac splitter is shown and there's no indication on that it's disabled. What I will do is that I will hide the splitter until we know how to handle it.

Filed bug 591779 for the splitter/grippy issue.

For the scope bar, the switch to buttons created some problems (the buttons should not look like pushbuttons), but I managed to re-style the buttons so they look the same as before (I used "button[group="scopeBar"]" instead of id's since I'm worried that someone will add a new button without knowing that this will regress mac). As for the new splitter with its grippy, see bug 591779. I might re-visit the icons and fine-tune the color scheme a bit later on (there is for instance some mis-match in color between the unsorted bookmarks icon and the bookmarksToolbar button), but I think this looks pretty good now.

(In reply to comment #20) > For the scope bar, the switch to buttons created some problems (the buttons > should not look like pushbuttons) You mean button[type="radio"] doesn't do the right thing on Mac? [Although I'm not sure that we wouldn't be better off with a radiogroup.]

(In reply to comment #21) > (In reply to comment #20) > > For the scope bar, the switch to buttons created some problems (the buttons > > should not look like pushbuttons) > You mean button[type="radio"] doesn't do the right thing on Mac? The scope bar buttons are sort of unique.

Btw, I forgot to remove the gAMA, cHRM, iCCP and sRGB chunks - I'll do that once the reviews are done.

Comment on attachment 470296 [details] [diff] [review] Final patch Neil thought there wasn't any point in him looking at this (he hasn't reviewed much of the places code), so I'm cancelling the request. I'll eventually let IanN take a look.

I also noted that I probably need a html namespace in one of the files

Comment on attachment 470296 [details] [diff] [review] Final patch Asking IanN for feedback since Neil passed the sr.

This one includes the missing namespace. I also added the default xul one, not sure if that is needed, though.

(In reply to comment #23) > Btw, I forgot to remove the gAMA, cHRM, iCCP and sRGB chunks - I'll do that > once the reviews are done. Filed bug 593840 for that - better do all at once.

Comment on attachment 472424 [details] [diff] [review] New version with namespace declarations +/* We just want this when they're in the PT */ FYI, this comment is misleading since we want this if they're in the main toolbar as well

Comment on attachment 472424 [details] [diff] [review] New version with namespace declarations Looks good, just some questions: >- width: 0px; >- height: 0px; >+ width: 0; >+ height: 0; Any particular reason for that? >+ border-right: 1px solid #d7dad7; I suppose there isn't a predefined native color for that? >+ border: solid #B3B3B3; And you maybe want to use a common casing style. :-P r=me either way.

(In reply to comment #30) > Comment on attachment 472424 [details] [diff] [review] > New version with namespace declarations > > Looks good, just some questions: > > >- width: 0px; > >- height: 0px; > >+ width: 0; > >+ height: 0; > > Any particular reason for that? No, not more than that the other files in the patch uses '0'. I can actually revert those 2 changes in bookmarks.css to decrease the blame. > > >+ border-right: 1px solid #d7dad7; > > I suppose there isn't a predefined native color for that? No, sorry. This is btw the same colour used in mailNews/Addressbook. > > >+ border: solid #B3B3B3; > > And you maybe want to use a common casing style. :-P Ah, thanks - I must have missed those, I aimed at using uppercase for everything. Will go through the colours again! > > r=me either way.

(In reply to comment #31) > > >+ border-right: 1px solid #d7dad7; > > > > I suppose there isn't a predefined native color for that? > > No, sorry. This is btw the same colour used in mailNews/Addressbook. Ignore the last sentense since I didn't looked at the selector and confused it with the splitter.

Comment on attachment 472424 [details] [diff] [review] New version with namespace declarations Seems fine apart from the unneeded 0px to 0 changes. (Code inspection only)

I pushed attachment #472424 [details] [diff] [review] with the following adjustments: - removed comment (comment #29) - some whitespace fixes - reverted the 0px->0 changes in bookmarks.css - fixed casing (all colours should be upper-cased) http://hg.mozilla.org/comm-central/rev/fd843ab226f5