Comment 91 for bug 115484

Comment on attachment 346231
Updated patch - removed the index attributes in abCardOverlay.xul

git-apply complained of 32 whitespace errors, of which at least 25 are new.
If you can't find them try /msg firebot review 346231 (note: it's designed to review C++ so some of its other quibbles aren't always appropriate.)

I thought the display looked a little crowded; the current version at least has those groupboxes that break up the dialog into manageable pieces.

>+<!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 may allow
>+tracking of the message being read. Checking this box will allow such external
>+embedded images in HTML messages from this contact." >
I don't know how common wrapping long entities is, but you've wrapped the text in the entity rather than the text in the file, which looks odd. Also speaking of whitespace errors you can lose the space before the >

> .CardEditWidth {
>- width: 25em;
>+ width: 21em;
Is it possible to change these widths to the equivalent in ch?
(This affects themes/platforms that use a bold font)

> .alignBoxWithFieldset {
> margin-left: 6px;
> margin-right: 5px;
> }
You don't use this any more. (Both comments apply for all themes of course.)

>+ <hbox id="nickNameContainer">
>+ <spacer flex="1"/>
>+ <label control="NickName" value="&NickName.label;"
>+ accesskey="&NickName.accesskey;" class="CardEditLabel"/>
>+ <hbox class="CardEditWidth">
>+ <textbox id="NickName" flex="1"/>
> </hbox>
> </hbox>
Just picking on this one because it was easy to extract on the diff, but this applies to most of the fields in this window.
Rather than setting the top margin in the CardEditLabel class, I think the outer hbox needs an align="center" so the labels line up properly with the textboxes.
If you wanted to, you could get rid of the <spacer> by using <label flex="1"> and adding text-align: right; to the CardEditLabel class.

>- <caption>
>- <label control="Notes" value="&Notes.box;" accesskey="&Notes.accesskey;"/>
>- </caption>
This caption was important as it labelled the Notes control...