Page MenuHomePhabricator

Send rich HTML mail bodies without impacting reply handling functionality
Closed, ResolvedPublic

Description

The diffs that differential includes in email would be more readable with two "tweaks"

  1. Fixed width fonts
  2. Red/blue colors for +/- lines

This might be hard, idk (it is a jump to HTML emails). It would be good to understand how hard it is. Numerous people here have even been so bold as to say, "I like phab, but I need better diffs in email, and will code it myself if you show me the code...how hard could it be" LOL.

Event Timeline

Makinde added a subscriber: Makinde.

I like phab, but I need better diffs in email, and will code it myself if you show me the code...how hard could it be

https://github.com/facebook/phabricator/

There's an old implementation that just doesn't have any calls but probably mostly works:

https://secure.phabricator.com/diffusion/P/browse/master/src/applications/differential/parser/changeset/DifferentialChangesetParser.php;900190b2fe3a3720$1639

Alternatively we could manually mangle the diff by inserting code here and making the root call $mail->setIsHTML(true) or setHTML(true) or whatever the method is:

https://secure.phabricator.com/diffusion/P/browse/master/src/applications/differential/mail/reviewrequest/DifferentialReviewRequestMail.php;900190b2fe3a3720$136

The major problem this will cause is that replying to these mails is very unlikely to work well. Separating plain text reply content from quoted and HTML content is probably extremely challenging (there's some confirmation of this from people who have tried, on T185). We should probably drop the reply capability entirely when mail is marked HTML.

I'm okay with pursuing this myself soonish since I don't think it's too complicated, but that's likely to be the price for HTML mail.

Thanks for the analysis. Will think about it and get back to you on the
ticket.

Although maybe it's not really that bad, I assume most users just want to !accept stuff and we can probably make that work okay even if we have to have some heuristic that gives up on the rest of it.

yeah, that's my guess as well, they just want !accept/comments at the top
to work.

epriestley triaged this task as Normal priority.Mar 15 2012, 1:17 AM

In case you all want to follow along, or dive in and knock off the task...

btrahan added a project: Differential.
epriestley renamed this task from Colorful diffs in emails to Colorful diffs in emails (HTML email).May 9 2013, 11:51 AM
talshiri edited this Maniphest Task.Jun 19 2014, 6:18 PM
talshiri added a comment.EditedJun 19 2014, 6:22 PM

D9375 works reasonably well, but need to verify that it doesn't break reply-by-email in popular email client (see differential review for details)

epriestley changed the visibility from "All Users" to "Public (No Login Required)".Jul 7 2014, 4:25 PM
epriestley renamed this task from Colorful diffs in emails (HTML email) to Send rich HTML mail bodies without impacting reply handling functionality.Aug 4 2014, 4:23 PM

I think we can move this forward, but want to be slightly more gradual about it:

  • In the new Settings > Email Format screen, add a new user preference.
    • Options are: Use Server Default (Plain Text) -- this is not configurable for now, Send Plain Text Email and Send HTML Email.
  • For v1, let's just do some simple HTML to get the basic infrastructure in, like making the headers stand out and making links work. The infrastructure stuff in D9375 mostly looks good to me, although I have some minor feedback once we get there.
  • For v2, we should render remarkup blocks.
  • For v3, we can add the actual diffs and inlines and whatever.
  • Cool, I'll add the formatting option.
  • Correctly rendering remarkup blocks might be a little hairy, as it requires inlining CSS. Maybe we need to teach PhutilRemarkupBlockRule how to render tags for email, or figure out some post-processing css inline pass?

Yeah, it's probably going to be a mess. I think adding a third rendering target (Email With Inline CSS vs HTML vs Text) might be easier than trying to do post-processing safely, although maybe post-processing isn't too bad. XSS isn't too threatening in email, and we can trust that the input is well-formed, so maybe we can just look for class="..." inside tags and replace that with style="...". Maybe worth taking a shot at first, and we can add a third target if it's a huge gross mess?

Yeah, this needs some poking around, although I'm leaning towards teaching PhutilRemarkupBlockRule how to render things for email.
There's just one render target right now I think. The text rendering just dumps the remarkup as is.

I have yet to see a post-processing solution that I was happy with though. We may also need to render certain things a little differently for emails.
This is actually why I started off with the just doing the diff. It was easy and it was immediately useful.

What if we initially rolled it out without remarkup rendering, so we can gather some feedback?

Yeah, I think a v1 with no remarkup rendering is totally fine.

I mean, make v1 also do diffs (because they are so easy compared to remarkup), which is pretty much D9375 plus minor fixes plus per-user config. I can also rip that part out if you'd prefer, although we found it to be useful.

If you want to do the diff thing before remarkup that's OK, but I think it should be separate from the basic infrastructure patch.

Cool. I'll chop em up and send patches.

Makinde removed a subscriber: Makinde.Aug 13 2014, 9:05 PM
waynea added a subscriber: waynea.Aug 16 2014, 1:37 PM
nickz added a subscriber: nickz.Aug 20 2014, 12:39 AM

Add myself as a follower.

cakoose removed a subscriber: cakoose.Nov 9 2015, 11:46 PM
epriestley closed this task as Resolved.May 22 2016, 7:20 PM
epriestley claimed this task.

We now send HTML mail by default.

We aren't currently aware of any reply handling problems this causes. We've supported HTML mail for quite some time now and received enough feedback about bugs and formatting to be reasonably confident it's seeing some use, but little or no feedback about reply handling, which suggests that's working fairly well as-is.

Work connected to T10694 has improved some of the specific diff-related formatting behaviors that got specific mentions earlier in this task.