Improve inline mail snippet rendering, possibly fixing Airmail?
ClosedPublic
Actions

Authored by epriestley on May 6 2016, 6:50 PM.

Details

Reviewers

chad

Maniphest Tasks

T10694: Improve behavior of context for inline comments in mail

Commits

rP412fc3455730: Improve inline mail snippet rendering, possibly fixing Airmail?

Summary

Ref T10694. General improvements:

Remove leading empty lines from context snippets.
Remove trailing empty lines from context snippets.
If we removed everything, render a note.
Try using style instead of <pre>? My thinking is that maybe Airmail has weird default rules for <pre>, since that's the biggest / most obvious thing that's different about this element to me.

Test Plan

Viewed normal comments locally, faked a comment on an empty line in the middle of a lot of other empty lines.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley updated this revision to Diff 38215.May 6 2016, 6:50 PM

epriestley retitled this revision from to Improve inline mail snippet rendering, possibly fixing Airmail?.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: chad.

epriestley added a task: T10694: Improve behavior of context for inline comments in mail.

epriestley added inline comments.May 6 2016, 6:51 PM

src/applications/differential/mail/DifferentialInlineCommentMailView.php
380	Also wondering if the `float: right;` on the "View Inline" link might be messing it up, so I threw this in there for good measure.

chad accepted this revision.May 6 2016, 6:55 PM

chad edited edge metadata.

This revision is now accepted and ready to land.May 6 2016, 6:55 PM

Closed by commit rP412fc3455730: Improve inline mail snippet rendering, possibly fixing Airmail? (authored by epriestley, committed by epriestley). · Explain WhyMay 6 2016, 6:58 PM

This revision was automatically updated to reflect the committed changes.

epriestley added inline comments.May 6 2016, 7:03 PM

src/applications/differential/mail/DifferentialInlineCommentMailView.php
386	This is live now -- any better in Airmail?
src/applications/differential/render/DifferentialChangesetOneUpMailRenderer.php
51	Here's a comment after some whitespace.
133	Here's a comment on some whitespace.

61751230-6C75-44B8-BDEA-73267645D43B.png (1×1 px, 189 KB)

it's less worse, but somethings still off. not a big deal.

The wordbreak here:

implode('
', $style)

...makes me think this is an Airmail rendering bug, since I think it's breaking on the <style> tags. There's definitely no newline there, the raw source is all one line with no linebreaks (this element below renders with linebreaks because the codeblock has a pre-wrap style on it):

<span style="color: #004012" data-symbol-name="implode">implode</span><span style="color: #aa2211">(</span><span style="color: #766510">&#039; &#039;</span><span style="color: #aa2211">,</span> <span style="color: #001294">$style</span><span style="color: #aa2211">);</span>

The other rendering (with everything smushed up against the left margin) seems buggy, too: it's ignoring leading whitespace and that seems fairly unambiguously wrong of it.

It's also not totally-utterly-completely unreadable, so maybe wait for users to complain I guess, and then we can distill a test case and file an upstream bug against Airmail?

I found some vague stuff in their bugtracker but it all looks fairly useless, like "HTML emails don't work right":

http://feedback.airmailapp.com/forums/274948-airmail-mac-2-0/suggestions/6867770-airmail-doesn-t-render-some-html-emails-correctly

epriestley added inline comments.May 6 2016, 7:18 PM