63941 - New Card layout shouldn't assume high screen resolution

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect, P2)

Description

[Casey Perkins]

When clicking the New Card button in the Address Book, the dialog box that
appears is too tall for any resolution lower than 1024*768, not to mention being
poorly positioned on the screen.
I am running on a monitor that can't do better than 800*600 well. When I click
the New Card button, the box appears down and to the right every time. If I pull
the title bar up to the very top of the screen, I can barely see the OK and
Cancel buttons just peeking above the taskbar. For someone running a 640*480
resolution (God bless 'em), the OK and Cancel buttons would be impossible to
reach. For 800*600, they are just decidedly inconvenient.
It seems the better approach would be one or more of the following:
1) Make better use of the available space (there is a lot of wasted space
between the 3 main categories)
2) Either move some of the info to another tab (or new one), or provide a "More"
button that expands the box horizontally, or opens an additional dialog box.
3) Center the dialog box (horiz. and vert.) on the screen when it appears. This
part at least should be an easy hack.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Reporter, what build are you using?  There was a regression, recently fixed, in 
which the dialog in question was sized too large.

Comment 2

[Casey Perkins]

I am using the most recent build.

Comment 3

[Keyser Sose]

WORKSFORME
Platform: PC
OS: Linux 2.2.16
Mozilla Build: 2001010221


Reporter try with a new profile my bet is that will fix it.

Comment 4

[Casey Perkins]

I have a lot invested in the profile I've got. Besides, why should that fix it?
Aren't window designs, sizes and positioning specified in XUL files and C++
code? What does this have to do with a new profile?

Comment 5

[Casey Perkins]

I downloaded Mozilla onto another computer, created a new profile, opened up
Address Book and clicked New Card. Once again, the window sizing and positioning
were terrible for 800*600 and 640*480 resolutions. So creating a new profile
does not help, unfortunately.
Please take a look at the screenshot page I prepared in order to demonstrate
what I am talking about, and try it also on your own computer.
<a href="www.caseyperkins.com/newcard.html">www.caseyperkins.com/newcard.html
</a>
Thanks.

Comment 6

[Keyser Sose]

Marking NEW as per reporters comments.

Comment 7

[Boris Zbarsky [:bzbarsky]]

*** Bug 65997 has been marked as a duplicate of this bug. ***

Comment 8

[Boris Zbarsky [:bzbarsky]]

os/platform -> all, since the duplicate is reported on a Mac running System 9.

Comment 9

[scottputterman]

reassigning to chuang.

Comment 10

[fenella]

QA-assign-to fenella.

Comment 11

[Casey Perkins]

It seems like fixing would be a simple matter of x and y positioning and sizing.
I do some GUI programming (in Java) and that would be how I would do it there.
Can it be that different in C++/XUL?

As an example of centering the box in reference to the Mozilla window:
int height=450;
int width=300;
newCardBox.setSize(width,height);
int setLeft=(((mozWindow.getWidth()-width)/2)+mozWindow.getX());
int setRight=(((mozWindow.getHeight()-height)/2)+mozWindow.getY());
newCardBox.setLocation(setLeft,setRight);
newCardBox.show();

Yes, its Java pseudo-code, not C++, but the logic is good. Can't something
similar to this be implemented for the New Card box?

Comment 12

[Boris Zbarsky [:bzbarsky]]

*** Bug 84990 has been marked as a duplicate of this bug. ***

Comment 13

[Keyser Sose]

*** Bug 94805 has been marked as a duplicate of this bug. ***

Comment 14

[Keyser Sose]

*** Bug 97041 has been marked as a duplicate of this bug. ***

Comment 15

[Ninoschka Baca]

Marking nsbeta1 since running at a resolution of 640x480 makes the new card
dialog unusable. It's so large that the bottom half of the dialog cannot be
seen, there is no way to traverse to the bottom to select the OK/Cancel buttons.
Resolution 600x800 is also too large but I can atleast move the dialog to access
the OK/Cancel buttons.

Comment 16

[scottputterman]

