799723 - [email] Automatically linkify URLs and e-mail addresses in plaintext and HTML e-mails

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect, P1)

Description

[Andrew Sutherland [:asuth] (he/him)]

URLs and e-mail addresses in the message body (and potentially subject) that are not already part of a hyperlink (in the case of HTML messages) should be converted into hyperlinks.

This is a gecko/Thunderbird platform parity issue.  This does not and should not include linkification of phone numbers which has been explicitly postponed and will need to be its own bug.

See bug 799715 for making links actually clickable in HTML messages.  We will want to reuse that UI even in plaintext messages (even though phishing should not be an issue) for consistency and for accidental link opening avoidance.

Comment 1

[Andreas Gal :gal]

Is this for v1?

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Andreas Gal :gal from comment #1)
> Is this for v1?

It was originally slated for v1.  It's a feature, so we'll need permission to develop and land it and its prerequisite.

Comment 3

[Chris Lee [:clee]]

Without copy and paste in v1, this seems like a key usability issue for Email.  Marking as blocking.

Comment 4

[Dylan Oliver [:doliver]]

Cody - can you take a look at this one? This is some late feature work that we need to get done asap.

Comment 5

[Cody Brocious [:Daeken]]

Sure, on it now.

Comment 6

[Cody Brocious [:Daeken]]

I have linking working for emails and URLs in plaintext emails now, though I'm going to have to send them over to the proper place (i.e. opening a compose card for emails rather than using a mailto: link, and sending URLs to the browser).  Going to fix that and implement proper support for linking URLs in HTML emails tomorrow, and then this should be ready to roll.

Comment 7

[Alex Keybl [:akeybl]]

P1 given the new criteria at https://etherpad.mozilla.org/gaia-triage (feature work)

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Cody Brocious [:Daeken] from comment #6)
> I have linking working for emails and URLs in plaintext emails now, though
> I'm going to have to send them over to the proper place (i.e. opening a
> compose card for emails rather than using a mailto: link, and sending URLs
> to the browser).  Going to fix that and implement proper support for linking
> URLs in HTML emails tomorrow, and then this should be ready to roll.

Can you push the commit to a public branch or post it on the bug here so I can take a look and provide feedback?

Comment 9

[Cody Brocious [:Daeken]]

Attachment: Initial patch

This patch takes care of linkifying both HTML and plaintext emails, and provides a (probably incorrectly worded) browse confirmation dialog, as per bug 799715.

I still need to clean it up, comment it, remove the horrible <nil> hack, and add tests, but it seems to be fully functional at this point!

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

From an event handling perspective, I think it's preferable for us to take a page from Thunderbird's event-handling book and have a single event listener on the outside of the iframe.  The primary reasons for this are:
- Efficiency: why have a listener on N links if we can have one on the frame?  (Also, the code currently generates N closures that are effectively identical.)
- When HTML e-mails request a page width, we run them in a pinch-to-zoom context.  Since these happen on the iframe, it's nice to keep the event handling at the same level.

For an example of how to handle that, here's Thunderbird's handler:
https://mxr.mozilla.org/comm-central/source/mail/base/content/contentAreaClick.js#13

Instead of href, we can just look for ext-href.


In terms of hooking that event handling up to the message reader, if you look at createAndInsertIframeForContent it already has a documented callback that is intended to be passed in that will be invoked when links get clicked on.  I think what I would suggest is creating a helper method that adds the single listener and have createAndInsertIframeForContent call that.  Then also have the plaintext case call that helper too.


For the linkification proper, I would strongly prefer that we avoid using innerHTML[1].  Since we are running in a DOM possessing context, we can just create DOM directly.  Instead of creating a "nil" element, you can just create a text node directly (and 'a' elements directly): https://developer.mozilla.org/en-US/docs/DOM/document.createTextNode

1: Our existing uses only put into innerHTML what we took out of innerHTML after running our sanitization pass on an actual DOM tree.

Comment 11

[Alex Keybl [:akeybl]]

We're marking this bug with the C1 milestone since it follows the criteria of "unfinished feature work" (see https://etherpad.mozilla.org/b2g-convergence-schedule).

If this work is not finished by Nov19, this bug will need an exception and will be called out at the upcoming Exec Review.

Comment 12

