1217809 - [Email] quoted reply generation is failing to inject newlines for the non-base-case

Status: VERIFIED FIXED

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

Description

[Augustin Trancart [:autra]]

[Steps]
- with thunderbird of the email application of Firefox OS, send yourself a mail with a bit of content.
- reply to this mail to yourself
- reply a second time to have a 2 level deep quote

[Expected] 
- the first mail appears quoted 2 times in the 3rd one, ie with two vertical bars. See attached screenshots for another conversation that works (with other mail clients)

[actual]

The second level has only a slightly thicker bars. See screenshots.

Comment 1

[Augustin Trancart [:autra]]

Attachment: mail_quote_actual.png

Actual result.

Comment 2

[Augustin Trancart [:autra]]

Attachment: mail_quote_expected.png

This is an expected result with answers from other people.

Comment 3

[Augustin Trancart [:autra]]

Attachment: Actual result in thunderbird

This is the same conversation than mail_quote_actual.png viewed with thunderbird

Comment 4

[Augustin Trancart [:autra]]

Attachment: test2 done with thunderbird viewed in thunderbird

I did the test by generating a conversation with thunderbird: here is what it looks in thunderbird.

Comment 5

[Augustin Trancart [:autra]]

Attachment: test2 done with thunderbird in FxOS

And here is how the second test looks in FxOS. The levels does not appear.

Comment 6

[Augustin Trancart [:autra]]

tested on flame.
BuildId: 20151009

tested with gmail accounts.

Comment 7

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

Apologies, but I'm having trouble identifying which of the two situations you're reporting here:

1) When replying to itself, Firefox OS is screwing up the quoting in the message it generates; there's a newline missing or something like that.  (Indicated by the first pair of screenshots, with the second pair illustrating a scenario where that is not happening because Thunderbird does not have the bug.)

2) The visual display of quoting uses thicker bars for nested quotes instead of multiple thin bars.  (Visible in both pairs of screenshots.)

The second is intentional (although it absolutely can be improved and should likely be revisited), while the first seems like a bug in our compose UI and/or the quoted reply or quote analysis logic.


If it's the first case, it would also be great if you could provide a screenshot of the compose UI immediately after you hit "reply" and before you typed anything, and then again prior to you hitting send.  I can probably also try and reproduce this myself if you can let me know which version/build of Firefox OS you're using and what locale was used during the process.  (I'm assuming v2.5/trunk based on your email address and en-US based on the strings I'm seeing in the messages and on the screen.)

Thanks!

Comment 8

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

(In reply to Augustin Trancart [:autra] from comment #6)
> tested on flame.
> BuildId: 20151009

Whoops.  I missed this, but I guess this could still be a v2.2 branch?  Still, I'll assume v2.5/trunk otherwise.

Comment 9

[Augustin Trancart [:autra]]

Oh sorry, I forgot to mention: yes it's on v2.5/trunk branch, with US locales (the prod one, not the devs).

Also sorry: you're right, there's 2 bugs here. I didn't realize it at first.

The original issue I wanted to report is 1/. 

For 2/, it is also a matter of UX decision right?
Anyway, FxOS behaves differently if the mail comes from gmail address (see mail_quote_expected.png), and this difference is definitely a bug. Opening one now.

Comment 10

[Augustin Trancart [:autra]]

I filled bug 1218340.

Comment 11

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

It turns out I have a fix for this on the convoy branch.  It builds on 1169130, so that will be taken too.  (Without the bug 1169130 fix, the problem is somewhat mitigated through the inclusion of newlines in the block, which is why this bug was not previously more obvious.)

Fix and additional test cases coming shortly.

[Blocking Requested - why for this release]:
Nested quoting of text/plain messages gets corrupted due to newline normalization on parse but which is not regenerated on output.

Comment 12

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

Attachment: GELAM pr

The bullet points reviewers crave:
- This includes my fix for bug 1169130.
- The reply logic for text/plain was totally failing to insert newlines where it was appropriate to insert them.  This was bad because we normalize the blocks in our quotechew representation to not include trailing newlines.
- This was not a problem for the forwarding logic because the goal for forwarding logic was always to try and regenerate the original message and our quotechew-representation went out of its way to track all the newlines it ate.  And...
- The forwarding case had significantly better test coverage.  test_mail_quoting would check that its input messages would roundtrip through forwarding because this was very easy to do because of the verbatim goal.  Only one case was not roundtripped.  Unfortunately, for replies, the test was against the derived representation we produced, and never against the string we would produce for creating a reply from that representation.  mea culpa.  mea maxima culpa.
- I have added a test that actually tests quotechew.generateReplyText.
- Most of the changes to the quote chewing logic are comments that past-me forgot to write.
- Our generateReplyText always had the intent that it was improving things rather than just cramming a ">" at the front.  Given the massive longstanding screwup here, it does beg the question of whether this was actually a good idea.  I think much of my rationale was that because of the need to wrap text/plain messages and the inherent contribution of inserting ">" to the need-to-wrap, roundtripping was already bad news.  Also...
- One can argue that ours is indeed a noble goal to improve the generated/derived quoted reply, since these things frequently do become a train-wreck.  However, in the bright future we may someday reach, I'd expect we instead do it by round-tripping everything through a text/html representation.  (Which we may then subsequently downconvert to text/plain, although I'm still generally of the mind that people who refuse to read text/plain are better served by having their MUA downconvert to text/plain on their end from text/html rather than relying on the sending client to do it competently.)