reassigning to racham.

Comment 17

[Felix Miata]

Is this something hard to fix? I would think not. If not, please move this from
future to something sooner. There is simply no good reason for this to be so tall.

At 800 X 600 in Linux Mandrake KDE and in OS/2, the new card window is taller
than the space between the edge of the screen and the toolbar. In W2K at 800 X
600, it is only slightly less tall than that space, and opens way below 0,0,
leaving the OK & cancel buttons offscreen by default. In OS/2, it opens each
time with the titlebar way above the top of screen. See screenshot
http://bugzilla.mozilla.org/attachment.cgi?id=91249&action=view and bug 67913.

Simply right adjusting the currently left adjusted 'add to . . .' or the
'name,address,other' tabs would allow substantial vertical compaction without
losing [the excessive] whitespace between any entry fields.

The currently excessive whitespace means this could easily be made much shorter
than it currently is and accomodate minimum screen resolutions.

Comment 18

[Leston Buell]

Attachment: screenshot of new card in KDE, with KDE toolbar, at 800x600

Attaching screenshot of new card in KDE, with KDE toolbar, at 800x600.

Comment 19

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 20

[bulk88]

As of "Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040616" 
I still see this bug (no ok and no cancel) on Win2k with a 3 window high 
taskbar. This needs to be fixed and should be simple XUL I think.

Comment 21

[bulk88]

As of "Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040616" 
I still see this bug (no ok and no cancel) on Win2k 800x600 with a 3 window 
high taskbar. This needs to be fixed and should be simple XUL I think.

Comment 22

[Mike Kaply [:mkaply]]

*** Bug 165750 has been marked as a duplicate of this bug. ***

Comment 23

[Mark Banner (:standard8)]

*** Bug 313058 has been marked as a duplicate of this bug. ***

Comment 24

[Mark Banner (:standard8)]

*** Bug 293800 has been marked as a duplicate of this bug. ***

Comment 25

[Mark Banner (:standard8)]

*** Bug 315223 has been marked as a duplicate of this bug. ***

Comment 26

[Mark Banner (:standard8)]

*** Bug 317857 has been marked as a duplicate of this bug. ***

Comment 27

[Magnus Melin [:mkmelin]]

*** Bug 323507 has been marked as a duplicate of this bug. ***

Comment 28

[JohnMichael Williams]

This bug should be assigned a priority -- it affects the visually handicapped like me, and is a really ugly propblem on the "new card" pagelength problem reported as a "duplicate" to this report.

Comment 29

[Mark Banner (:standard8)]

*** Bug 344234 has been marked as a duplicate of this bug. ***

Comment 30

[Mark Banner (:standard8)]

*** Bug 349109 has been marked as a duplicate of this bug. ***

Comment 31

[Felix Miata]

Attachment: 633px tall new card screenshot 1024x768x96DPI (KDE)

~633px is the minimum required height @ 96 DPI

Comment 32

[Felix Miata]

Attachment: KDE fullscreen screenshot 800x600x96DPI

The problem is actually worse now than when attachment 112016 [details] was affixed. DPI is now max(96dpi,system setting), which means unless a user has taken the trouble via user.js or about:config to force Gecko to use ~84 DPI or less, the card can't fit in the available vertical space, even with the taskbar hidden.

Comment 34

[Jo Schaper]

 Ok, but is there a solution to this problem? Reassigning it to this bug number just moves it around. 

Comment 35

[jason]

There are a couple of solutions discussed in the Mozillazine forums:
http://forums.mozillazine.org/viewtopic.php?t=390988
http://forums.mozillazine.org/viewtopic.php?t=455604

These have to be manually added to a UserChrome.css file in each user profile though -- doesn't address the underlying problem.

Comment 37

[Wayne Mery (:wsmwk)]

bug 220443  has similar issue?

Comment 41

[Mark Banner (:standard8)]

Its about time we put a proper solution together for this. Requesting appropriate flags. Bryan, any suggestions?

Comment 42

[Steve & Lorraine]

