315928 - "Copy Address" on mailto: link with multiple email addresses on captures one address

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Bug Reporter]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051107 Camino/1.0b1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051107 Camino/1.0b1

When viewing a webpage that contains a mailto: link of the type:
<a href="mailto:[e-mail],[e-mail],[e-mail],[e-mail]">E-MAIL US</a>, control-clicking the email link correctly provides the "Copy Address" option, but only copies the first address.




Reproducible: Always

Steps to Reproduce:
1. Control-click email address link.
2. Select "Copy Address".
3. Switch to a text editor and Paste the clipboard contents.

Actual Results:  
Only the first emaill address appears.

Expected Results:  
In other browsers (e.g., Firefox, Mozilla), the full list of addresses is correctly copied.


This bug appears to be Camino-specific.  Other Mozilla-based browsers behave correctly.

I have not been able to test whether the "Add to Address Book" option has the same issue, but I suspect it does.

Comment 1

[Bug Reporter]

Attachment: A small webpage with a multi-address mailto link.

Comment 2

[Samuel Sidler (old account; do not CC)]

Confirmed.

Comment 3

[Chris Lawson (gone)]

This was by design of the person who wrote the code. From BrowserWindowController.mm:

    // mailto: links can contain arguments (after '?') and/or multiple e-mail
    // addresses (comma separated). We want just the first e-mail address.

Dunno who wrote that, but do we want to contine with that policy, or do we want to fix the multiple e-mail issue? (I don't think we want to try to do anything with the arguments after the '?' at all.)

Also, copyAddressToClipboard (the IB action called by the menu item) uses getMailAddressFromContextMenuLinkNode, which is also used by addToAddressBook, to get the e-mail address from the link. How do we want to handle mailto links with multiple addresses when adding to the Address Book? Separate cards? A group card?

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

That was Bruce in bug 166958.

Also note that that the address book link (in normal circumstances) changes to "Show foo in Address Book", or something like that, if the email address is already in Address Book, which is perhaps part of the reason we only deal with the first address?

Comment 5

[Bug Reporter]

I agree with not handling arguments after the '?' (although, this should be a documented security feature).

Just about every browser handles multiple email addresses.  In fact Safari goes further and changes the control-click menu to read "Copy email addresses" (i.e., plural) when it detects multiple email addresses.  Perhaps this should be a Camino feature as well.

In terms of address book cards, I am uncertain of the normal behaviour of other browsers.  I could see an argument for both using separate cards and for using one single card with multiple addresses (i.e., a card for a group of people).  Personally, I think spawning multiple cards makes more sense.

Comment 6

[Bruce Davidson]

This was a deliberate simplification on my part to get the functionality in in the first place. I'm happy to take on extending it now to cope with multiple e-mail addresses - taking bug.

Comment 7

[Bruce Davidson]

I now have a patch that fixes this bug. When you click on a mailto: link you will now get seperate "Add abc@xyz to Address Book" (or "Open Joe Public in Address Book" if the e-maila address is already known) items for each e-mail address in the mailto: link. The "Copy Address" menu item is also appropriately pluralised.

Comment 8

[Bruce Davidson]

Attachment: Updated localized strings file

New version of Localizable.strings - adds plural form "Copy Addresses" and now displays the e-mail address in the "Add abc@xyz to Address Book" item, so that different entries can be distinguished.

Comment 9

[Bruce Davidson]

Attachment: BrowserWindow.nib (tar/gzipped)

Comment 10

[Bruce Davidson]

Attachment: Initial patch

Initial patch. Requesting review from kreeger.

Comment 11

[Nick Kreeger]

Comment on attachment 211770 [details] [diff] [review]
Initial patch

+  IBOutlet NSMenuItem*          mCopyEmailAddressItem;  /* mailto: link item */
+  IBOutlet NSMenuItem*          mCopyEmailAddressItem2; /* image mailto: link item */


Its ok to just use a '//' comment here since this isn't a C.
I don't like the naming convention here, maybe something like |mImageCopyEmailAddressItem| or something that goes along with what that param does.

Also could you be a little more clear in the comments, I am not to sure what "image mailto: link item" means?

+- (NSString*)getMailAddressFromContextMenuLinkNode:(unsigned)index
+{
+  NSArray* mailAddresses = [self mailAddressesInContextMenuLinkNode];
+  
+  if ( index < [mailAddresses count] )
+    return [mailAddresses objectAtIndex:index];
+  
+  return nil;
+}


More of a style thing here, can you get rid of the extra spaces in the if statement by the parens?

-    return linkTargetText;
+    return [linkTargetText componentsSeparatedByString:@","];
   }
-  
-  return nil;
+
+  return [NSArray array];
 }


