166958 - context menu: add/change items for mailto: links

Status: VERIFIED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

[sairuh (rarely reading bugmail)]

in the spirit of integrating chimera with OS X applications, we could add some
more items to the context menu of mailto: links.

currently, we have the following when you control-click on mailto: link:

Open Link in New Window
Open Link in Tab
Copy Link Location
Download Link Target
Bookmark This Link

how about the following instead? let me know if i should spin off separate bugs
if particular items would take more time than others to implement.

Add Email Address to Address Book
Copy Email Address (copies "[e-mail]")
Copy Link Location (copies "mailto:[e-mail]")
Send Page to This Email Address (would bring up default mail application)
(not sure if Bookmark This Link would be relevant/common for mailto:'s)

Comment 1

[Bartosz Wucke]

>Copy Email Address (copies "[e-mail]")
>Copy Link Location (copies "mailto:[e-mail]")

copying "mailto:" URLs will make sense only while composing your own webpage and
copying emails for it from other webpages. Not a common activity, we could give
up  "copy link location" item freely.

> Send Page to This Email Address (would bring up default mail application)

Erm, which page? The one you're visiting? (would mean usually sending a page to
its author). Or maybe arbitrary file? (just "compose..." and add attachment instead)

(not sure if Bookmark This Link would be relevant/common for mailto:'s)

Not really necessary feature, but why limiting users' possiblities? For a small
set of addresses, personal toolbar folder is much better than addressbook.

"Create filter from address" could be relevant too - just like in email header
in MailNews.

Comment 2

[sairuh (rarely reading bugmail)]

revised suggestion:

Copy Email Address (copies "[e-mail]")
Add to Address Book (only available on 10.2+)
[separator]
Save Page As...
Bookmark This Page...

Comment 3

[Jasper]

Oh wow, I never actually read this bug yet but the suggestions look absolutely
fantastic. As we would then provide very gracefull integration with Addresbook
and Mail.

Comment 4

[Bruce Davidson]

Hmm... it looks like this really depends on adding a fair bit of support to the
core product, rather than just fiddling in the Camino code. Bug 86924, which
would probably add lots of the support, has unfortunately been untouched in over
a year.

Ideally we'd like to add a new enumeration value to the nsIContextMenuListener
class, e.g. "CONTEXT_MAILTO_LINK", and fix up the code that fires the
onShowContextMenu() method, so that all the tools can use it. (In fact to really
get to grips on all the various issues (e.g. different icons when you hover over
them) a couple of extra methods on nsIHTMLAnchorElement (or whichever
proprietary interface we use to extend the W3C DOM approved interface), e.g.
"isMailLink" and "linkMustBeHandledExternally".)

Without this we are left changing BrowserWindowController's getContextMenu()
method to examine the text of the link's href attribute and deciding if its a
mailto. This is perfectly doable.

We then need a new action for the context menu to call. The first one is
presumably quite easy if we have any code that deals with the address book at
the moment. (I looked at AddressBook.h, but comments indicate its obsolete once
we switch completely to 10.2 and I wasn't sure where else to look). However the
second menu option gives us a problem, if we want to continue to use Mozilla's
core clipboard support. The nsIClipboardCommands interface only allows us to
copy the link location (which would include the "mailto:" text) or a selection.
There's no support for copying arbitrary text to the clipboard using that
interface. I don't know what the default Apple interfaces for talking to the
clipboard are like; it may be that they're clean enough to use and ignore the
core mozilla issue.

If anyone can provide some pointers for the address book code I'm happy to have
a go at a camino specific implementation. I'm not involved enough to want to
start changing core interfaces (which are probably frozen anyway) right now.

Comment 5

[Bruce Davidson]

Having a go at doing the quick and dirty Camino only solution. Assigning the bug
to myself.

Current state: accurately determines whether you're on a mailto: link and if so
presents alternate context menu. Next step is the copy and "add to/open in"
address book. (Noticed that Mail.app does a cool thing where the address book
option on an e-mail address depends on whether the address already exists in
your address book or not.)

Comment 6

[Bruce Davidson]

Attachment: Interim patch 1

This interim patch fixes the "copy" aspect (in combination with a new .nib that
I'll attach). Still need to tidy up this patch and do the address book
integration stuff but its a start (and makes requesting reviews in Bugzilla
much easier).

I'm not requesting review until I have a finished patch, but any comments
gratefully received.

Comment 7

[Bruce Davidson]

Okay, I've got a working implementation of this, which I'll tidy up later this
week. This comment is to document the proposed UI and invite any comments.

The new functionality changes the context menus for mailto: links, including
mailto: links around images. Instead of the normal link options two options are
provided (plus image options when appropriate):

Copy Address
Add to Address Book

The copy option does what it says on the tin, it copies the e-mail address from
the link, without any prefixes to the clipboard. It is always enabled.

The add to address book option is enabled if and only if the e-mail address in
the link does not exist in your address book. When selected it creates a new
entry in your address book with the work e-mail set to the address in the link. 

If the e-mail address already exists in your address book then the option is
disabled. If a real name or company name (for a company record) is associated
with the e-mail address the menu option title is changed to display the
real/company name. If there is no real or company name I'm unsure whether to
simply disable the menu item (but leave it reading "Add to Address Book") or
whether to disable it and change the text to the e-mail address. Any comments on
this?

Comment 8

[Bruce Davidson]

Okay, Geoff has just pointed me at a URL for providing an "open in address book"
option in the same way that Mail does when the entry already exists in your
address book. Personally I'm not sure you'd ever really want this option from
your web browser, but (especially if we insert the name, e.g. "Open Bruce
Davidson in address book") its much clearer than the disabled e-mail
address/"Add to Address Book" text in the case where the address book only
contains the e-mail. Again, any thoughts from UI experts on what the best option is?

Comment 9

[Jasper]

I think geoff was very right in his comment. Duplicating the behaviour of Mail
seems a good idea. That way we go with expectations on this level. Otherwise I
also can't think of what else we should do becuase we already have 1) create a
new email using click, 2) copying the email address [for cc or other things] 3)
and adding email addresses to addressbook. Nothing should be added more.

Quick question. One user can have multiple email addresses in address book. In
mail adding an address is smart in the sence that it looks at the full name of
the sender to detirmine to what existing person the email address should be
added. Is there any way of Camino also being as smart as that? If not what else
can we do to help our users? Look at mail perhaps?

Comment 10

[Bruce Davidson]

Mail will add the address to the correct person if its from an e-mail address 
that has the full name (e.g. "Bruce Davidson <[e-mail]>"). 
Unfortunately there is no equivalently defined format for mailto: links. 
There's nothing in the href text to tell us the real name of the recipient. 
The link text could be anything under the sun; I don't think parsing it to 
determine a real name will achieve anything. So the add to address book option 
is just going to add it as a new entry in the address book.

Comment 11

[Mike Pinkerton (not reading bugmail)]

do we want to launch address book and try to have this card selected after
choosing this context menu item? that'll allow them to finish filling it in if
they want.

Also, searching abook is very fast, it would be pretty easy to scan the entire
abook (even a large one) for the email address to make sure it's not already in
there before we allow them to add it.

Comment 12

[Bruce Davidson]

Attachment: Full patch and new source files

This is a full patch for both the copy and "add to/open in address book"
funtionality, as discussed in earlier comments. The tar file contains an
updated BrowserView.nib, two new source files to add a category for
ABAddressBook and patches to existing source files and the .xcode project. Not
sure how well patches to these work - but if not it should be pretty easy to
read the patch to work out what you need to do in xcode to add it.
Requesting review.

Comment 13

[Bruce Davidson]

Attachment: Revised patch with correct NIB file

Grabbed the nib file from the wrong directory in the previous tar file. Sorry.

Comment 14

[Josh Aas]

Comment on attachment 166885 [details]
Revised patch with correct NIB file

Maybe a year ago or so there were a few nightly builds that had the "copy email
address" part of this bug done. Someone was mucking with that code and within a
few days the feature disappeared.

Anyway, I feel like gecko ought to support this in general without us having to
add Camino specific code. I feel like the code is already there somehow based
on what I saw before and the fact that it makes sense for it to be in gecko. I
don't think hacking in this feature is a good idea.

Comment 15

[Jasper]

On the other side, if we waited for Gecko to have this we would have grown a
very big beard :)

The way Bruce did it makes sure that Camino will always have this feature even
if they decide to put it in gecko some day, or decide to remove it again or
something brakes it.

But I guess I agree that in the end it would be best if the mailto: detection
would take place i Gecko. But wouldn't that mean the UI for all the gecko based
apps would have to changed aswell for that to be checked in? Any chance somebody
could help out bruce to find where that kind of stuff happens?

Comment 16

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #14)
> Anyway, I feel like gecko ought to support this in general without us having to
> add Camino specific code. I feel like the code is already there somehow based
> on what I saw before and the fact that it makes sense for it to be in gecko. I
> don't think hacking in this feature is a good idea.

I'm going to disagree with this assessment. It's a weak defense, but "firefox does it this way". Looking 
at where it happens in firefox:
http://lxr.mozilla.org/mozilla/source/browser/base/content/browser.js#3288

this would place it in a very similar place for us. If it appears in gecko, they have the same problem we 
do.

Comment 17

[Geoff "temporarily off bugmail" Beier]

This patch relies on bits of the AB api only available on 10.3 and later:

From ABAddressBook+Utils.mm:

int displayMode = [[person valueForProperty:kABPersonFlags] intValue] & kABShowAsMask;
if (displayMode == kABShowAsPerson) {

From ABGlobals.h:

extern NSString * const kABPersonFlags             AVAILABLE_MAC_OS_X_VERSION_10_3_AND_LATER;     /
/ Various flags - kABIntegerProperty
    #define kABShowAsMask                           000007
    #define kABShowAsPerson                         000000

Since we build against the 10.2.8 SDK and support 10.2, we need to find a way to do this without using 
10.3-only features, or make the feature conditional upon a runtime check... 

As an aside, with the way my build systems are configured, this patch won't even build because of this.
r-

Comment 18

[Bruce Davidson]

Attachment: Updated patch to build on 10.2 and 10.3

I agree with Josh's sentiment that it would be nice to do this purely in Gecko.
However the support isn't there (see my earlier comment on the extent of the
changes required). Given that there doesn't appear to be any progress on adding
this to Gecko, I think we should however go ahead and add this functionality to
Camino in the meanwhile. Most of this patch is taken up with the UI and address
book functionality we'll require in any case. The amount of code we'd have to
remove/change to use a gecko notification of mailto links is pretty small.

This revised patch fixes several issues different people have noticed:
* Should build and run on 10.2 as well as 10.3 (use our own copies of the 10.3
specific constants and detect if the property is not available)
* Cope with multiple e-mail addresses/subject lines etc, by ensuring we only
grab the first e-mail address
* Added the missing localised string

Have spoken with Pinkerton briefly, and he seems happy enough with the idea of
landing this rather than waiting for gecko, so I'm requesting reviews on this
patch.

Comment 19

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #18)
> This revised patch fixes several issues different people have noticed:
> * Should build and run on 10.2 as well as 10.3 (use our own copies of the 10.3
> specific constants and detect if the property is not available)

The patch as supplied does not build against the 10.2 SDK:

@@ -310,18 +311,18 @@
 //194
 //290
 //291
 //292
 //293
 //294
 		29B97313FDCFA39411CA2CEA = {
 			buildSettings = {
-				MACOSX_DEPLOYMENT_TARGET = 10.2;
-				SDKROOT = /Developer/SDKs/MacOSX10.2.8.sdk;
+				MACOSX_DEPLOYMENT_TARGET = 10.3;
+				SDKROOT = /Developer/SDKs/MacOSX10.3.0.sdk;
 			};
 			buildStyles = (
 				4A9504CCFFE6A4B311CA0CBA,
 				4A9504CDFFE6A4B311CA0CBA,
 				F593CA5E034EB88E01A967F3,
 			);
 			hasScannedForEncodings = 1;
 			isa = PBXProject;

That hunk changes the SDK used by the camino tree to the 10.3.0 sdk. We can't do that right now. (The 
reason I noticed this is because I don't have a shared menus framework linked inside my 10.3.0 sdk :-))

Once I removed that hunk and repatched, stuff built fine, but whenever I click on a mailto link I don't 
see anything on the context menu. Perhaps the nibs in the patch are incorrect or incomplete somehow? 
That's what it looks like after a quick pass with a debugger anyway.

Comment 20

[Bruce Davidson]

Attachment: NIBS tarred rather than as patches

I've attached the NIB files as a tar.gz as clearly just leaving them to patch
doesn't work. (Although most of the NIB now seems to be text there is still at
least one binary file in there).

Hopefully you'll have more luck with this.

Comment 21

[Geoff "temporarily off bugmail" Beier]

This patch is really hard to apply. Much as I hate to ask, could you respin it
against current CVS such that:

1. Everything needed to patch the tree is in 1 tarball.
2. None of the .nib goo is in the patch.
3. (As I mentioned in comment # 19) it's built against the 10.2.8 SDK instead of
the 10.3 SDK.

That said, once I manually fix (2) and (3), the patch appears to work as
described and the code looks good. I'd like to re-review the fixed patch, but
assuming that gets done I'm prepared to r+. I should be able to review again on
Monday, so fixing it before then would be optimal.

Also, you might want to put your name on (at least) ABAddressBook+Utils.h and
.mm :-)

Comment 22

[Bruce Davidson]

Attachment: Updated patch against latest CVS

I've fixed the one remaining reference to a 10.3 constant, so should be 10.2
clean.

Updated tarball containing:
- The patch (excluding NIB stuff) as a text file
- The two new files (with my name added to the contributors list)
- The NIB files

Hopefully this works.

Comment 23

[Bruce Davidson]

Comment on attachment 169509 [details]
Updated patch against latest CVS

Obsoleting this patch because it causes a conflict with Josh's checkin to tidy
up the old drawer code.

Comment 24

[Bruce Davidson]

Attachment: Updated patch against latest CVS

Updated patch. Also tidy up where files are placed so that you can just untar
this into your mozilla/camino directory and have everything in the right place.
Should apply cleanly against latest CVS after Josh's code tidy up checkin.

Requesting review.

Comment 25

[Jasper]

I have been test driving a build with this patch and it is, how shall I say, a
pure testament of how well apps on OS X can be integrated. It's as if Address
Book was part of Camino. Eat that Safari and Firefox ;)

