460699 - Make the default theme look better on mac

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Stefan [:stefanh]]

Some things that I think will be necessary (putting this on blocking-radar for now):

- unified toolbar
- some icon tweaks: Keep the original icons, but tweak them so they fit better on mac, this is necessary to make the main toolbar icons work with a unified toolbar
- new tabbrowser look (tab, tabs, tabpanels, loading icon etc)
- lots of small tweaks (fonts, colors, paddings, margins, maybe drop some wrongly used -moz-appearance stuff

Note: this will probably require a new set of mac-specific files.

Comment 1

[Stefan [:stefanh]]

(setting a possible target milestone)

Comment 2

[Robert Kaiser]

Let's put it up as wanted, not sure about blocking, but it's on a radar at least.

Comment 3

[Stefan [:stefanh]]

Adding a bunch of bugs that will hopefully be fixed here.

Comment 4

[Stefan [:stefanh]]

Attachment: communicatoricons.png for mac

This is a version of communicatoricons.png that is suitable for mac. They hover:active (second column) and disabled state (third column) matches quite well how the icons would look in those states in a native cocoa app.

Comment 5

[Stefan [:stefanh]]

Attachment: editoricons.png

Comment 6

[Stefan [:stefanh]]

Attachment: messengericons.png

Comment 7

[Stefan [:stefanh]]

Attachment: navigatoricons.png

Comment 8

[Robert Kaiser]

1) Any reason we're not just going with different opacity to get those?
2) Do they fit well with the unified toolbar style, so we can can enable this once we're using those icons?

Comment 9

[Stefan [:stefanh]]

Attachment: Screenshot of navigator icons in unified toolbar

