586026 - New places ui lacks mac css and certain icons doesn't blend in with existing

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Stefan [:stefanh]]

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

Comment 1

[Stefan [:stefanh]]

For the record, this has the bad

Comment 2

[Stefan [:stefanh]]

(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

Comment 3

[Stefan [:stefanh]]

Attachment: screenshot, normal and hover:active state

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.

Comment 4

[Stefan [:stefanh]]

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.

Comment 5

[Robert Kaiser]

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

Comment 6

[Stefan [:stefanh]]

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

[Stefan [:stefanh]]

Attachment: Fix the most obvious thing for a3 (pushed for a3)

Comment 8

[Robert Kaiser]

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 9

[Stefan [:stefanh]]

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

Comment 10

[Stefan [:stefanh]]

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

Comment 11

[Stefan [:stefanh]]

Attachment: WIP

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

Comment 12

[Stefan [:stefanh]]

Attachment: Another WIP

Comment 13

[Stefan [:stefanh]]

Attachment: Next WIP

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.

Comment 14

[Stefan [:stefanh]]

KaiRo,

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

Comment 15

[Stefan [:stefanh]]

Attachment: New WIP

I had to do some changes because of rev 64ad57c8da27

Comment 16

[Robert Kaiser]

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

Comment 17

[Stefan [:stefanh]]

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

Comment 18

[Stefan [:stefanh]]

Attachment: Another WIP

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.

Comment 19

[Stefan [:stefanh]]

Filed bug 591779 for the splitter/grippy issue.

Comment 20

[Stefan [:stefanh]]

Attachment: Final patch

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.

Comment 21

[[e-mail]]

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

Comment 22

[Stefan [:stefanh]]

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

Comment 23

[Stefan [:stefanh]]

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

Comment 24

[Stefan [:stefanh]]

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.

Comment 25

[Stefan [:stefanh]]

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

Comment 26

[Stefan [:stefanh]]

Comment on attachment 470296 [details] [diff] [review]
Final patch

Asking IanN for feedback since Neil passed the sr.

Comment 27

[Stefan [:stefanh]]

Attachment: New version with namespace declarations

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

Comment 28

[Stefan [:stefanh]]

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

[Stefan [:stefanh]]

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 30

[Karsten Düsterloh]

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.

Comment 31

[Stefan [:stefanh]]

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

Comment 32

[Stefan [:stefanh]]

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

[Ian Neal]

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)

Comment 34

[Stefan [:stefanh]]

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