Page MenuHomePhabricator

Improve inline mail snippet rendering, possibly fixing Airmail?
ClosedPublic

Authored by epriestley on May 6 2016, 6:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Aug 15, 8:57 AM
Unknown Object (File)
Tue, Aug 9, 10:12 PM
Unknown Object (File)
Tue, Aug 9, 9:21 PM
Unknown Object (File)
Mon, Aug 8, 3:22 PM
Unknown Object (File)
Mon, Aug 8, 2:20 PM
Unknown Object (File)
Fri, Aug 5, 12:14 AM
Unknown Object (File)
Thu, Aug 4, 1:27 PM
Unknown Object (File)
Tue, Aug 2, 3:13 PM
Subscribers
None

Details

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 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.
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 edited edge metadata.
This revision is now accepted and ready to land.May 6 2016, 6:55 PM
This revision was automatically updated to reflect the committed changes.
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.

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

src/applications/differential/mail/DifferentialInlineCommentMailView.php
386

(Here's a reply comment.)