There's lots of work left to be done in the css files, but here's a screenshot of tweaked navigator icons in a unified toolbar. Apart from maybe tweaking the disabled ones, I don't think there's so much more to do when it comes to those icons (after all, I'm just tweaking the default ones).

Comment 10

[Stefan [:stefanh]]

(In reply to comment #8)
> 1) Any reason we're not just going with different opacity to get those?

I'm a bit unsure if that will work well when the window is not focused, because the standard thing to do then is to put opacity on all toolbarbutton icons (toolbars, statusbar should also be grayed out).

Comment 11

[Robert Kaiser]

OK, thanks for clearing this up.

And you're right in that disabled looks too little disabled in that screen shot, at least for my eyes. Is that normal on Leopard or did you just make them too little transparent?

Comment 12

[Markus Stange [:mstange]]

(In reply to comment #10)
> because
> the standard thing to do then is to put opacity on all toolbarbutton icons
> (toolbars, statusbar should also be grayed out).

Actually, I'd love to get rid of that; no native Cocoa app does that. I think Kevin did it to indicate that the buttons don't accept click-through (in contrast to native Cocoa apps). (Click-through is bug 392188.)

But IMO it just looks ugly... and users don't know about click-through and its indications anyway.

Comment 13

[Stefan [:stefanh]]

(In reply to comment #12)

> Actually, I'd love to get rid of that; no native Cocoa app does that.

Hmm, that's a point. But maybe Kevin was right since the buttons doesn't accept click-through... Have to think about it :-)

Comment 14

[Philip Chee]

Err how about small icon versions for those buttons or are the existing small icons suitable?

Comment 15

[Stefan [:stefanh]]

(In reply to comment #14)
> Err how about small icon versions for those buttons or are the existing small
> icons suitable?

Of course there will be small versions as well. I just haven't done them yet.

Comment 16

[Stefan [:stefanh]]

Markus suggested that I could use opacity instead of providing icons with disabled look (he changed his mind re comment #12). Now when I think of it, I could actually do something like this:

"toolbarbutton[disabled="true"] > .toolbarbutton-icon {
  opacity: 0.5;
}"

Markus, would it make sense to make at least all toolbarbuttons in the main toolbar look disabled when the window isn't active (opacity of 0.5 on the icons and also make the label text have the disabled look)?

Comment 17

[Markus Stange [:mstange]]

(In reply to comment #16)
> Markus, would it make sense to make at least all toolbarbuttons in the main
> toolbar look disabled when the window isn't active (opacity of 0.5 on the icons
> and also make the label text have the disabled look)?

Yes, I think that's the right way to go. Even though it doesn't look too good IMO...

Comment 18

[Stefan [:stefanh]]

So, what are the chances that someone hides the left/rightmost tab in pageInfo? I found a way to make pageinfo look like the add-on mngr (yeah, for windows it might look bad, but on mac it's really macish), but since the "outer" tabs are rounded and there is no magic attribute to keep track on visibility...

Comment 19

[Stefan [:stefanh]]

Attachment: WIP1

Just attaching a diff against the current files. I didn't used any git diff, because the patch would have been quite large then. But I have all toolbar icons in both sizes. I haven't done anything (yet) about the non-active states and currently I have icons for all 3 states. mstange showed me some cool svg stuff that might make it possible to just have one set of icons for all states, but I haven't had the time to play with it -  I've been trying to focus on the overall look first.

Lots of to-do's, including:

- Figure out if it's ok restyling the pageinfo window despite comment #18
- Get some new icons (folder icons are a priority)
- Figure out what to do with the search button
- Figure out what to do with the grippies (I fail to understand how they can be made to fit in with a native look). Currently they're hidden and that would be what I personally would use and what I would eventually put up on AMO. I might be able to do something with the splitter grippies, if can attach some enhanced binding, though.

Comment 20

[Stefan [:stefanh]]

Attachment: Another wip

As previous patch, this one is made without any forked files - it's a diff against the existing classic files. This patch also incudes all icons. Basically, the only difference compared to the previous patch is that I'm using the toolkit dirListing folder icon as a bookmarks folder. It's not perfect, but I wanted to see how it looked.

Apart from the previous to-do's I think the messengerwindow needs some love. In general, the code also needs to be cleaned up.

Comment 21

[Stefan [:stefanh]]

Attachment: Screenshot, browser and mailNews

Here's a screenshot showing browser and MailNews windows with the patch applied

Comment 22

[Robert Kaiser]

(In reply to comment #21)
> Here's a screenshot showing browser and MailNews windows with the patch applied

Judging from the screen shot alone, I'd be happy to go with this for Beta, it's probably better already than what we have now, and I'd really like to get feedback on it from people. The main thing that stands out for me (apart from the missing grippies, which are as we know their own topic) is that the location bar looks too square compared to all the rounded stuff around, but that can be fixed in a followup, IMHO.

Comment 23

[Stefan [:stefanh]]

Attachment: wip3

Comment 24

[Stefan [:stefanh]]

Attachment: wip4

Comment 25

[Stefan [:stefanh]]

Attachment: jar.mn diff

I'll need some feedback on how to arrange stuff. This is a diff of the current jar.mn file I have. I've just put the forked files in a mac dir together with the new icons. That is, themes/classic/navigator/mac/ <-- css files and icons. I haven't done the same for non-mac. There are a few icons that are mac-only, but they're packaged for all OS (as non-mac icons are packaged for mac).

Comment 26

[Stefan [:stefanh]]

(In reply to comment #25)
> Created an attachment (id=374593) [details]
> jar.mn diff
> 
> I'll need some feedback on how to arrange stuff. This is a diff of the current
> jar.mn file I have. I've just put the forked files in a mac dir together with
> the new icons. That is, themes/classic/navigator/mac/ <-- css files and icons.
> I haven't done the same for non-mac.

Actually, that isn't really true. Only when there is a subdir the win/nix files haven't been moved to topdir/win/.

Comment 27

[Stefan [:stefanh]]

Attachment: Complete patch

Here's a complete patch that also adds the files. Now that I think of it, I forgot to actually move the windows files.

Comment 28

[Stefan [:stefanh]]

So, what's the best procedure here in order to preserve history as much as possible?

Comment 29

[Stefan [:stefanh]]

Attachment: classic.jar, for testing

I'm attaching a classic.jar package that can be used for testing. Note that the pageinfo window will lack a unified toolbar, because pageInfo.xul needs this:

+  <windowdragbox>
     <tabs id="tabs" onselect="[gImageView, gFormView, gLinkView].forEach(ensureSelection);">
       <tab id="generalTab"  label="&generalTab;"  accesskey="&generalTab.accesskey;"/>
       <tab id="mediaTab"    label="&mediaTab;"    accesskey="&mediaTab.accesskey;"/>
       <tab id="feedTab"     label="&feedTab;"     accesskey="&feedTab.accesskey;"/>
       <tab id="permTab"     label="&permTab;"     accesskey="&permTab.accesskey;"/>
       <tab id="formsTab"    label="&formsTab;"    accesskey="&formsTab.accesskey;"/>
       <tab id="linksTab"    label="&linksTab;"    accesskey="&linksTab.accesskey;"/>
       <tab id="securityTab" label="&securityTab;" accesskey="&securityTab.accesskey;"/>
       <!-- Others added by overlay -->
     </tabs>
+  </windowdragbox>

Comment 30

[Stefan [:stefanh]]

OK, so the current status here is that I think things are "ready to use". There are some rough edges, but it looks much better than the current look.

The controversial (?) parts are:
1) No grippies - there's no way we can use the current ones with the new look. I also don't really see how any grippy could be made to fit in with a mac "native" look. Mac windows do have a similar feature, though - in the title bar there's usually a pill button that hides the toolbar (yeah, native mac apps have one 'real' toolbar). Mozilla XUL apps will hide all their toolbars when you click this pill button.
2) The current patch also lacks splitter grippies. It might be possible to make them fit with the ui, but the current ones are definitely not suitable. Note also that the new Leopard ui makes usage (in certain places) of a so-called "zero-width splitter": http://devworld.apple.com/documentation/UserExperience/Conceptual/AppleHIGuidelines/XHIGControls/XHIGControls.html#//apple_ref/doc/uid/TP30000359-CHDDBIJE

JFTR, I use this style in mailNews/Addressbook.

As you can see, it's a bit of a challenge to make splitter grippies integrate :/

Some comments: The Personal Toolbar is basically a Camino rip-off and I might want to change that in the future (even though I like it, I'd like sm to have its own PT style). Basically, when I started the toolbar work I decided to not follow Safari/Firefox (dark secondary 'toolbars') - so I used the same style as Camino for the PT instead (lighter, and text-shadow on labels in hover:active/open states)

As for the rest, there is some work left - even though I think this is much better that the current look. Messenger will need some more love and Sidebar definitely needs love. Browser tabs need some polish as well.

Comment 31

[Stefan [:stefanh]]

Comment on attachment 374665 [details]
classic.jar, for testing

Mark said he could have a look at this. Note the previous comment re the page info window.

Comment 32

[Stefan [:stefanh]]

> Note the previous comment re the page
> info window.

Sorry, that would be comment #29. Note that attachment #374597 [details] [diff] [review] should work fine, but the layout of the forked files might not be correct. And I have just added all files (not addremoved/moved them)

Comment 33

[Stefan [:stefanh]]

It appears that content in certain account settings panels doesn't fit (cropped at bottom). This is even worse without my fixes here, but the current classic.jar package suffers from too large panel buttons.

Comment 34

[Stefan [:stefanh]]

Attachment: Updated classic.jar

Here's an updated package that addresses my previous comment.

Comment 35

[Stefan [:stefanh]]

Attachment: Screenshot showing main panel in account manager before/after

Re comment #33 - here's a screenshot, unpatched/patched account manager (main panel - server settings panel has similar issues). I can't really do much more in the theme files, so I think we need about 1em more vertical space in order for it to look good (I haven't tried but we have 49em and thunderbird have 50em).

Comment 36

[Robert Kaiser]

Stefan, as this patch already makes things better, this looks like a good candidate for a followup issue to me. Let's get in what we have here now and work on further improvements in followups, with one issue per bug/patch, if possible.

Comment 37

[Stefan [:stefanh]]

mstange noted 2 things when he tried the package (I add it here so I won't forget about it):

1. the bookmarks toolbar is draggable, although it's not dark
2. on 10.4, the tabstrip doesn't have stripes, but the selected tab does

For 1) we'll need 'nowindowdrag="true"' in the xul. Of course, atm all our toolbars are draggable (and none of them are dark)
For 2) I'll need to investigate, but it's probably just an adjustment of a bg-color.

Comment 38

[Stefan [:stefanh]]

Attachment: Patch against current theme files

Attaching a patch against the current theme files - no file moves. Some minor changes included. I've got some suggestions for how to organize the forked files, but I think it's easier to keep the patch without any file moves as long as possible.

Comment 39

[Stefan [:stefanh]]

Attachment: Updated classic.jar

This package is made from attachment #377958 [details] [diff] [review] - it includes some minor changes I've done since last time. Please note comment #29.

Comment 40

[Mark Banner (:standard8)]

(In reply to comment #38)
> Created an attachment (id=377958) [details]
> Patch against current theme files

This patch doesn't have communicator/bookmarks/bookmark-folder-active.png and I suspect some others.

(In reply to comment #39)
> Created an attachment (id=377959) [details]
> Updated classic.jar

Some first impressions:

- The contrast between buttons disabled and enabled isn't good enough. When I first started SeaMonkey I was confused as to if the back and forward where enabled or not until I actually selected another page and enabled one. This might just take a bit of getting used to, but I'm not sure.
- The search bar in mailnews is styled with a slight shadow, but the rest of the equivalent bars in the app are not. This gives a mismatch of inconsistent backgrounds.
- The mix of folder pane background, thread pane background and header background in mailnews is a bit strange. The folder pane & header backgrounds especially don't seem to quite fit together.
- The zero-width grippy in address book I think doesn't work. Especially when collapsed. We had bug reports on Thunderbird due to the address book pane being collapsed and users not realising it was there because although there was a bar there was no grippy indication at the time. Having no bar at all means the user is even more likely to not know about it if they aren't used to the app.

Comment 41

[Stefan [:stefanh]]

(In reply to comment #40)
> (In reply to comment #38)
> > Created an attachment (id=377958) [details] [details]
> > Patch against current theme files
> 
> This patch doesn't have communicator/bookmarks/bookmark-folder-active.png and I
> suspect some others.

Oops, sorry.

> 
> (In reply to comment #39)
> > Created an attachment (id=377959) [details] [details]
> > Updated classic.jar
> 
> Some first impressions:
> 
> - The contrast between buttons disabled and enabled isn't good enough. When I
> first started SeaMonkey I was confused as to if the back and forward where
> enabled or not until I actually selected another page and enabled one. This
> might just take a bit of getting used to, but I'm not sure.

It might be habit since both the disabled and the hover:active "looks" are copied from other mac apps (disabled is basically the normal icon with 50% opacity and active look is a light reduction - mac apps doesn't need to provide 3 sets of icons).

> - The search bar in mailnews is styled with a slight shadow, but the rest of
> the equivalent bars in the app are not. This gives a mismatch of inconsistent
> backgrounds.

Do you mean the actual search field or the whole bar (with the Advanced button etc)? If you mean the whole bar, I used a slightly darker gradient than the personal toolbar, because I thought it looked better. Can you give an example of other bars that you think should have the same background? If you think of messengercompose and composer, they're definitely candidates. I haven't really done so much work on messengercompose and composer - the format bars needs some love, but I wonder how the icons would look with a darker bg-color.  

> - The mix of folder pane background, thread pane background and header
> background in mailnews is a bit strange. The folder pane & header backgrounds
> especially don't seem to quite fit together.

The colors are the same in Addressbook, except for the header background. Maybe the header background should be white instead? We could have a border surrounding the header to separate it from the rest.

> - The zero-width grippy in address book I think doesn't work.

I'm open for suggestions since I'm unsure of how to fix this and still keep the "mac" look. Basically, all panes with a zero-width splitter have a standard blue background-color. Those panes are called "Primary source lists" in the mac world. With a thick vertical splitter, it'd probably look odd if I kept the background-color - but having the old background-color would make the pane look a bit outdated, I think. That said, I do realize the problem with those thin splitters. Maybe it could be fixed with xbl somehow - although not perfect, Apple's Mail has a little drag-button at the lower right of the folder pane. Maybe it's possible to do something similar (and better)?

Comment 42

[Stefan [:stefanh]]

Comment on attachment 374593 [details] [diff] [review]
jar.mn diff

I've discussed the directory structure with Neil and I'll go for suite/themes/classic/mac/ <- mac-specific files, but in the master folder
structure.

Comment 43

[Stefan [:stefanh]]

Attachment: New patch against current theme files

Here's an updated patch - the old one had bitrottened.

Comment 44

[Mark Banner (:standard8)]

(In reply to comment #41)
> (In reply to comment #40)
> > (In reply to comment #38)
> > - The contrast between buttons disabled and enabled isn't good enough. When I
> > first started SeaMonkey I was confused as to if the back and forward where
> > enabled or not until I actually selected another page and enabled one. This
> > might just take a bit of getting used to, but I'm not sure.
> 
> It might be habit since both the disabled and the hover:active "looks" are
> copied from other mac apps (disabled is basically the normal icon with 50%
> opacity and active look is a light reduction - mac apps doesn't need to provide
> 3 sets of icons).

Ok, looking again these aren't too bad, lets see how it goes.

> > - The search bar in mailnews is styled with a slight shadow, but the rest of
> > the equivalent bars in the app are not. This gives a mismatch of inconsistent
> > backgrounds.
> 
> Do you mean the actual search field or the whole bar (with the Advanced button
> etc)?

The whole bar

> If you mean the whole bar, I used a slightly darker gradient than the
> personal toolbar, because I thought it looked better. Can you give an example
> of other bars that you think should have the same background? If you think of
> messengercompose and composer, they're definitely candidates. I haven't really
> done so much work on messengercompose and composer - the format bars needs some
> love, but I wonder how the icons would look with a darker bg-color.  

Well when I look at it, unless I look very closely the personal toolbar doesn't look like it has any gradient (and actually there seems to be a 1 or 2 pixel lighter line at the bottom of it). So I think darkening it slightly may be better to make it look like it has some gradient.

> > - The mix of folder pane background, thread pane background and header
> > background in mailnews is a bit strange. The folder pane & header backgrounds
> > especially don't seem to quite fit together.
> 
> The colors are the same in Addressbook, except for the header background. Maybe
> the header background should be white instead? We could have a border
> surrounding the header to separate it from the rest.

A white background isn't good - too plain. I guess the contrast isn't too bad and we can just see how it goes.

> > - The zero-width grippy in address book I think doesn't work.
> 
> I'm open for suggestions since I'm unsure of how to fix this and still keep the
> "mac" look. 
...
> That said, I do realize the problem with those thin
> splitters.

I'm not sure what to do here, but I think the narrow splitters could just be undiscoverable to some folks. I've just realised that in the few mac cases I've looked at they get away with zero size because they don't allow the pane to collapse fully.

Comment 45

[Mark Banner (:standard8)]

Comment on attachment 386111 [details] [diff] [review]
New patch against current theme files

minus based on the previous comments, though this is generally looking much better.

Comment 46

[Stefan [:stefanh]]

Mark and I had talk, and I'll look at some solutions re the narrow splitters later on this week. Basically, the idea is to show something (maybe a thicker splitter) in the collapsed state to indicate that there is a way to get the pane "back".

Comment 47

[Stefan [:stefanh]]

Attachment: have a 5px wide splitter when it's collapsed

This is one suggestion re the splitter: Make it 5px wide when collapsed. I think it'd work - I've changed the cursor to e-resize as well (not visible in the screenshot).

Comment 48

[Stefan [:stefanh]]

Regarding the toolbar backgrounds, mark was ok with keeping the current look. I've been playing a bit with the different gradients and I think I should re-visit this once this bug is closed. In my local tree, the PT gradient is rgb(236, 236, 236) --> rgb(214, 214, 214) atm, while the tabstrip is rgb(213, 213, 213). However, I think the tabstrip might look better if it's darker - I'm also not 100% sure that the colors should be done with rgba values since they will then differ depending on what background the parent window has.

Comment 49

[Stefan [:stefanh]]

Attachment: More or less complete patch

More or less complete patch (minor code-cleanup left), includes copying/moving. Non-win/nix files are now in their resp platform-dir in classic/ (original structure preserved). Files in "win" folders have been moved out to the main folders. The patch doesn't remove the old folders, I'll do that as a 2:nd step.

Some look & feel changes:
- splitter is now wide when it's collapsed.
- added nowindowdrag="true" to all toolbars that are not dark grey.
- I decided to remove bookmark-item-active.png for now, since I have no styling for active bookmark favicons. I think I can fix this with svg, but I'll look at that in a follow-up.

Comment 50

[Stefan [:stefanh]]

Comment on attachment 388756 [details] [diff] [review]
More or less complete patch

Sorry, I forgot a couple of files, gimme a few minutes...

Comment 51

[Stefan [:stefanh]]

Attachment: Correct version

OK, this one should be correct. I've also done renames where possible. See also comment #49.

Comment 52

[Stefan [:stefanh]]

Attachment: New version

I found some glitches that I wanted to correct, so here's a new patch:

1) I noticed that the threadpane splitter looked bad in vertical layout. It looks better now - might need some more tweaks, but I'd prefer doing that in a follow-up.

2) The header twisties should look OK now. The old patch used -moz-appearance and for some reason I found it hard to make them behave as I wanted to with moz-appearance, so I added 2 icons instead (they mimic the tree twisties, so the look hasn't really changed except that in this version the "From" twisty is also changed).

Comment 53

[Stefan [:stefanh]]

Comment on attachment 389402 [details] [diff] [review]
New version

For some reason I made a mistake when fixing the vertical splitter in mailNews- it's not 1px wide anymore. I've fixed that locally and it now looks like the splitter in Addressbook.

Comment 54

[Mark Banner (:standard8)]

Comment on attachment 389402 [details] [diff] [review]
New version

Ok, let's go with this for now.

Comment 55

[Stefan [:stefanh]]

Attachment: Addressbook - left pane

This change is not in the patch (no column header), but Mark was ok with it. So, I'll include it in the final patch.

Comment 56

[Robert Kaiser]

Stefan, for code review, I guess it's best to go with r?Standard8 sr?Neil as Standard8 already knows what it ends up as visually and works full-time on Mozilla stuff (easier to sneak in some time for us) and Neil needs to look at it in some way anyhow, I guess.

Comment 57

[Stefan [:stefanh]]

Attachment: Final patch

Here's the final patch. Should be ready for review. I'm a little bit unsure of who should do the r? part, but I figured Neil can look at this until we know.

Comment 58

[Stefan [:stefanh]]

(In reply to comment #57)

> but I figured Neil can look at this until we know.

I was trying to say that Neil could review this from an sr pow now, since the requirements is an sr from him anyway.

Comment 59

[Stefan [:stefanh]]

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

JFTR: Git informed me about a couple of whitespace adds, that I've corrected locally.

Comment 60

[[e-mail]]

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

>+    <windowdragbox>
>+      <tabs id="tabs" onselect="[gImageView, gFormView, gLinkView].forEach(ensureSelection);">
What's this for? Why would I want to drag Page Info anyway?

>+#elif XP_OS2
>+  skin/classic/communicator/toolbar.css                                 (os2/communicator/toolbar.css)
> #else
This is no good. I know it wants its own toolbar.css but it still needs the rest of the non-Mac stuff!

>+#historyTree {
>+  border: none;
>+  margin: 0;
>+}
I think adding class="plain" to the tree might work better.

[...]
+#help-forward-button[buttondown="true"] > .toolbarbutton-menubutton-dropmarker {
+  list-style-image: url("chrome://global/skin/arrow/arrow-dn.png") !important;
+  -moz-image-region: auto !important;
+
 }
So what image do you get by default?
[Nit: blank line before the }]

>+  -moz-border-end: 1px solid #bebebe;
Nit: uppercase colour codes please

>copy from suite/themes/classic/communicator/icons/communicatoricons-small.png
>copy to suite/themes/classic/mac/communicator/icons/communicatoricons-small.png
>index ecb085538365d8b5cdba5885d4a48d91e06a09c2..8c8ad1bdae9279fd8488ae08ddb2323832427fd3
>GIT binary patch
Why is it worth hg copying these?

>+/* Error Console toolbar doesn't center without this */
>+#ToolbarMode .toolbar-holder {
>+  -moz-box-pack: center;
Does adding class="box-inherit" to the toolbar-holder help?

>+#editorWindow {
>+  -moz-appearance: none;
>+  background-color: #eeeeee;
>+}
>+
>+#editorWindow[chromehidden~="toolbar"][chromehidden~="location"][chromehidden~="directories"] {
>+  border-top: 1px solid rgba(0,0,0,0.65);
What appearance would you normally get and why is it wrong?

Comment 61

[[e-mail]]

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

>+button:not(.spinbuttons-button):not(.dialog-button) {
>+  min-height: 19px;
>+}
What's this needed for?

>+#addressbookWindow:not([active="true"]) > hbox > #dirTreeBox {
>+  background-color: #e8e8e8;
>+}
Should go after the rule for #dirTreeBox so you can see the override.

>+#dirTree-splitter[state="collapsed"] {
>+  min-width: 5px;
>+  background-color: #d6dde5;
>+  cursor: e-resize;
Why change the width of the splitter when it's collapsed?
[Changing the cursor should ideally be done in global...]

>+  font-weight: 600;
As a name please?

>+/* Make sure users find the splitter when it's collapsed... */
Aha...

>+#threadpane-splitter {
>+  background-color: -moz-dialog;
>+  border-top: 1px solid #a5a5a5;
>+  border-bottom: 1px solid #a5a5a5;
>+}
>+
>+/* Thick vertical splitter */
>+
>+#threadpane-splitter:not([orient="vertical"]) {
>+  border: solid #b3b3b3;
>+  border-width: 0 0 0 1px;
This makes no sense, because you have the vertical splitter before the horizontal one, no? Anyway, it would look neater if you swapped them:
+#threadpane-splitter {
+  background-color: -moz-dialog;
+  border: solid #b3b3b3;
+  border-width: 0 0 0 1px;
+}
+
+/* Thick vertical splitter */
+
+#threadpane-splitter[orient="vertical"] {
+  border: solid #a5a5a5;
+  border-width: 1px 0;
[Do we have an RTL-safe version of border-width: 0 0 0 1px; ?]

> #link-last:hover,
> #link-last:hover:active
...
>-#link-feed:hover {
>-  -moz-image-region: rect(16px 32px 32px 16px);
>-}
Obviously we have redundancy in Classic because :hover:active is included by :hover but what do you really want to do on the Mac?

Comment 62

[Stefan [:stefanh]]

(In reply to comment #60)
> (From update of attachment 393377 [details] [diff] [review])
> >+    <windowdragbox>
> >+      <tabs id="tabs" onselect="[gImageView, gFormView, gLinkView].forEach(ensureSelection);">
> What's this for? Why would I want to drag Page Info anyway?

I give the pageInfo window a toolbar by styling the windowdragbox as a unified toolbar. Basically, the tabs are made to look like viewbuttons, kind of a mix between tabs and toolbarbuttons. I think it's more suitable for this window (makes it look like error console/add-ons manager).
> 
> >+#elif XP_OS2
> >+  skin/classic/communicator/toolbar.css                                 (os2/communicator/toolbar.css)
> > #else
> This is no good. I know it wants its own toolbar.css but it still needs the
> rest of the non-Mac stuff!

Ugh, sorry. Fixed (you might have a better idea of how to do it, though).
> 
> >+#historyTree {
> >+  border: none;
> >+  margin: 0;
> >+}
> I think adding class="plain" to the tree might work better.


Yes, you're right. Thanks (added class="plain").

> [...]
> +#help-forward-button[buttondown="true"] > .toolbarbutton-menubutton-dropmarker
> {
> +  list-style-image: url("chrome://global/skin/arrow/arrow-dn.png") !important;
> +  -moz-image-region: auto !important;
> +
>  }
> So what image do you get by default?
http://mxr.mozilla.org/mozilla1.9.1/source/toolkit/themes/pinstripe/global/toolbar/dropmark-nav.png and it's only there for the help toolbar and it only fits with the default toolbar icons.

> [Nit: blank line before the }]

Fixed. Fwiw, I noticed that the dropmark-nav.png appeared when pressing a disabled button, so I looked through the style rules again and discovered that I don't need the dreadful descendant selector and that I needed add some "not([disabled="true"])".
> 
> >+  -moz-border-end: 1px solid #bebebe;
> Nit: uppercase colour codes please

Fixed all instances :-)
> 
> >copy from suite/themes/classic/communicator/icons/communicatoricons-small.png
> >copy to suite/themes/classic/mac/communicator/icons/communicatoricons-small.png
> >index ecb085538365d8b5cdba5885d4a48d91e06a09c2..8c8ad1bdae9279fd8488ae08ddb2323832427fd3
> >GIT binary patch
> Why is it worth hg copying these?

No special reason apart from that this is a tweak of the original ones and I figured it could be worth recording that. But if you want I can just add all toolbar icon sets (editor, navigator, messenger etc).
> 
> >+/* Error Console toolbar doesn't center without this */
> >+#ToolbarMode .toolbar-holder {
> >+  -moz-box-pack: center;
> Does adding class="box-inherit" to the toolbar-holder help?


No, it doesn't (the .toolbar-box hbox has "-moz-box-pack: center as computed style).
> >+#editorWindow {"
> >+  -moz-appearance: none;
> >+  background-color: #eeeeee;
> >+}
> >+
> >+#editorWindow[chromehidden~="toolbar"][chromehidden~="location"][chromehidden~="directories"] {
> >+  border-top: 1px solid rgba(0,0,0,0.65);
> What appearance would you normally get and why is it wrong?
I think the main difference is what background-color the window has, but we don't show any part of the bg in the window so in reality there is no difference. But when the toolbar(s) are collapsed, you need a border at the top since there's no bottom-border on the title bar and the -moz-appearance rule makes it impossible to add this border. The alternative would be to use -moz-appearance: none for "#editorWindow[chromehidden~="toolbar"]... ..." ( I think Firefox/Toolkit used to do that, but now they're doing it like I do here), but then I think I'd have problems with the tab strip et al because the bg-color is done with rgba values. That said, I think I should look at the rgba usage and use "real" colors instead (follow-up, please).


(In reply to comment #61)
> (From update of attachment 393377 [details] [diff] [review])
> >+button:not(.spinbuttons-button):not(.dialog-button) {
> >+  min-height: 19px;
> >+}
> What's this needed for?
 All buttons in the account manager, except the standard dialog buttons (and the spinbuttons) should be a bit smaller, otherwise we'll run out of space. See comment #35. This is actually a rip-off from the equivalent mail file.

> >+#addressbookWindow:not([active="true"]) > hbox > #dirTreeBox {
> >+  background-color: #e8e8e8;
> >+}
> Should go after the rule for #dirTreeBox so you can see the override.


OK - fixed. Fixed the one in mailWindow1.css as well.

> >+#dirTree-splitter[state="collapsed"] {
> >+  min-width: 5px;
> >+  background-color: #d6dde5;
> >+  cursor: e-resize;
> Why change the width of the splitter when it's collapsed?
> [Changing the cursor should ideally be done in global...]
> 
> >+  font-weight: 600;
> As a name please?

Sorry, for some reason I thought that 600 wasn't bold...

> >+/* Make sure users find the splitter when it's collapsed... */
> Aha...
I added that comment in the addressbook file.

> >+#threadpane-splitter {
> >+  background-color: -moz-dialog;
> >+  border-top: 1px solid #a5a5a5;
> >+  border-bottom: 1px solid #a5a5a5;
> >+}
> >+
> >+/* Thick vertical splitter */
> >+
> >+#threadpane-splitter:not([orient="vertical"]) {
> >+  border: solid #b3b3b3;
> >+  border-width: 0 0 0 1px;
> This makes no sense, because you have the vertical splitter before the
> horizontal one, no? Anyway, it would look neater if you swapped them:
> +#threadpane-splitter {
> +  background-color: -moz-dialog;
> +  border: solid #b3b3b3;
> +  border-width: 0 0 0 1px;
> +}
> +
> +/* Thick vertical splitter */
> +
> +#threadpane-splitter[orient="vertical"] {
> +  border: solid #a5a5a5;
> +  border-width: 1px 0;
> [Do we have an RTL-safe version of border-width: 0 0 0 1px; ?]
I dunno about the RTL. I swapped them (and used uppercase color letters). I did this because the splitter needs to look a bit different depending on what layout you use (vertical vs other).

> > #link-last:hover,
> > #link-last:hover:active
> ...
> >-#link-feed:hover {
> >-  -moz-image-region: rect(16px 32px 32px 16px);
> >-}
> Obviously we have redundancy in Classic because :hover:active is included by
> :hover but what do you really want to do on the Mac?