Have yet to find any problem.

Comment 26

[Geoff "temporarily off bugmail" Beier]

Comment on attachment 169911 [details]
Updated patch against latest CVS

I still can't apply this patch and build... now there's an absolute path
reference included to a path that doesn't (and shouldn't) exist on my system:

PBXCp build/Camino.app/Contents/Resources/layout_xul_tree.xpt
/Users/Shared/camino/mozilla/dist/bin/components/layout_xul_tree.xpt
    cd /Users/gbeier/camino-patchreview/mozilla/camino
    /Developer/Tools/pbxcp -exclude .DS_Store -exclude CVS
-resolve-src-symlinks
/Users/Shared/camino/mozilla/dist/bin/components/layout_xul_tree.xpt
/Users/gbeier/camino-patchreview/mozilla/camino/build/Camino.app/Contents/Resou
rces
/Users/Shared/camino/mozilla/dist/bin/components: No such file or directory

Comment 27

[Bruce Davidson]

Attachment: Further patch (up to date against CVS)

Revised patch addressing the problem with the project file. (Apologies,
remnants of an attempt to fix about:config). Updated against the latest CVS.
Requesting review.

Comment 28

[Noemi]

Tested on 10.2.8, and it seems to work fine -- I was able to add an email
address to the address book and re-open it later using the contextual menu.  The
name from the address book showed up, too.  No crashes or other unexpected behavior.

Comment 29

[Mike Pinkerton (not reading bugmail)]

landed with some cleanup to project and window controller, fixed a leak in the
ab extensions. 

Comment 30

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 170496 [details]
Further patch (up to date against CVS)

clearing review flags, sr=pink.

Comment 31

[sairuh (rarely reading bugmail)]

looks great using 2005011808-trunk (10.3.7). now have Copy Address and Add to
Address Book.