Yikes, instead of returning a blank array, lets return nil like you did for the other methods.

+- (NSMenuItem*)prepareAddToAddressBookMenuItem:(NSString*)emailAddress
 {
-  if ([[ABAddressBook sharedAddressBook] emailAddressExistsInAddressBook:emailAddress]) {
-    NSString* realName = [[ABAddressBook sharedAddressBook] getRealNameForEmailAddress:[self getMailAddressFromContextMenuLinkNode]];
-    [addToAddressBookItem setTitle:[NSString stringWithFormat:NSLocalizedString(@"Open %@ in Address Book", @""), realName != nil ? realName : @""]];
+  if ( [emailAddress length] > 0 ) {
+    NSMenuItem* addToAddressBookItem = [[NSMenuItem alloc] init];
+    if ([[ABAddressBook sharedAddressBook] emailAddressExistsInAddressBook:emailAddress]) {
+      NSString* realName = [[ABAddressBook sharedAddressBook] getRealNameForEmailAddress:emailAddress];
+      [addToAddressBookItem setTitle:[NSString stringWithFormat:NSLocalizedString(@"Open %@ in Address Book", @""), realName != nil ? realName : @""]];
+    } else {
+      [addToAddressBookItem setTitle:[NSString stringWithFormat:NSLocalizedString(@"Add %@ to Address Book", @""), emailAddress]];
+    }
     [addToAddressBookItem setEnabled:YES];
-  } else {
-    [addToAddressBookItem setTitle:NSLocalizedString(@"Add to Address Book", @"")];
-    [addToAddressBookItem setEnabled:([emailAddress length] > 0) ];
+    [addToAddressBookItem setRepresentedObject:emailAddress];
+    [addToAddressBookItem setAction:@selector(addToAddressBook:)];
+  
+    return [addToAddressBookItem autorelease];
   }
+  
+  return nil;
 }

Could you space out your if/else statements, it makes this much easier to look at

+  NSArray* emailAddresses = nil;
+  unsigned numEmailAddresses = 0;
+
   if ((contextMenuFlags & nsIContextMenuListener::CONTEXT_LINK) != 0)
   {
-    NSString* emailAddress = [self getMailAddressFromContextMenuLinkNode];
-    
+    emailAddresses    = [self mailAddressesInContextMenuLinkNode];
+    numEmailAddresses = [emailAddresses count];

Since we want to return 'nil' on |mailAddressesInContextMenuLinkNode|, we should do a little checking before calling for size


     if ((contextMenuFlags & nsIContextMenuListener::CONTEXT_IMAGE) != 0) {
-      if (emailAddress) {
-        [self prepareAddToAddressBookMenuItem:mAddToAddressBook2 address:emailAddress];
+      if ( numEmailAddresses > 0 )
         menuPrototype = mImageMailToLinkMenu;
-      } 
       else
         menuPrototype = mImageLinkMenu;
     } 
     else {
-      if (emailAddress) {
-        [self prepareAddToAddressBookMenuItem:mAddToAddressBook address:emailAddress];
+      if ( numEmailAddresses > 0 )
         menuPrototype = mMailToLinkMenu;
-      } 
       else
         menuPrototype = mLinkMenu;
     }
   }

Watch your spacing inside the if statements here too.

   
+  // Add items to add/open each e-mail address in a mailto: link and
+  // change "address" to the plural form if necessary
+  if ( numEmailAddresses > 0 ) {
+    for ( unsigned i = 0; i < numEmailAddresses; ++i ) {
+      NSMenuItem* item = [self prepareAddToAddressBookMenuItem:[emailAddresses objectAtIndex:i]];
+      if ( item )
+        [result insertItem:item atIndex:1];
+    }
+    if ( numEmailAddresses > 1 )
+      [[result itemWithTarget:self andAction:@selector(copyAddressToClipboard:)] setTitle:NSLocalizedString( @"Copy Addresses", @"" )];
+  }
+  

Again spacings..

I didn't test the patch... I will give this a good test the next go around.

Comment 12

[Bruce Davidson]

> I don't like the naming convention here, maybe something like
> |mImageCopyEmailAddressItem| or something that goes 

Infact we don't need these any more at all, so I've taken them out altogether.

> More of a style thing here, can you get rid of the extra spaces in the if
> statement by the parens?

Readability of all if statements reduced as requested :-)

> Yikes, instead of returning a blank array, lets return nil like you did for the
> other methods.
[...]
> Since we want to return 'nil' on |mailAddressesInContextMenuLinkNode|, we
> should do a little checking before calling for size

But this is exactly why we return an empty array rather than nil. That way there's no risk of someone calling the function and one day forgetting to check the return value for being nil. (I know that you can call methods on nil in ObjC, but its poor form to rely on that.)