Here's an unsolicited suggestion.
At the moment, Add Card contains three tabs (Contact - Address - Other). In these days of structured programming and OOD and all those other alphabet soups, how difficult would it be to distribute this over more tabs (and an obvious distribution would be Name - Internet - Phones - HomeAddress - WorkAddress - Other)? That would solve the problem at the expense of more tabs; which in my opinion is no price at all.

In case you are wondering why in 2008 I am using such a low-res screen, we bought in Singapore last week for the princely sum of the equivalent of about $US 300 an Asus Easy-Easy-Easy notebook with an 8.1" screen which has about 1024 X 600 resolution. Asus is predicting 2008-09 sales of tens of millions of these with Linux - and hence Thunderbird. Presumeably most of these people are going to want to add New Card.

Comment 43

[Dan Mosedale (:dmosedale, :dmose)]

It would be interesting to know how other addressbook UI (Evolution, Outlook, Outlook Express, etc.) addresses this problem (assuming they do)...

Comment 44

[Worcester12345]

URL doesn't work:
www.caseyperkins.com/newcard.html

Comment 45

[Steve & Lorraine]

Dan: I just tried this in Outlook Express. It organizes a "new contact" into five tabs. It has a variable size window with a minimum size that looks about 640x480, but no scroll bars, though. But that doesn't matter coz you finish it off with File -> Save and Close, which being in top LH corner you always see.

Comment 46

[Bryan Clark (DevTools PM) [:clarkbw]]

Ok, I'm collecting information on the dev wiki.  I've collected screenshots of systems I have access to: Evolution and Outlook.  I don't seem to have Outlook Express installed so I can't check that, if you have screenshots of other systems you could mail them to me and I'll post them as well.

Once we have a general understanding of how a number of other systems are tackling this problem we can start with some ascii art mockups, then move to more substance.  Much of this is going to change or shape the fields available [2] and how they are presented.

In general I we should be able to fit into about an 800x600 window (with tabs), however it will take a number of changes to make that happen.

[1] http://wiki.mozilla.org/MailNews:Address_Book_New_Card 
[2] http://wiki.mozilla.org/MailNews:Address_Book_Card_Fields

Comment 48

[Magnus Melin [:mkmelin]]

blocking‑thunderbird3+ given this can affect a lot more people with the rice of subnotebooks. Don't think this have to happen for 3.0a2.

Comment 51

[David Ascher (:davida)]

I think we'll get to this in Tb3, but we probably wouldn't block a release for it.   

Comment 52

[Worcester12345]

What would be really nice is to just have it go more in the horizontal direction and eliminate all the tabs. It would be FAR, FAR easier to have it all in one place.

Comment 53

[Friedrich Beckmann : Fritz]

Attachment: Change the arrangement of the new card entry to allow 800x600 resolution (2.x branch)

This is a patch for the abCardOverlay.xul file which rearranges the layout of the new card entry. It makes it wider and less tall in order to fit in 800x600 resolution.

Comment 54

[Friedrich Beckmann : Fritz]

Attachment: Picture of the rearranged layout from the previous patch with 800x600 resolution

This is a picture of the new arrangement of the boxes in the new card box with 800x600 resolution.

Comment 55

[Friedrich Beckmann : Fritz]

Just to note: The patch is vs. Thunderbird version 2.0.0.16.

Comment 56

[Friedrich Beckmann : Fritz]

Attachment: This patch changes the order of the boxes for a new card. (vs 1.50)

This patch is versus the latest 1.50 CVS version which is also the latest comm-central one. The previous patch is versus the version in 2.0.0.16.
The patch moves the "Phone Number" section to the right of "Name" and "Internet" in the first "Contact" tab. In the second tab, the two boxes are also placed side by side.

Comment 57

[Friedrich Beckmann : Fritz]

Are there any opinions on this layout: https://bugzilla.mozilla.org/attachment.cgi?id=338651?

Comment 58

[Mark Banner (:standard8)]

Comment on attachment 338649 [details] [diff] [review]
Change the arrangement of the new card entry to allow 800x600 resolution (2.x branch)

