612172 - Update Modern for Sync UI

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

Bug 576970 will bring Sync (fka. Weave Sync) to SeaMonkey, but only include Classic (Default theme) theming for starters. This bug is for adding at least the minimal set of files needed to use the new feature with Modern (Preferences panel, dialogs, toolbar button; possibly notifications).

Comment 1

[Philip Chee]

Kuden has granted SeaMonkey the rights to use Past Modern under the tri-licence

License:
    * This theme is published under the Mozilla Public License Version 1.1.
    * Exception: If you use the resource included in Past Modern for The SeaMonkey Project, all the codes and images are published under the triple licence (GPL/LGPL/MPL). 

<http://maguroban.s41.xrea.com/theme/pastmodern.html>

I generally look there first to see if there is any relevant CSS or PNGs that I can borrow.

Comment 2

[Ian Neal]

Attachment: Modern issue

This is an issue with modern theme when setting up sync and you enter the wrong password (scrollbar appears).

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #2)
> This is an issue with modern theme when setting up sync and you enter the wrong
> password (scrollbar appears).

Fixed before bug 576970 landed. Thanks for noticing!

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

The below is from Neil's bug 576970 comment 180. The issues that affect Classic, too, need to be fixed there as well.

Had a look at the Modern theme but some of this applies to Classic too.

>+#tabs-display,
>+#tabsList {
>+  background-color: transparent;
>+  -moz-appearance: none;
>+  margin: 0;
>+}
None of this makes sense for #tabs-display. For #tabsList, only the background
colour makes sense; the rest should be implemented via class="plain".

>+#tabsList {
>+  width: 100%;
Already the "default" in XUL. (% usually makes no sense in XUL anyway.)