[Cody Brocious [:Daeken]]

Attachment: Fixed most issues

This patch resolves most of the issues that were mentioned in response to the original patch.  The one remaining thing is handling the event routing inside the iframe; I centralized the event handling in onHyperlinkClick now, but it still handles the setup outside of the iframe.  I can't see a good way to do this, since the links are created after the iframe.  Any thoughts here?

Still need to switch over to L10n as well, but that's more bug 799715.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

This is close enough that I think I can just land this with minor touch-ups.  I will push the linkification proper to be a helper method exposed by MailAPI so we can have some very quick unit tests (in the back-end) and other e-mail UIs can reuse the logic.  I'll be sure to attribute the hunks.  Thanks for the patch!

In terms of the link handler and the ordering, life-cycle does not matter to us because of the DOM event model. jQuery's live() idiom probably has a good explanation of the concept, although it might not be entirely clear how it maps to proper HTML APIs: http://api.jquery.com/live/

Comment 14

[Dietrich Ayala (:dietrich)]

When will this patch be up for review for reals?

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

Per the last comment, I'll be landing this.  I'm actually in the process of doing this currently but need to step out.  This should be landed by this evening.

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

back-end patch landed on gaia-email-libs-and-more/master by:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/79

front-end patch with back-end changes landed on gaia/master by:
https://github.com/mozilla-b2g/gaia/pull/6419

As planned, I migrated most of the logic to the back-end and added some unit tests.  We aren't quite as fancy as mozTXTToHTMLConv, so I commented out some of the tests that would only be appropriate if we were that fancy.  The most notable thing is that we won't linkify "www.google.com", you have to actually have "http://www.google.com" for us to pick up on it.

I'm going to leave the call as to whether we want to improve the code to do that to the drivers dutifully reading this bug.  No comment will be assumed to mean that we should close.

Comment 17

[Dylan Oliver [:doliver]]

(In reply to Andrew Sutherland (:asuth) from comment #16)
> The
> most notable thing is that we won't linkify "www.google.com", you have to
> actually have "http://www.google.com" for us to pick up on it.

Do you already have a regex in mind for finding these type of linkable strings?

Comment 18

[Cody Brocious [:Daeken]]

We could switch over to using something like http://daringfireball.net/2010/07/improved_regex_for_matching_urls fairly easily.  It's not perfect, but it'll give us much better matching, at the cost of a small number of false positives.

Comment 19

[Dylan Oliver [:doliver]]

That would be a nice touch if we can squeeze it in for C1 but I wouldn't want to take it on if it puts this (or other things) at risk for missing the milestone.

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

If it passes the unit tests and expanded unit tests, it seems good to me.  The unit tests I added live in:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/test/unit/test_linkify.js

Run with pushing results to your ArbPL by doing:
make post-one-imap-test SOLO_FILE=test_linkify.js

It might be good to add a does-not-match case or two; reportUrls would probably want to count how many links it emitted and if it's 0, then just emit null values and the expectation could notice if the url field is null and then emit null expectations for both.

Comment 21

[Dietrich Ayala (:dietrich)]

Hi Cody, any update on this? Do you have an ETA for this change?

Comment 22

[Cody Brocious [:Daeken]]

Attachment: Improved the URL matching

This patch adds the improved URL regex, which provides significantly better URL matching.

Comment 23

[Jim Porter (:squib)]

Comment on attachment 683454 [details] [diff] [review]
Improved the URL matching

Review of attachment 683454 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. I ran with this, and things seem good. A comment pointing to the blog post about this couldn't hurt, since the regex is pretty hard to follow.

Comment 24

[Cody Brocious [:Daeken]]

I've landed this patch.  A test (and a quick comment about the source) will be coming shortly but this bug is otherwise resolved.

Comment 25

[Andreas Gal :gal]

Please add a comment with a link to the patch.

Comment 26

[Mila Davydova]

Verified with Unagi, Build ID 20130103070201

Comment 27

[Tracy Walker [:tracy]]

we'll need a specific moztrap test case for the email linkification portion of this bug. Other cases are already and/or will be covered in their own test cases.

Comment 28

[Darren]

Created a test case for checking links in e-mail body.

https://moztrap.mozilla.org/manage/cases/?filter-id=9363