I know you did this originally against the 2.x branch, and updated for trunk. However we won't accept changes like this on the 2.x branch because that is for security and stability only. So r- this patch just because of that. I'm looking at the trunk one separately.

Comment 59

[Mark Banner (:standard8)]

Comment on attachment 338728 [details] [diff] [review]
This patch changes the order of the boxes for a new card. (vs 1.50)

We're now using Mercurial as our source control tool, so please can any future patches you supply use that please (see http://developer.mozilla.org/en/docs/comm-central for more info).

Although on Windows the layout fits within 800x600, on my mac, the dialog is 450x700 - obviously a bit too wide.

The fields on the notes page are also all to the right, which isn't good.

I also wonder if its possible to move the address book selector down alongside the ok/cancel buttons.

I think this may be a good start, but Bryan, our user experience expert, should also take a look at this, so I'll request ui-review from him.

Comment 60

[Bryan Clark (DevTools PM) [:clarkbw]]

I need to see how the changes effect the other tabs as well.  But overall the new arrangement leaves a lot of oddly stacked empty space.  Perhaps we should be splitting things up differently?

Comment 61

[Worcester12345]

(In reply to comment #60)
> I need to see how the changes effect the other tabs as well.  But overall the
> new arrangement leaves a lot of oddly stacked empty space.  Perhaps we should
> be splitting things up differently?

(In reply to comment #52)
> What would be really nice is to just have it go more in the horizontal
> direction and eliminate all the tabs. It would be FAR, FAR easier to have it
> all in one place.

Comment 62

[Friedrich Beckmann : Fritz]

Attachment: New Address Card Layout patch with 4 Tabs

Hi, thank you for your comments! In the meantime I returned my company laptop so it took a little while to respond. The new changes are versus the comm-central mercurial tree. I moved the home address and the birthday to a new tab. I will also add pictures of the 4 tabs for ui review. The size of the dialog box on windows is now 552x350. I guess the previous patch did not include the birthday changes... The patch changes the following files: abCardOverlay.xul, abCardOverlay.dtd and cardDialog.css. So it will requires changes in the language sets.

Comment 63

[Friedrich Beckmann : Fritz]

Attachment: Picture of the 1st tab for the 552x350 layout patch

This is a picture of the first tab for the new layout with the patched 552x350 version.

Comment 64

[Friedrich Beckmann : Fritz]

Attachment: Picture of the 2nd tab for th 552x350 layout patch 342082

Comment 65

[Friedrich Beckmann : Fritz]

Attachment: Picture of the 3rd tab for the 552x350 layout patch 342082

Comment 66

[Friedrich Beckmann : Fritz]

Attachment: Picture of the 4th tab for the 552x350 layout patch 342082

Comment 67

[Neil Marshall]

If you change this line in the birthday section:
<hbox class="AddressCardEditWidth">
to
<hbox class="AddressCardEditWidth" align="center">

The "or" should align itself a bit better.

Comment 68

[Worcester12345]

What we need is FEWER tabs, not MORE tabs! Better still, eliminate ALL the tabs and put everything in one place. Why make it harder?

Comment 69

[Friedrich Beckmann : Fritz]

(In reply to comment #67)
> If you change this line in the birthday section:
> <hbox class="AddressCardEditWidth"> to
> <hbox class="AddressCardEditWidth" align="center">
> The "or" should align itself a bit better.

I changed it in my place for the moment. Good idea.

(In reply to comment #68)
> What we need is FEWER tabs, not MORE tabs! Better still, eliminate ALL the tabs
> and put everything in one place. Why make it harder?

The problem I want to solve is to make the card dialog fit to lower resolution displays. The original version is too high to fit to the Acer Aspire One which has 1024x600 resolution. You cannot even see the "Ok" and "Cancel" buttons so this is killing the whole address book on these devices. Putting everything in one place and eliminating all the tabs is probably not going in that direction. So I put the most important things in the first tab (Name, Email, Phone). The second tab is the private/home address and the third tab gives the business address. I split the home and business address to save some space. Do you think that removing the tabs will make the whole dialog smaller?

Comment 70

[Friedrich Beckmann : Fritz]

Today there was a short discussion on IRC between standard8, asuth and me regarding the address card edit. There is the idea that the address entries could also be edited "inline" in the "preview pane". At the moment the display of the preview pane can be disabled. Result of the the discussion is that the "inline-editing" is independent of the card dialog editing. It could be done regardless of this patch.

Comment 71

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 342083 [details]
Picture of the 1st tab for the 552x350 layout patch

I don't really like how the "Allow remote images" lines up here.  But it's not too big a deal and I'm hopeful it could be removed (bug 457296)  Otherwise things look fine.

Comment 72

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 342084 [details]
Picture of the 2nd tab for th 552x350 layout patch 342082

The City text box seems larger than it needs to be, however I don't know how you'd arrange the extra space.  It's better to use it up like this than to have the empty block in the middle.

Comment 73

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 342085 [details]
Picture of the 3rd tab for the 552x350 layout patch 342082

Looks good overall

Comment 74

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 342086 [details]
Picture of the 4th tab for the 552x350 layout patch 342082

Not a pretty tab, but nothing to do with this bug.  Looks good overall.

Comment 75

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 342082 [details] [diff] [review]
New Address Card Layout patch with 4 Tabs

I already gave the plus for all the screenshots, as usual I expect the other reviewer to look at the code itself.

Comment 76

[Friedrich Beckmann : Fritz]

(In reply to comment #75)
> (From update of attachment 342082 [details] [diff] [review])
> I already gave the plus for all the screenshots, as usual I expect the other
> reviewer to look at the code itself.
thank you for the review!
(In reply to comment #71)
> (From update of attachment 342083 [details])
> I don't really like how the "Allow remote images" lines up here.  But it's not
> too big a deal and I'm hopeful it could be removed (bug 457296)  Otherwise
> things look fine.
I have it here in this tab because this is the "email" section. I do not know a better solution. 
(In reply to comment #72)
> (From update of attachment 342084 [details])
> The City text box seems larger than it needs to be, however I don't know how
> you'd arrange the extra space.  It's better to use it up like this than to have
> the empty block in the middle.
I was more concerned about the smaller department and title box which I put in one line. As you mention in your comment, the width is just there to make the city box that wide. 

Again, thanks for the review!

Friedrich

Comment 77

[Mark Banner (:standard8)]

Comment on attachment 342082 [details] [diff] [review]
New Address Card Layout patch with 4 Tabs

I'll do a more detailed review later, but my first comment is that abCardOverlay.xul is shared between SeaMonkey and Thunderbird.

So would you mind porting your changes to abCardOverlay.dtd and cardDialog.css to the files in the mail/ directory? (i.e. Thunderbirds).

Comment 78

[Friedrich Beckmann : Fritz]

Attachment: New Card Layout - Patch now with seamonkey and thunderbird plus themes

This is an updated patch which modifies 

mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
mail/themes/pinstripe/mail/addrbook/cardDialog.css
mail/themes/qute/mail/addrbook/cardDialog.css
mailnews/addrbook/resources/content/abCardOverlay.xul
suite/locales/en-US/chrome/mailnews/addressbook/abCardOverlay.dtd
suite/themes/classic/messenger/addressbook/cardDialog.css
suite/themes/modern/messenger/addressbook/cardDialog.css

in order to not break the seamonkey code and the different themes. However, this is done in blind flight as I do not have a build environment. I only changed the code based on a nightly snapshot and copied it back to a mercurial source tree. 
I also added the small change regarding the "or" positioning in the birthday.

Comment 79

[Mark Banner (:standard8)]

Comment on attachment 344934 [details] [diff] [review]
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.

Comment 80

[Friedrich Beckmann : Fritz]

Attachment: New Card layout - Updated patch including the comments from Standard8

This patch includes the comments from standard8. I changed the indention and removed useless tabs and boxes. There was no difference in the patch size between -U8 and -U8 -w, so I only included the -U8 diff. The files

mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
suite/locales/en-US/chrome/mailnews/addressbook/abCardOverlay.dtd

are identical. However there is a (small) difference in the license text in the original code. So I had to choose one to make the files really identical. I took the one from the template that standard8 mentionend. Thanks for the review!

Comment 81

[Friedrich Beckmann : Fritz]

(In reply to comment #79)
> >- 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/
I reversed this. Now it complies to the template. 
> 
> 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."
I took your wording. 
> >-<!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.
Done. 
> 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.
I removed the <tabpanel> and the useless <vboxes> and <hboxes>. The diff -U8 -w showed no difference to the diff -U8, so I only added the full patch. 
> 
> 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.
I did the wraps to have less line length overall. 
> 
> 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.
Thanks for review!

Comment 82

[Mark Banner (:standard8)]

(In reply to comment #81)
> > Once you have done this, please attach the full patch and a diff -w patch.
> I removed the <tabpanel> and the useless <vboxes> and <hboxes>. The diff -U8 -w
> showed no difference to the diff -U8, so I only added the full patch. 

Unfortunately diff -w doesn't always work in Mercurial, see https://developer.mozilla.org/en/Mercurial_FAQ#How_do_I.c2.a0get_a_diff_-w.3f for more info.

I'm just reviewing the patch though, so don't worry about adding a diff -w straight away.

Comment 83

[Mark Banner (:standard8)]

Comment on attachment 345282 [details] [diff] [review]
New Card layout - Updated patch including the comments from Standard8

>     <tabpanels id="abTabPanels" flex="1">
>+      <!-- ** Name Tab ** -->
>+      <!-- The following vbox contains two hboxes top: Name/Email/Phonenumber bottom: Email prefs. -->
>+      <vbox>
>+        <!-- This hbox contains two vboxes left: Name/Email, right: Phonenumbers -->
>+        <hbox index="name" flex="1">

I think the index/flex attributes should be on the <vbox> element above.

I think the business vbox also needs an index.
 
Otherwise r=me with those fixed. You'll need an sr for this patch as well.

Comment 84

[Friedrich Beckmann : Fritz]

Attachment: Updated patch - removed the index attributes in abCardOverlay.xul

I removed the index attributes as they are useless. I added ids to the for tabs. They may be useful for future overlays.

Comment 85

[[e-mail]]

Comment on attachment 346231 [details] [diff] [review]
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...

Comment 86

[Friedrich Beckmann : Fritz]

Attachment: Updated patch - included comments from neil

This patch includes changes based on the comments from Neil.
* I removed the cardEditLabel class altogether. The alignment of the labels is done via the align="center" attributes.
* Changed the css unit from em to ch
* Added the notes label
Thanks for the review!

Comment 87

[Friedrich Beckmann : Fritz]

Attachment: Updated patch - included comments from neil and whitespace

This patch includes changes based on the comments from Neil.
* I removed the cardEditLabel class altogether. The alignment of the labels is
done via the align="center" attributes.
* Changed the css unit from em to ch
* Added the notes label
* Reduced the whitespace and longline style bugs
Thanks for the review!

Comment 88

[Friedrich Beckmann : Fritz]

(In reply to comment #85)
> (From update of attachment 346231 [details] [diff] [review])
> 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.)
Thanks for the hint. I removed the whitespace and longline errors but there are 4 left in the current patch. I have changed it locally here for the moment. 
> 
> I thought the display looked a little crowded; the current version at least has
> those groupboxes that break up the dialog into manageable pieces.
I removed the groupboxes on purpose because they consume quite some space. Because I address is now split in home and business, that one is obsolete. For the phone numbers and Emails on the first tab, I compared it to Outlook where they also have it like this. So in the end I removed them only because of the space. 
> >+<!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 >
I split it a little differently to have no line longer than 80 but I left it essentially. 
> 
> > .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)
Done.
> 
> > .alignBoxWithFieldset {
> >   margin-left: 6px;
> >   margin-right: 5px;
> > }
> You don't use this any more. (Both comments apply for all themes of course.)
Removed it.
> 
> >+            <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.
I removed the CardEditLabel class and did the alignment with the attribute. 
> 
> >-          <caption>
> >-            <label control="Notes" value="&Notes.box;" accesskey="&Notes.accesskey;"/>
> >-          </caption>
> This caption was important as it labelled the Notes control...
I added the label again. 

Thanks for the review!

Comment 89

[Friedrich Beckmann : Fritz]

Attachment: Updated patch - modified the accesskeys

Modified the access keys and removed the remaining 4 whitespace errors. 
Should be possible to navigate throught the dialog with keys.

Comment 90

[[e-mail]]

Comment on attachment 346269 [details] [diff] [review]
Updated patch - modified the accesskeys

> <!ENTITY WorkZipCode.label              "ZIP/Postal Code:">
>-<!ENTITY WorkZipCode.accesskey          "P">
>+<!ENTITY WorkZipCode.accesskey          "z">
Nit: Prefer to match the case of the accesskey to the label's character.

Comment 91

[Friedrich Beckmann : Fritz]

Attachment: Updated patch - now with capital access keys

Just changed some access keys to capital letters.

Comment 92

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/4ae62e9a43c3

Thanks for all your work on this Fritz.

Comment 93

[Friedrich Beckmann : Fritz]

(In reply to comment #92)
> Checked in: http://hg.mozilla.org/comm-central/rev/4ae62e9a43c3
> 
> Thanks for all your work on this Fritz.
Thank you, Clark, Neil and the others for your reviews and help! 
Nice environment here.

Comment 94

[Rimas Kudelis [use needinfo to ping]]

<!ENTITY Birthday.label                 "Birthday:">
<!ENTITY Birthday.accesskey             "B">
<!ENTITY Year.emptytext                 "Year">
<!ENTITY Or.value                       "or">
<!ENTITY Age.emptytext                  "Age">


I suggest to move these to the "Private" section to reflect the situation in UI.

Comment 95

[Friedrich Beckmann : Fritz]

Attachment: Move birthday section in abCardOverlay.dtd as proposed in Comment #94

This is a small patch to rearrange the entities in the abCardOverlay.dtd file. The birthday section is moved from the "Other Tab" to the "Private Tab" entries to be aligned with the display in the user interface address card dialog. This patch does not change behaviour.

Comment 96

[Mark Banner (:standard8)]

Comment on attachment 346645 [details] [diff] [review]
Move birthday section in abCardOverlay.dtd as proposed in Comment #94 

I'm not sure its absolutely vital that we do this. However, seeing as the patch is here and they are reasonably well organised in the file, we can take it.

Comment 97

[Friedrich Beckmann : Fritz]

(In reply to comment #96)
> (From update of attachment 346645 [details] [diff] [review])
> I'm not sure its absolutely vital that we do this. However, seeing as the patch
> is here and they are reasonably well organised in the file, we can take it.
The changes has been checked in: http://hg.mozilla.org/comm-central/rev/eb6e30c22d74
Thanks! Friedrich

Comment 100

[Brenda Make]

The bug is still in:
thunderbird-2.0.0.19.en-US.win32.installer.exe

I guess the nightly's aren't.

The bugfix is in the 3.x series which isn't compatible with lightning.
Does anyone know when the bugfix will make into a nightly?

BTW, Thanks for fixing the bug : )

Comment 101

[Mark Banner (:standard8)]

(In reply to comment #100)
> The bug is still in:
> thunderbird-2.0.0.19.en-US.win32.installer.exe

Patches like this will never make it into released major versions because the updates are for security and stability updates.

> The bugfix is in the 3.x series which isn't compatible with lightning.
> Does anyone know when the bugfix will make into a nightly?

If you mean a 2.x nightly then the answer is never. If you mean a 3.x nightly then it is already there and there are lightning versions available for the 3.x nightlies.

Comment 105

[Frank Parkinson]

All the discussions regarding what fields go where - surely making the Edit Card scrollable is the best answer!!