I am planning for us to uplift this to v2.5.  In general, I would view this as safe because our lack of newlines clearly sucks, and erring on the side of adding too many newlines is not likely to make things worse.

Comment 13

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

Er, re re-wrapping as things get "wider" from prefixing extra >'s, I should note that we don't actually (intentionally) do anything useful here.  However, we are in good (AKA equally bad) company here, and I would be concerned regressions with doing that.  Whereas this is "just" newlines (sometimes pretty-whitespace newlines, but still, just newlines).

Comment 14

[James Burke [:jrburke]]

Comment on attachment 8682930 [details] [review]
GELAM pr

r+ given that it is an improvement over existing output, and there are tests now to cover the expected logic.

Comment 15

[Dylan Oliver [:doliver]]

I don't think this fits our blocking criteria but please request approval‑gaia‑v2.5 on this one.

Comment 16

[GitHub Autolander Bot]

Attachment: [gaia] asutherland:email-quote-reply > mozilla-b2g:master

Comment 17

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

landed on trunk:
https://github.com/mozilla-b2g/gaia/pull/33007
https://github.com/mozilla-b2g/gaia/commit/8d78e18bb619abac74d58d97ab20f5fbdf173ac6

https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/389
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/41c2a6771f16dbcd8df85f3442f26d129396cecf

Comment 18

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

Comment on attachment 8683424 [details] [review]
[gaia] asutherland:email-quote-reply > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by]:
Not a regression.

[User impact] if declined:
When replying to a text/plain message, particularly one that is itself a reply to another text/plain message and included quoting, newlines will not be emitted between most sections, resulting in things looking bad.  See attachment 8678084 [details] as an example of things looking bad.

[Testing completed]:
Introduced new automated back-end test coverage covering many text/plain quoting cases.  These tests join existing coverage for quote parsing and forwarded message generation.

[Risk to taking this patch]:
This is believed to be quite low risk.  The new logic introduces a bounded amount of additional newlines where previously there were too few.  Even if it glitched it's still an improvement.

[String changes made]:
None.

Comment 19

[Mahendra Potharaju [:mahe]]

Comment on attachment 8683424 [details] [review]
[gaia] asutherland:email-quote-reply > mozilla-b2g:master

Approved for 2.5 uplift. 

Thanks

Comment 20

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: verified fixed screenshot

Took me a while to reproduce this bug on an earlier build. Looks like it will have to be from Thunderbird that's sending the original/first email for this to occur.

This issue is verified fix on latest Aries and Flame. Quotes are generated as expected on this particular use case. The quoted line gets thicker for older level replies. See attached screenshot for verified behavior.

Device: Aries 2.6 Master
BuildID: 20151112122058
Gaia: 27bc9412ca607648bc398b25bb1ae25653b2b278
Gecko: 3cc3b1968524248450c465c4ea2ee5596ffa65f2
Gonk: a19052e4389c3ae2d8fc3e7a74a475401baacc56
Version: 45.0a1 (2.6) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:45.0) Gecko/45.0 Firefox/45.0

Device: Flame 2.6 Master
BuildID: 20151112030244
Gaia: 27bc9412ca607648bc398b25bb1ae25653b2b278
Gecko: 3cc3b1968524248450c465c4ea2ee5596ffa65f2
Gonk: 205ac4204bbbb2098a8046444acba551ba5dc75a
Version: 45.0a1 (2.6) 
Firmware Version: v18Dv4
User Agent: Mozilla/5.0 (Mobile; rv:45.0) Gecko/45.0 Firefox/45.0

Comment 21

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Setting verifyme for checking this when the 2.5 uplift occurs.

Comment 22

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/2e02071f7580140a333248a40e66aeebd8eff35c for 2.5