I'll change it if you insist, but I believe that returning an empty array is better style.

> I didn't test the patch... I will give this a good test the next go around.

Updated patch and NIB files being attached in a moment.

Comment 13

[Bruce Davidson]

Attachment: Updated BrowserWindow.nib (tar/gzipped)

Comment 14

[Bruce Davidson]

Attachment: Updated patch addressing review comments

All review comments addressed, except as noted in comment #12.

Comment 15

[Nick Kreeger]

> Yikes, instead of returning a blank array, lets return nil like you did for the
> other methods.
[...]
> Since we want to return 'nil' on |mailAddressesInContextMenuLinkNode|, we
> should do a little checking before calling for size

But this is exactly why we return an empty array rather than nil. That way
there's no risk of someone calling the function and one day forgetting to check
the return value for being nil. (I know that you can call methods on nil in
ObjC, but its poor form to rely on that.)

----

Yes I understand why you wanted to do it this way, it just doesn't seem to make a lot of sense to allocate a blank NSArray just to check to see if its size is greater than one :-)

Comment 16

[Simon Fraser [no longer active]]

Comment on attachment 211793 [details] [diff] [review]
Updated patch addressing review comments

I'm surprised the changes are so widespread. This indicates that perhaps the code needs factoring into its own class or something.

Comment 17

[Bruce Davidson]

Attachment: Updated patch

Revised patch now returns nil when obtaining array of e-mail addresses from a non-mailto: link.

Removed redundant function that had been left in by mistake.

smfr: Not sure which bits can sensibly be refactored: all the significant change is limited to three methods in the controller, which is where at least two of the methods (context menu preparation) would appear to belong.

Comment 18

[Nick Kreeger]

Comment on attachment 211832 [details] [diff] [review]
Updated patch

R=me

Comment 19

[Ludovic Hirlimann [:Usul]]

Comment on attachment 211832 [details] [diff] [review]
Updated patch

r+

Comment 20

[Mike Pinkerton (not reading bugmail)]

+    return [addToAddressBookItem autorelease];
   }
+  
+  return nil;

i'd prefer to see a local var that was assigned to as the return val, initialized to nil, and returned only once at the end. Just makes things a little cleaner rather than having multiple returns right next to each other.

+    emailAddresses    = [self mailAddressesInContextMenuLinkNode];

why so much space between the var and the equals?

+    for ( unsigned i = 0; i < numEmailAddresses; ++i ) {
+      NSMenuItem* item = [self prepareAddToAddressBookMenuItem:[emailAddresses objectAtIndex:i]];
+      if ( item )

no spacing around parens. eg:

 for (unsigned ... ++i)
 if (item)

+      [[result itemWithTarget:self andAction:@selector(copyAddressToClipboard:)] setTitle:NSLocalizedString( @"Copy Addresses", @"" )];

again, nix the spaces by the parens for NSLocalizedString

+  // Add items to add/open each e-mail address in a mailto: link and
+  // change "address" to the plural form if necessary

do we really want to do this? Couldn't this make the context menu super-large for a long string?

Comment 21

[Bruce Davidson]

Attachment: Updated localized strings file

Comment 22

[Bruce Davidson]

Attachment: Patch addressing Pinkerton's review comments

Patch addressing Mike's review comments.

On your last point its possible that the context menu could be quite large if there are lots of really long e-mail addresses, but I don't really see what else we can do. Given that there are only 1+n (or 4+n for image+mailto: link) items on the context menu for a mailto: link I don't think it will be a problem on any realistic link.

Comment 23

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 215405 [details] [diff] [review]
Patch addressing Pinkerton's review comments

sr=pink

Comment 24

[Bruce Davidson]

Attachment: Revised patch that avoids reversing order of addresses in context menu

The previous patch actually put items in the context menu in the opposite order to the way they were in the mailto: link, this fixes it.

Comment 25

[Bruce Davidson]

Attachment: Revised patch following other checkins

Revised patch following checkin of other changes that clashed. Revised .nib file to follow too.

Comment 26

[Bruce Davidson]

Attachment: Latest BrowserWindow.nib

Comment 27

[Mark Mentovai]

Comment on attachment 216350 [details] [diff] [review]
Revised patch following other checkins

Cancelling rereview request, this version of the patch was just avoiding bitrot.

Comment 28

[Mark Mentovai]

Checked in, trunk and 1.8 branch.  Thanks, Bruce!

Comment 29

[Mark Mentovai]

I corrected a typo in Localizable.strings.  It's generally easier if Localizable.strings changes are described instead of having ugly diff-unfriendly utf-8 attached to bugs.

Comment 30

[Mark Mentovai]

utf-16!  sweet-16!