In the toolbar, I want the normal one in the normal state, for the hover:active state I think the rect(48px 32px 64px 16px) works, but ideally it should possibly be a bit darker. The disabled one is far too washed out, so I use opacity: 0.5 on all those smaller buttons in PT and LT. I'm not sure if the opacity is the best solution since I'm a bit worried that it'd be a bit expensive, but I figured it'd be OK for the first round.

Comment 63

[Karsten Düsterloh]

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

So I fed this patch into Tiger and it looked quite decent indeed. 
While this isn't "real" fullscale/detailed review, I do have some notes'n'nits:

>diff --git a/editor/ui/composer/content/editor.xul b/editor/ui/composer/content/editor.xul
>+  <toolbar class="chromeclass-toolbar" id="FormatToolbar" persist="collapsed"
>+           nowindowdrag="true" grippytooltiptext="&formatToolbar.tooltip;">

If there are more attributes than will fit onto one line, please - for the sake of readability - use one line per attribute and align them vertically, preferrably sorting attributes like id-label-accesskey-key-class-persist-other-command.
Same goes for all XUL elements you touch below, especially for those where id isn't the first attribute.

>+  color: #7C7C7C !important; /* remove this when we support click-through */

You have quite some hardcoded colours in your patch - I gather there's no respective native theming colour (-moz-something) you can take?

