New Card layout shouldn't assume high screen resolution

RESOLVED FIXED in Thunderbird 3.0b1

Status

defect
P2
normal
RESOLVED FIXED
19 years ago
9 years ago

People

(Reporter: caseyperkins, Assigned: friedrich.beckmann)

Tracking

(Depends on 1 bug, {access, helpwanted, uiwanted})

Dependency tree / graph
Bug Flags:
blocking-thunderbird3 -
wanted-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: nab-visual)

Attachments

(9 attachments, 10 obsolete attachments)

46.74 KB, image/png
Details
26.42 KB, image/png
Details
80.24 KB, image/png
Details
14.31 KB, image/png
clarkbw
: ui-review+
Details
13.22 KB, image/png
clarkbw
: ui-review+
Details
12.71 KB, image/png
clarkbw
: ui-review+
Details
11.42 KB, image/png
clarkbw
: ui-review+
Details
44.74 KB, patch
Details | Diff | Splinter Review
4.56 KB, patch
standard8
: review+
Details | Diff | Splinter Review
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.
Reporter, what build are you using?  There was a regression, recently fixed, in 
which the dialog in question was sized too large.
I am using the most recent build.
WORKSFORME
Platform: PC
OS: Linux 2.2.16
Mozilla Build: 2001010221


Reporter try with a new profile my bet is that will fix it.
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?
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.
Marking NEW as per reporters comments.
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: esther → pmock
*** Bug 65997 has been marked as a duplicate of this bug. ***
os/platform -> all, since the duplicate is reported on a Mac running System 9.
OS: Windows 98 → All
Hardware: PC → All
reassigning to chuang.
Assignee: putterman → chuang
QA-assign-to fenella.
QA Contact: pmock → fenella
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?
*** Bug 84990 has been marked as a duplicate of this bug. ***
QA Contact: fenella → nbaca
*** Bug 94805 has been marked as a duplicate of this bug. ***
*** Bug 97041 has been marked as a duplicate of this bug. ***
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.
Keywords: nsbeta1
Whiteboard: nbaca-visual
Whiteboard: nbaca-visual → nb-visual
Whiteboard: nb-visual → nab-visual
reassigning to racham.
Assignee: chuang → racham
Status: NEW → ASSIGNED
Keywords: nsbeta1nsbeta1-
Target Milestone: --- → Future
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.
Attaching screenshot of new card in KDE, with KDE toolbar, at 800x600.
mass re-assign.
Assignee: racham → sspitzer
Status: ASSIGNED → NEW
Product: Browser → Seamonkey
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.
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.
*** Bug 165750 has been marked as a duplicate of this bug. ***
Assignee: sspitzer → mail
Assignee: mail → nobody
Component: Address Book → MailNews: Address Book
Product: Mozilla Application Suite → Core
QA Contact: nbaca → addressbook
Target Milestone: Future → ---
*** Bug 313058 has been marked as a duplicate of this bug. ***
*** Bug 293800 has been marked as a duplicate of this bug. ***
Blocks: 259228
*** Bug 315223 has been marked as a duplicate of this bug. ***
*** Bug 317857 has been marked as a duplicate of this bug. ***
Keywords: helpwanted
*** Bug 323507 has been marked as a duplicate of this bug. ***
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.
*** Bug 344234 has been marked as a duplicate of this bug. ***
*** Bug 349109 has been marked as a duplicate of this bug. ***
Keywords: access
~633px is the minimum required height @ 96 DPI
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.
Duplicate of this bug: 368850
 Ok, but is there a solution to this problem? Reassigning it to this bug number just moves it around. 

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.
Duplicate of this bug: 381437
bug 220443  has similar issue?
Duplicate of this bug: 366202
Duplicate of this bug: 354165
Depends on: 329918
Duplicate of this bug: 432788
Its about time we put a proper solution together for this. Requesting appropriate flags. Bryan, any suggestions?
Flags: wanted-thunderbird3.0a2?
Flags: blocking-thunderbird3?
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.
It would be interesting to know how other addressbook UI (Evolution, Outlook, Outlook Express, etc.) addresses this problem (assuming they do)...
Keywords: uiwanted
URL doesn't work:
www.caseyperkins.com/newcard.html
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.
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
Duplicate of this bug: 433085
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.
Flags: wanted-thunderbird3.0a2?
Flags: blocking-thunderbird3?
Flags: blocking-thunderbird3+
Duplicate of this bug: 444584
Duplicate of this bug: 445398
Product: Core → MailNews Core
I think we'll get to this in Tb3, but we probably wouldn't block a release for it.   
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Priority: -- → P2
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.
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.
This is a picture of the new arrangement of the boxes in the new card box with 800x600 resolution.
Attachment #338649 - Flags: review?(bugzilla)
Assignee: nobody → friedrich.beckmann
Just to note: The patch is vs. Thunderbird version 2.0.0.16.
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.
Attachment #338728 - Flags: review?
Attachment #338728 - Flags: review? → review?(bugzilla)
Status: NEW → ASSIGNED
Are there any opinions on this layout: https://bugzilla.mozilla.org/attachment.cgi?id=338651?
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.
Attachment #338649 - Attachment description: Change the arrangement of the new card entry to allow 800x600 resolution → Change the arrangement of the new card entry to allow 800x600 resolution (2.x branch)
Attachment #338649 - Flags: review?(bugzilla) → review-
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.
Attachment #338728 - Flags: ui-review?(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?
(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.
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.
Attachment #338649 - Attachment is obsolete: true
Attachment #338651 - Attachment is obsolete: true
Attachment #338728 - Attachment is obsolete: true
Attachment #342082 - Flags: ui-review?(clarkbw)
Attachment #342082 - Flags: review?
Attachment #338728 - Flags: ui-review?(clarkbw)
Attachment #338728 - Flags: review?(bugzilla)
This is a picture of the first tab for the new layout with the patched 552x350 version.
Attachment #342083 - Flags: ui-review?(clarkbw)
Attachment #342085 - Flags: ui-review?(clarkbw)
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.
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?
(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?
Attachment #342082 - Flags: ui-review?(clarkbw) → ui-review?(bugmail)
Attachment #342082 - Flags: ui-review?(bugmail) → ui-review?(clarkbw)
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 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.
Attachment #342083 - Flags: ui-review?(clarkbw) → ui-review+
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.
Attachment #342084 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 342085 [details]
Picture of the 3rd tab for the 552x350 layout patch 342082

Looks good overall
Attachment #342085 - Flags: ui-review?(clarkbw) → ui-review+
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.
Attachment #342086 - Flags: ui-review?(clarkbw) → ui-review+
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.
Attachment #342082 - Flags: ui-review?(clarkbw) → ui-review+
(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 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).
Attachment #342082 - Flags: review? → review?(bugzilla)
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.
Attachment #342082 - Attachment is obsolete: true
Attachment #342082 - Flags: review?(bugzilla)
Attachment #344934 - Flags: review?(bugzilla)
Attachment #344934 - Attachment is patch: true
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.
Attachment #344934 - Flags: review?(bugzilla) → review-
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!
Attachment #344934 - Attachment is obsolete: true
Attachment #345282 - Flags: review?(bugzilla)
(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!
(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.
Attachment #345282 - Flags: review?(bugzilla) → review+
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.
I removed the index attributes as they are useless. I added ids to the for tabs. They may be useful for future overlays.
Attachment #345282 - Attachment is obsolete: true
Attachment #346231 - Flags: superreview?
Attachment #346231 - Flags: review?
Attachment #346231 - Flags: superreview? → superreview?(neil)
Attachment #346231 - Flags: review? → review+
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...
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!
Attachment #346231 - Attachment is obsolete: true
Attachment #346254 - Flags: superreview?(neil)
Attachment #346231 - Flags: superreview?(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
* Reduced the whitespace and longline style bugs
Thanks for the review!
Attachment #346254 - Attachment is obsolete: true
Attachment #346259 - Flags: superreview?(neil)
Attachment #346254 - Flags: superreview?(neil)
(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!
Modified the access keys and removed the remaining 4 whitespace errors. 
Should be possible to navigate throught the dialog with keys.
Attachment #346259 - Attachment is obsolete: true
Attachment #346259 - Flags: superreview?(neil)
Attachment #346269 - Flags: superreview?(neil)
Attachment #346269 - Flags: superreview?(neil) → superreview+
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.
Just changed some access keys to capital letters.
Attachment #346269 - Attachment is obsolete: true
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/4ae62e9a43c3

Thanks for all your work on this Fritz.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b1
(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.
<!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.
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.
Attachment #346645 - Flags: review?(bugzilla)
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.
Attachment #346645 - Flags: review?(bugzilla) → review+
(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
Depends on: 469090
Duplicate of this bug: 473399
Duplicate of this bug: 474385
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 : )
(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.
Duplicate of this bug: 479149
Duplicate of this bug: 483017
Duplicate of this bug: 483094
Blocks: 458590
Depends on: 485516
No longer depends on: 485516
All the discussions regarding what fields go where - surely making the Edit Card scrollable is the best answer!!
Duplicate of this bug: 505627
Duplicate of this bug: 515333
Duplicate of this bug: 520502
Duplicate of this bug: 523738
Duplicate of this bug: 540240
Duplicate of this bug: 548394
You need to log in before you can comment on or make changes to this bug.