Comment 85 for bug 115484

Comment on attachment 344934
New Card Layout - Patch now with seamonkey and thunderbird plus themes

>diff -r 785c708554f7 mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
>--- a/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd Mon Oct 27 14:33:43 2008 +0100
>+++ b/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd Mon Oct 27 17:33:41 2008 +0100
>@@ -17,18 +17,18 @@
> The Initial Developer of the Original Code is
> Netscape Communications Corporation.
> Portions created by the Initial Developer are Copyright (C) 1998-1999
> the Initial Developer. All Rights Reserved.
>
> Contributor(s):
>
> Alternatively, the contents of this file may be used under the terms of
>- either the GNU General Public License Version 2 or later (the "GPL"), or
>- the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ either of the GNU General Public License Version 2 or later (the "GPL"),
>+ or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),

Please don't change this. The license has to be a set format and wording as specified here: http://www.mozilla.org/MPL/boilerplate-1.1/

>@@ -75,49 +75,50 @@
> <!ENTITY PreferMailFormat.label "Prefers to receive messages formatted as:">
> <!ENTITY PreferMailFormat.accesskey "r">
> <!ENTITY PlainText.label "Plain Text">
> <!ENTITY HTML.label "HTML">
> <!ENTITY Unknown.label "Unknown">
> <!ENTITY ScreenName.label "Screen Name:">
> <!ENTITY ScreenName.accesskey "S">
>
>-<!ENTITY allowRemoteContent.label "Allow remote images in HTML mail.">
>+<!ENTITY allowRemoteContent.label "Allow remote images.">
> <!ENTITY allowRemoteContent.accesskey "w">
>+<!ENTITY allowRemoteContent.tooltip "In HTML messages it is possible to embed images from remote sources. Opening such a message will open a connection to this external source. This allows to track the reading of the message. This check box will allow such external embedded images in HTML messages.">

Slightly better wording:

"In HTML messages it is possible to embed images from remote sources. Opening such a message will open a connection to this external source. This may allow tracking of the message being read. Checking this box will allow such external embedded images in HTML messages from this contact."

> <!ENTITY Custom4.label "Custom 4:">
> <!ENTITY Custom4.accesskey "4">
>-<!ENTITY Notes.box "Notes">
>-<!ENTITY Notes.accesskey "N">
>+

No need for a blank line here, as long as there's just one new line after the last > we're fine.

> <tabpanels id="abTabPanels" flex="1">
>
> <!-- ** Name Tab ** -->
>- <vbox index="name" flex="1">
>- <groupbox flex="1">
>- <caption label="&Name.box;"/>
>+ <tabpanel>
>+ <vbox>
>+ <hbox index="name" flex="1">
>+ <vbox>
> <vbox>

If you look at the existing code, <tabpanel> isn't necessary.

You also have an extra <vbox> in here (after the <hbox>) that you don't need.

Unfortunately, that means you still have an extra box element, so you need to re-indent the lines it encloses by two spaces.

Once you have done this, please attach the full patch and a diff -w patch.

>+ <vbox>
>+ <hbox>
>+ <spacer flex="1"/>
>+ <label control="WorkPhone" value="&WorkPhone.label;" accesskey="&WorkPhone.accesskey;" class="CardEditLabel"/>

As you're now touching these lines, please can you wrap accesskey onto the next line and align it with the start of control. You may need to wrap the class attribute.

ditto with other similar lines that you touch please.

> <menulist id="PreferMailFormatPopup">
> <menupopup>
> <!-- 0,1,2 come from nsIAbPreferMailFormat in nsIAbCard.idl -->
> <menuitem value="0" label="&Unknown.label;"/>
> <menuitem value="1" label="&PlainText.label;"/>
> <menuitem value="2" label="&HTML.label;"/>
> </menupopup>
> </menulist>
>+
>+ <checkbox id="allowRemoteContent" label="&allowRemoteContent.label;"
>+ accesskey="&allowRemoteContent.accesskey;"
>+ tooltiptext="&allowRemoteContent.tooltip;"/>
> </hbox>

This should be indented inline with the </menulist> above.

>+ <hbox class="AddressCardEditWidth">
>+ <textbox id="JobTitle" flex="1"/>
> <label control="Department" value="&Department.label;"
> accesskey="&Department.accesskey;" class="CardEditLabel"/>
>- <hbox class="CardEditWidth">
>- <textbox id="Department" flex="1"/>
>+ <textbox id="Department" flex="1"/>
> </hbox>

Again, need to fix the indentation.

Mostly this is looking fine, I've verified it all seems to work on SM (both themes) & TB on Mac, I'll check it on Linux once you update the patch.