>+#tabs-display {
>+  background: #fff url(chrome://communicator/skin/sync/sync-bg.png) repeat-x center -80px;
I was going to nit you on #FFFFFF but I found we are already using #FFF
(although not #fff!) a few times.

>+  margin-top: 4px;
>+  -moz-margin-start: 2em;
>+  -moz-margin-end: 2em;
Same as margin: 4px 2px 0px;

>+#tabsListHeading {
>+  font-size: 140%;
>+  font-weight: bold;
[Interestingly HTML headings jump from 1.17em to 1.5em, a little too big.]

>+richlistitem[selected="true"],
>+richlistitem:focus {
>+  outline-style: none;
Modern's richlistitems don't use outlines, they change various colours.

>+  background-color: menu;
Not in Modern you don't! (Happens more than once)

>+.title,
>+.clientName {
>+  color: #000000;
>+  font-size: 1.1em;
>+}
>+
>+.title[selected="true"],
>+.url[selected="true"] {
>+  color: inherit;
Can't these just inherit all the time?

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

KaiRo said in bug 576970 comment 181:

> >+  margin-top: 4px;
> >+  -moz-margin-start: 2em;
> >+  -moz-margin-end: 2em;
> Same as margin: 4px 2px 0px;

You mean |margin: 4px 2em 0px;| ;-)

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Attachment: fix about:sync-tabs theming

This is only the first part, which addresses the initial review comments. I think we should first port more dialogs/setup changes from FF4 before updating Modern further.

(In reply to comment #4)
> >+  background-color: menu;
> Not in Modern you don't! (Happens more than once)

AFAIK we don't have a guide for usage of colors in Modern so I just did a quick comparison of the first file I found per system color value:
-moz-MenuHover -> global/menu.css rule menu[_moz-menuactive="true"]
menu -> global/popup.css rule menupopup
-moz-nativehyperlinktext -> global/global.css rule .text-link

> >+.title,
> >+.clientName {
> >+  color: #000000;
> >+  font-size: 1.1em;
> >+}
> >+
> >+.title[selected="true"],
> >+.url[selected="true"] {
> >+  color: inherit;
> Can't these just inherit all the time?

I found that the color setting for .title and .clientName made no (visual) difference so I removed it. The font-size setting made a difference so I kept it.

Comment 7

[[e-mail]]

(In reply to comment #6)
> (In reply to comment #4)
> > >+.title,
> > >+.clientName {
> > >+  color: #000000;
> > >+  font-size: 1.1em;
> > >+}
> > >+
> > >+.title[selected="true"],
> > >+.url[selected="true"] {
> > >+  color: inherit;
> > Can't these just inherit all the time?
> I found that the color setting for .title and .clientName made no (visual)
> difference so I removed it. The font-size setting made a difference so I kept
> it.
Sorry, I was only referring to the colours, you were right to keep the font.

Comment 8

[[e-mail]]

Comment on attachment 509264 [details] [diff] [review]
fix about:sync-tabs theming

These comments probably apply to both themes.

> #tabsList {
>   background-color: transparent;
> }
> 
> #tabs-display {
>-  background: #fff url(chrome://communicator/skin/sync/sync-bg.png) repeat-x center -80px;
>+  background: #ffffff url(chrome://communicator/skin/sync/sync-bg.png) repeat-x center -80px;
> }
It occurs to me, is it worth putting the background on the #tabsList rather than on the #tabs-display?

>+  margin: 4px 2em 0;
[Nit: I prefer 0px;]

> .url {
>   color: -moz-nativehyperlinktext;
>   font-size: 0.95em;
> }
>+.url[selected="true"] {
>+  color: inherit;
>+}
Nit: blank line between blocks

>+  background: #ffffff url(chrome://communicator/skin/sync/sync-bg.png) repeat-x center -80px;
My comment was way too subtle as usual; my point was that all of our other colour codes are in upper case...

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v1a [Checkin: comment 12]

(In reply to comment #8)
> It occurs to me, is it worth putting the background on the #tabsList rather
> than on the #tabs-display?

Seems to make no visual difference, so why not.

> My comment was way too subtle as usual; my point was that all of our other
> colour codes are in upper case...

That's obviously wrong, as grep will tell you. ITYM "should be".

Comment 10

[[e-mail]]

(In reply to comment #9)
> (In reply to comment #8)
> > My comment was way too subtle as usual; my point was that all of our other
> > colour codes are in upper case...
> 
> That's obviously wrong, as grep will tell you. ITYM "should be".

Well, all our colour codes in suite/themes/modern/communicator/*.css are, which is where I ran my grep... expanding, I see one in wizard.css, one in tree.css and one in editorFormatToolbar.css, plus the unowned EdImageMapPage.css ...

Comment 11

[[e-mail]]

Comment on attachment 509502 [details] [diff] [review]
patch v1a [Checkin: comment 12]

Modern looks great. But for some reason on my Linux Classic build I'm getting a -moz-MenuHover colour of a light cream, which is unreadable with white text :-( Can we use Highlight instead?

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 509502 [details] [diff] [review]
patch v1a [Checkin: comment 12]

http://hg.mozilla.org/comm-central/rev/6cecbf31112d
with comment 11 addressed.

Leaving bug open for actual Modern fixes (sync/*.css have only dialog sizes right now) which I'll probably delay until after more dialog/setup changes have landed.

Comment 13

[Ian Neal]

Should be no string changes, so setting blocking on final

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

Attachment: common, quota, setup

Comment 15

[[e-mail]]

Comment on attachment 523888 [details] [diff] [review]
common, quota, setup

>+.statusIcon[status="active"] {
>+  list-style-image: url("chrome://communicator/skin/brand/throbber16-anim.png");
loading.gif no?

>+treechildren::-moz-tree-checkbox {
>+  list-style-image: none;
>+}
>+treechildren::-moz-tree-checkbox(checked) {
>+  list-style-image: url("chrome://global/skin/checkbox/cbox-check.gif");
>+}
>+treechildren::-moz-tree-checkbox(disabled) {
>+  list-style-image: url("chrome://global/skin/checkbox/cbox-dis-check.gif");
>+}
Does (disabled) get used? I know the other two are duplicates of tree.css, so it might be possible to remove all three.

>+#enabled {
>+  width: 18px;
Actually 18px doesn't seem to be enough. (It's unusual for there to be no image or title on the column header. Maybe we need to set a generic minimum 20px width in tree.css?)

>+  margin: 0em;
>+  padding: 0 8em;
             ^ ;-)

>+.wizard-header-label {
>+  font-size: 24pt;
[I'm not used to seeing pt sizes! I see Help also uses them.]

>+  list-style-image: url("chrome://communicator/skin/brand/throbber16-anim.png");
[Ditto]

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

Attachment: common, quota, setup v1a [Checkin: comment 21]

(In reply to comment #15)
> >+  list-style-image: url("chrome://communicator/skin/brand/throbber16-anim.png");
> loading.gif no?

Absolutely. Followed Ratty's comment 10 over at bug 601562 blindly. :-(

> >+treechildren::-moz-tree-checkbox(disabled) {
> >+  list-style-image: url("chrome://global/skin/checkbox/cbox-dis-check.gif");
> >+}
> Does (disabled) get used?

Doesn't look like it.

> I know the other two are duplicates of tree.css, so
> it might be possible to remove all three.

First, the checked icon was not the same as defined in tree.css, but now that I checked (he) that one I think it's better. Still, I don't like the default (unchecked) one, so we still need two rules.

> >+#enabled {
> >+  width: 18px;
> Actually 18px doesn't seem to be enough.

It was the absolute minimum, but I agree it was a bit tight like that.

> (It's unusual for there to be no image
> or title on the column header. Maybe we need to set a generic minimum 20px
> width in tree.css?)

Agreed.

Comment 17

[[e-mail]]

Comment on attachment 523993 [details] [diff] [review]
common, quota, setup v1a [Checkin: comment 21]

>+treechildren::-moz-tree-checkbox {
>+  list-style-image: none;
>+}
>+
>+treechildren::-moz-tree-checkbox(checked) {
>+  list-style-image: url("chrome://global/skin/tree/checkbox-checked.gif");
>+}
I'm confused. The previous patch used the other checkbox (that's the same in the prefpane list) so I can see why you might want to do the same here. But instead you used exactly the same checkbox that we use in tree.css ...

> }
> 
>+treecol {
>+  min-width: 20px;
Perhaps we could compromise on 19px; since that's the natural width of the treecolpicker, we could merge this into the style block above ;-)

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #17)
> I'm confused. The previous patch used the other checkbox (that's the same in
> the prefpane list) so I can see why you might want to do the same here. But
> instead you used exactly the same checkbox that we use in tree.css ...

See comment 16: "I think it's better. Still, I don't like the default
(unchecked) one, so we still need two rules."

I tried to come up with a :not rule that only overrides the unchecked case but didn't succeed, so I was forced to duplicate the special rule because otherwise the more general one overrides it. Maybe there's another way to do what I want but which I don't know? 

> >+treecol {
> >+  min-width: 20px;
> Perhaps we could compromise on 19px; since that's the natural width of the
> treecolpicker, we could merge this into the style block above ;-)

OK.

Comment 19

[[e-mail]]

(In reply to comment #18)
> (In reply to comment #17)
> > I'm confused. The previous patch used the other checkbox (that's the same in
> > the prefpane list) so I can see why you might want to do the same here. But
> > instead you used exactly the same checkbox that we use in tree.css ...
> See comment 16: "I think it's better. Still, I don't like the default
> (unchecked) one, so we still need two rules."
Oh right, I overlooked that. Sorry.

> I tried to come up with a :not rule that only overrides the unchecked case but
> didn't succeed, so I was forced to duplicate the special rule because otherwise
> the more general one overrides it. Maybe there's another way to do what I want
> but which I don't know? 
No, there isn't.

Comment 20

[[e-mail]]

Comment on attachment 523993 [details] [diff] [review]
common, quota, setup v1a [Checkin: comment 21]

>+treechildren::-moz-tree-checkbox {
>+  list-style-image: none;
>+}
I'd like to stick with the default images for now, it seems more consistent.
r=me with that and the 19px treecol(picker) min width as discussed.

Comment 21

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 523993 [details] [diff] [review]
common, quota, setup v1a [Checkin: comment 21]

http://hg.mozilla.org/comm-central/rev/d07d208da1c2
with comment 20 addressed