Comment 64

[Stefan [:stefanh]]

(In reply to comment #63)
> (From update of attachment 393377 [details] [diff] [review])
> So I fed this patch into Tiger and it looked quite decent indeed. 
> While this isn't "real" fullscale/detailed review, I do have some notes'n'nits:
> 
> >diff --git a/editor/ui/composer/content/editor.xul b/editor/ui/composer/content/editor.xul
> >+  <toolbar class="chromeclass-toolbar" id="FormatToolbar" persist="collapsed"
> >+           nowindowdrag="true" grippytooltiptext="&formatToolbar.tooltip;">
> 
> If there are more attributes than will fit onto one line, please - for the sake
> of readability - use one line per attribute and align them vertically,
> preferrably sorting attributes like
> id-label-accesskey-key-class-persist-other-command.
> Same goes for all XUL elements you touch below, especially for those where id
> isn't the first attribute.

Sure, if neil is ok with that I can re-indent everything I touch.

> >+  color: #7C7C7C !important; /* remove this when we support click-through */
> 
> You have quite some hardcoded colours in your patch - I gather there's no
> respective native theming colour (-moz-something) you can take?

No, unfortunately it isn't.

Comment 65

[Stefan [:stefanh]]

Attachment: Final patch - corrected

Fixed all Neil's comments - see comment #62 (on IRC, Neil suggested box-inherit in the binding in 2 places - it worked, yay!).
Removed the hover: rules in linkToolbar.css (I forgot about them)
Re-indented all xul that I've touched according to Karsten's comment
Fixed one style rule in helpOverlay.css (wrong id)

Comment 66

[[e-mail]]

Comment on attachment 394125 [details] [diff] [review]
Final patch - corrected

>+#help-back-button > .toolbarbutton-menubutton-dropmarker,
>+#help-back-button:not([disabled="true"]):hover > .toolbarbutton-menubutton-dropmarker,
>+#help-back-button:not([disabled="true"]):hover:active > .toolbarbutton-menubutton-dropmarker,
>+#help-forward-button > .toolbarbutton-menubutton-dropmarker,
>+#help-forward-button:not([disabled="true"]):hover > .toolbarbutton-menubutton-dropmarker,
>+#help-forward-button:not([disabled="true"]):hover:active > .toolbarbutton-menubutton-dropmarker {
>+  list-style-image: url("chrome://global/skin/arrow/arrow-dn.png") !important;
>+  -moz-image-region: auto !important;
So, I take it the reason you're using :not is because you're using !important?
(I'd prefer it if you could remove both, but if you can't, you can't.)

>+#dirTree-splitter {
>+  border: solid #B3B3B3;;
Nit: double ;

Comment 67

[Stefan [:stefanh]]

Landed attachment #394125 [details] [diff] [review] with Neils comment #66 addressed (didn't need the :not and !important): http://hg.mozilla.org/comm-central/rev/ee7ce2ff0a5d

Comment 68

[Philip Chee]

-    <toolbar id="PersonalToolbar" class="chromeclass-directories" persist="collapsed"
+    <toolbar id="PersonalToolbar"
+             accesskey="&personalbarCmd.accesskey;"
+             class="chromeclass-directories"
+             persist="collapsed"
              grippytooltiptext="&personalToolbar.tooltip;"
-             toolbarname="&personalbarCmd.label;" accesskey="&personalbarCmd.accesskey;"
+             toolbarname="&personalbarCmd.label;"
+             nowindowdrag="true"

Just a FYI, the accesskey and toolbarname should go together since both are used together to generate the context menu show/hide toolbar menu items.

Comment 69

[Robert Kaiser]

Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query