Closed Bug 1288911 Opened 8 years ago Closed 8 years ago

Quote in Reply with unwanted line wraps (broken quote lines), quote wraps to overall body if mailnews.wraplength > 0.

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Version:
49 Branch
Platform:
Unspecified
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: RainerBielefeldNG, Assigned: jorgk-bmo)

References

Details

(Keywords: regression, reproducible)

Attachments

(2 files, 3 obsolete files)

Showing the effect in Firefox, quote is *not* wrapped to surrounding reply
1.38 KB, text/html
Details
1288911-css-to-avoid-wraps.patch (v2).
2.51 KB, patch
masayuki
: review+
jcristau
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
Steps how to reproduce with  English SeaMonkey 2.46a2  (Windows NT 6.1; WOW64; rv:49.0)  Gecko/20100101 Firefox/49.0 Build 20160723030242  (Default Classic Theme)  on German WIN7 64bit:

0. Launch SeaMonkey email client, if necessary menu ˋEdit → Preferences
   → Mail & Newsgroups → Composition → Wrap after 72 characters [ok] → 
   Edit → Preferences →  Mail and Newsgroups account settings → 
   news.mozilla.org → composition → Start my reply below quote → [ok]ˊ
1. In email-client open mozilla.support.seamonkey
   <news://news.mozilla.org/mozilla.support.seamonkey
2. find  Posting ”LastPass not working for certain sites”
   of “Mr. Cheese” 22.07.2016 11:21 -0400 
3. ˋSelect (click) this posting in Thread Pane  → Click icon
   'Reply'ˊ
   » Message composition opens with quoted old message
   Bug: A line with only word “the” without leading quoting mark, 
   compare screenshots <https://seamonkeyde.wordpress.com/2016/07/22/neue-offizielle-nigthly-trunk-builds-fuer-linux-und-windows-verfuegbar-2/>

a) Saving as draft heals the problem.
b) it seems that the new line will be started with the first word what
   will exceed the limitation of 72 words / line.
c) 2.47 also affected
d) Was still ok with  English SeaMonkey 2.45  (Windows NT 6.1; WOW64;
   rv:48.0)  Gecko/20100101 Firefox/48.0 Build 20160714190830  
  (Default Classic Theme)  on German WIN7 64bit
e) "reprodcucible" due to Rüdiger Lahl's comment above screenshots 
   in step 3
f) I will test Thunderbird, soon!
f): With steps 1 ... 3 also REPRODUCIBLE with Thunderbird Daily 50.0a1 (2016-07-05)
Component: MailNews: Composition → Composition
Product: SeaMonkey → MailNews Core
Version: SeaMonkey 2.46 Branch → 49
NEW due to multiple ways how to reproduce
Status: UNCONFIRMED → NEW
Ever confirmed: true
status-thunderbird50: --- → affected
Summary: Quote in Reply with unwanted line wraps → Quote in Reply with unwanted line wraps (broken quote lines)
This is a consequence of bug 387687 ... and I was afraid we would get complaints.

This is what happens, and you can use ThunderHTMLedit to look at it.

Before the plain text reply was formatted like this:
  <body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;">
  <div class="moz-cite-prefix">
  On 23/07/2016 21:17, Magnus Melin wrote:<br>
  </div><span style="white-space: pre;">here is the quoted text<br>

Now it's done like this:
  <body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;">
  <div class="moz-cite-prefix">
  On 23/07/2016 21:17, Magnus Melin wrote:<br>
  </div><span style="white-space: pre-wrap;">here is the quoted text<br>
=====================================^^^^^

So the overall plaintext sits in a style="white-space: pre-wrap; width: 72ch;"
and the quote in a style="white-space: pre-wrap;". Before that was style="white-space: pre;".

Change here:
https://hg.mozilla.org/mozilla-central/rev/192a5ffbf501#l1.30

So before the quote could be so long that scroll bars were displayed, that's what bug 387687 was about. Now the quote wraps, but leads to some ugly visual effects. Masayuki noticed this problem, see bug 387687 comment #33.

I'd mark this bug here WONTFIX, since you can't have it both ways, wrap and not wrap. To to put it with Shakespeare: To wrap or not to wrap, that is the question ;-)

What I do in a situation like this, I change the "pre-wrap; width: 72ch" to "pre-wrap; width: 80ch" in ThunderHTMLedit or use Edit > Rewrap.

Another option is: mailnews.wraplength = 0, see bug 387687 comment #32.

BTW: This is in Core::Editor.
Summary: Quote in Reply with unwanted line wraps (broken quote lines) → Quote in Reply with unwanted line wraps (broken quote lines), quote wraps to overall body if mailnews.wraplength > 0.
BTW, I'm open to better suggestions ;-)
I remember to have seen these single endless line quotes from time to time (rather seldom), that was ugly.
Now we have a quote issue visible in nearby every reply - I think that is more ugly. 
Workarounds  with 'pre-wrap' or 'mailnews.wraplength' only shift the problem to other text lines, that's not a solution.

Currently I don't have a suggestion, but the current behavior seems not satisfying, it seems we need a more sophisticated fix for Bug 387687. 

BTW, It seems I found a test case for what fix for Bug 387687 does not work, but I still need some more investigation.
Rainer, I can't confirm your experience. It depends which type of messages you are receiving and which e-mail client your correspondents were using (and whether or not plain-text format is used).

I've noticed though that lines in plain-text mode may prematurely wrap but look normal later, apparently being a character or two off (especially when forwarding a plain-text message, even one I've wrote myself and thus should be sent with the same wrapping). Does the end-of-line maybe count towards the 72 limit in one context but not in the other? Just to throw in an idea...
(In reply to rsx11m from comment #7)
> Rainer, I can't confirm your experience.

referring to:

(In reply to Rainer Bielefeld from comment #6)
> I remember to have seen these single endless line quotes from time to time (rather seldom), that
> was ugly. Now we have a quote issue visible in nearby every reply - I think that is more ugly.
(In reply to rsx11m from comment #7)
> It depends which type of messages
I think my step by step instruction is clear. And more or less half of the 10 or so emails to which I replied with affected SM showed broken quote lines 

> I've noticed though that lines in plain-text mode may prematurely wrap but
> look normal later, 

g) I forgot to mention that I also can confirm Rüdiger Lahl's observation
   <http://shortlinks.de/4jo5>: Saving as Draft heals the viewing problem, 
   and also received message with quote looks fine (I only did 1 single test).
   But I don't think that that is sufficient. We also could argue "yes, web 
   page looks ugly in Browser, but you can simply print it,
   and it will look ok" :-/
See Also: → 387687
I also noted the changed behaviour since it now affects *every* reply. Currently, I solve this by reformatting (^R), but it's already getting on my nerves that at first it's *always* wrong. (And sometimes, I don't want to rewrap, for example for tabulated content.)

Opposed to that, those messages from broken MUAs that send complete paragraphs as single lines need to be reformatted/rewrapped anyway.

So I clearly prefer the way it was before.

If you're "open to better suggestions", this might be one: Do not wrap the lines after quoting, as long as they fit into the editor window's width.
(In reply to Tilmann Reh from comment #10)
> If you're "open to better suggestions", this might be one: Do not wrap the
> lines after quoting, as long as they fit into the editor window's width.

Great suggestion, but we sadly can't do that.

This is the new state (see comment #4):
  <body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;">
  <div class="moz-cite-prefix">
  On 23/07/2016 21:17, Magnus Melin wrote:<br>
  </div><span style="white-space: pre-wrap;">here is the quoted text<br>
=====================================^^^^^

The pre-wrap will wrap to the size of the surrounding container, which has width: 72ch.

Please understand that the wrapping is a visual effect, we don't insert hard returns. There are only two options in CSS: |white-space: pre;| and |white-space: pre-wrap;|, that is, the previous state that caused long lines (where people complained in bug 387687) and the present state which wraps but causes the ugly comb effect.

If I run into a situation like this, I use ThunderHTMLedit to change the |pre-wrap; width: 72ch| to |pre-wrap; width: 80ch|.

I know, this is quite frustrating, I just don't have an answer that will satisfy both uses. I am afraid though that when TB 52 is released in March 2017, we will get more people complaining.
Thanks for the detailed explanation.

If we only have these two options (with both having their pros and cons, as well as their audience), please make it an option that the user can set to his preference (about:config). After all, that will be the only way to satisfy both types of user.
(In reply to Tilmann Reh from comment #12)
> If we only have these two options (with both having their pros and cons, as
> well as their audience), please make it an option that the user can set to
> his preference (about:config).
Sadly, this is also difficult since the code that produces the "pre-wrap" is buried deep down in Mozilla core code that Thunderbird is using. It's hard but not impossible to get this option there. This may be surprising but can be explained by the original origin of both FF and TB in Netscape Navigator.

Masayuki-san, what you predicted in bug 387687 comment #33 has happened. People aren't happy with the pre-wrap in replies. Do you have any good idea of how to improve this?
Flags: needinfo?(masayuki)
Difficult issue. First, character width depends on each glyph. Although, I don't know how to split the quoted lines, including wider characters may cause wider line which may cause this issue even if we split lines less characters.

What I mentioned bug 387687 comment 33 is that nested quoted text may be displayed too narrow boxes.

For example, if a line is quoted 10 times, there are 10 quoted marks in plaintext mode or 10 colored lines in HTML mode. Then, these objects eat the space in the composer. Therefore, I guessed that each quoted lines should be in a block box which is guaranteed the minimum width.

So, I meant that I don't think that quoted lines need to be wrapped by its parent box (including by the window width) since I think that users do not read quoted text carefully at writing their thoughts. So, overflowing few words from quoted box won't be problem, and it's better than current behavior. You can see too tall quoted boxes due to wrapped each line too small amount of words when you shrink composing window of writing a reply.
Flags: needinfo?(masayuki)
Thanks for your reply.

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #14)
> So, I meant that I don't think that quoted lines need to be wrapped by its
> parent box (including by the window width) [*] since I think that users do not
> read quoted text carefully at writing their thoughts. So, overflowing few
> words from quoted box won't be problem, and it's better than current
> behavior. You can see too tall quoted boxes due to wrapped each line too
> small amount of words when you shrink composing window of writing a reply.

[*] Yes, indeed, quoted lines don't need to be wrapped exactly to the width of the parent box, but I think they *do* need to be wrapped to the width of the screen. That's what bug 387687 was fixing. There are cases (replying to non-f=f emails with all-in-one-line paragraphs) were you get one really long long long line with a scrollbar and that's really inconvenient, even if you don't read what your answering ;-)

So the "pre-wrap" is not a bad idea, but sadly it now wraps to the parent box which is only 72 characters wide. Is there any trick what would make the child wrap to the screen and not to parent box? I couldn't find any.

So

|Here is the parent|          |
|> and here is the quoted text|
| wrapped to the screen but   |
|not the parent               |

instead of the original state

|Here is the parent|          |
|> and here is the quoted text|wrapped to the screen but not the parent
|                             |

or the current state

|Here is the parent|          |
|> and here is the |          |
|quoted text       |          |
|wrapped to the    |          |
|screen but not the|          |
|parent            |          |
Flags: needinfo?(masayuki)
Thanks Jorg for the detailed description. My favourite would be the first of your examples (pre-wrap quotes at the screen width, as suggested before) - but if I had to chose between the latter two, I'd clearly prefer the "original state".

When just reading messages in any folder (regardless of using the 3-pane or a separate window), they are always wrapped at the screen width - regardless of quote levels. Wouldn't it be possible to use the same mechanism in the composer window as well? That would result in exactly the desired behaviour.
Ah, right.

I wonder, when it's in 72 characters mode, we can scan all line length of quoted text if they are in 72 characters. If the longest line is 72 characters or less, we can use |white-space: pre;|.
Flags: needinfo?(masayuki)
Blocks: 387687
The trick is this: We use a <div> instead of a <span> since this is a block element. We size the <div> to the width of the screen, that is 100 viewport units.

Works like a charm. Also see HTML test page included attached next.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8818226 - Flags: review?(masayuki)
Sorry, forgot to update the comment.
Attachment #8818226 - Attachment is obsolete: true
Attachment #8818226 - Flags: review?(masayuki)
Attachment #8818230 - Flags: review?(masayuki)
Sorry about the noise. I played around with it and it turns out that with 100vw units you get a scroll bar which isn't necessary. So 98vw are better.
Attachment #8818230 - Attachment is obsolete: true
Attachment #8818230 - Flags: review?(masayuki)
Attachment #8818241 - Flags: review?(masayuki)
Comment on attachment 8818241 [details] [diff] [review]
1288911-css-to-avoid-wraps.patch (v1c).

Hmm, perhaps, this patch works actually, but this method assumes that it's inserted at root scrollable element (i.e., <body>). So, this is very Thunderbird specific code, although, this method shouldn't run in Firefox because HTML editor in Firefox is never in plaintext mode. In other words, if this is called for small <div> which is scrollable, it causes unexpected rendering result. So, this should check if it's parent is <body>.

Could you use "width: 98vw;" only when newNode's parent is a <body>?
Thanks for the comment.

HTMLEditor::InsertAsPlaintextQuotation() and its friends are all part of nsIEditorMailSupport.idl:

In total there are 7 functions, five defined in the IDL (unless I miscounted):

1) IDL void pasteAsQuotation(in long aSelectionType);
calls
1a)  IDL PasteAsCitedQuotation()
1b)  PasteAsPlaintextQuotation()

2) IDL void insertTextWithQuotations(in DOMString aStringToInsert);

3) IDL nsIDOMNode insertAsQuotation(in AString aQuotedText);
calls
3a)  IDL InsertAsCitedQuotation()
3b)  InsertAsPlaintextQuotation()

None of these are used in Firefox to my knowledge. I have suggested before to move that stuff out of M-C and to C-C without looking at how feasible that is.

The code I want to change is in InsertAsPlaintextQuotation().

In bug 387687 we changed "pre" to "pre-wrap"
https://hg.mozilla.org/mozilla-central/rev/192a5ffbf501#l1.29
with the idea to wrap long lines to the screen. Sadly we didn't get this right, since we don't wrap to the screen but to the <body> which is generally limited to 72ch.

So here we correct this by wrapping to the screen with width:98vw. It could be 100vw, but 98vw avoids a scrollbar.

Can you please explain your comment #22. I don't see the necessity to check the parent. I can certainly do that, but could you first give me an example where that would be necessary. You could modify the attached example.

I don't think we have a "small <div> which is scrollable" in TB.
When you enable "middle click to paste", even in Firefox, you can use "paste as quoted text" with Ctrl + middle click.

In <textarea>, it's handled without HTMLEditRules, so, this is really safe.

In HTML editor like <div contenteditable>, the editor should be always not in plain text editor mode (except XUL add-on sets plaintext editor flag, although, I don't have any idea of the motivation).

So, it must be safe in Firefox.

How about using "Paste as quoted text" in quoted block in plaintext composer of Thunderbird? I'm not sure what occurs when I use "paste as quoted text" in quoted block, though.
Comment on attachment 8818241 [details] [diff] [review]
1288911-css-to-avoid-wraps.patch (v1c).

(In reply to Jorg K (GMT+1) from comment #23)
> None of these are used in Firefox to my knowledge.
Oh, I was wrong. I learn something new every day ;-)
http://searchfox.org/mozilla-central/rev/36bfd0a8da5490274ca49c100957244253845e52/editor/libeditor/EditorEventListener.cpp#729
in EditorEventListener::HandleMiddleClickPaste()
Also called in PasteQuotationCommand::DoCommand().

Just for the record, the "middle button paste" is enabled with this pref: middlemouse.paste

I'll do some experiments and revise my patch as required.
Attachment #8818241 - Flags: review?(masayuki)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #24)
> How about using "Paste as quoted text" in quoted block in plaintext composer
> of Thunderbird?
That comment is 100% correct. When using "Paste As Quotation" inside some other block, you get that block ripped apart by the <div> I'm inserting which is unexpected.

As you suggested, it's a good idea to only do this when we're inserting into the body. New patch coming.
Since it's a little hard to know where I'm inserting, I'm always using a <span> now and then check its parent. I use |display: block;| to make it big ;-)
Attachment #8818241 - Attachment is obsolete: true
Attachment #8818664 - Flags: review?(masayuki)
Comment on attachment 8818664 [details] [diff] [review]
1288911-css-to-avoid-wraps.patch (v2).

>+    if (parent && parent->IsHTMLElement(nsGkAtoms::body)) {
>+      newNode->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
>+        NS_LITERAL_STRING("white-space: pre-wrap; display: block; width: 98vw;"),

nit: over 80 characters here, perhaps, it should be:

>+        NS_LITERAL_STRING(
>+          "white-space: pre-wrap; display: block; width: 98vw;"),
Attachment #8818664 - Flags: review?(masayuki) → review+
Thanks for the review.

I think we can live with a 81-character-long line for better legibility given that the same file already has:
  nsCOMPtr<nsIClipboard> clipboard(do_GetService("@mozilla.org/widget/clipboard;1", &rv)); - 90
    NS_ASSERTION(!aInsertHTML, "InsertAsCitedQuotation: trying to insert html into plaintext editor"); - 102
  DataTransfer::Cast(aDataTransfer)->GetDataAtNoSecurityCheck(aType, aIndex, getter_AddRefs(variant)); - 102
            selOffset = outVisOffset;  // PriorVisibleNode already set offset to _after_ the text or ws - 103

I think the TTY is slowly going out of fashion ;-)

===

Dear Sheriff,

no try run here since this code is only run in Thunderbird.
Keywords: checkin-needed
status-seamonkey2.45: unaffected → ---
status-seamonkey2.46: affected → ---
status-seamonkey2.47: affected → ---
status-thunderbird50: affected → ---
Component: Composition → Editor
Product: MailNews Core → Core
Version: 49 → 49 Branch
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3c63b6f02c2
Use CSS trick to avoid unwanted line wraps in quotes. r=masayuki
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d3c63b6f02c2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8818664 [details] [diff] [review]
1288911-css-to-avoid-wraps.patch (v2).

Before I fill in the request comment, let me explain the situation.

The code in question is almost only run in Thunderbird. "Almost", since Masayuki-san pointed out that if you set the preference "middlemouse.paste" in Firefox and then press <ctrl><middle-button>, you can also insert "quoted text", that is, text from the clipboard preceded by a ">" in Firefox. Hands up all people who even knew that ;-)

In bug 387687 we changed the CSS formatting of this text from "pre" to "pre-wrap". We did that, since pasting very long lines and inserting them in a "pre" block causes a very long line on the screen without wrapping, and you need to use the scroll bar to see it all. That was very inconvenient for users. With "pre-wrap", the inserted text is wrapped, but sadly not to the screen as intended, but to the <body>. And in Thunderbird, in the so-called "plain text" composition, which is in fact a HTML composition with a body that has
<body style="white-space: pre-wrap; width: 72ch;">,
the inserted lines were longer than 72 characters causing an ugly "comb effect".

Let me show you two examples, the rightmost "|" denotes the edge of the screen, the leftmost "|" denotes the body width.

Initial state before bug 387687:

|Here is the body  |          |
|> and here is the quoted text|not wrapped at all causing a long line.
|                  |          |
<---- scroll bar ---------------------------------------------------->

State after bug 387687 before fixing this bug here. See the "comb" effect, one "full" quoted line and one "dangling bit" on the next line.

|Here is the body  |          |
|> and here is the |          |
|quoted text       |          |
|> and here is the |          |
|quoted text       |          |
|                  |          |

New desired state after fixing this bug.

|Here is the parent|          |
|> and here is the quoted text|
|> and here is the quoted text|
|                  |          |

You can clearly see that the new state is best for both examples, the first with a very long line and the second with a quote line which is just wider than the body but usually fits nicely onto the screen.

Since Thunderbird is only doing ESR releases, it would be great to uplift this to mozilla52, currently in Aurora. Otherwise the Thunderbird release manager for TB 52, that's me, would have to put this patch onto a special branch to include it into TB. That's been done before for TB 38 and TB 45, but we'd like to avoid extra work as much as we can.

Convinced yet? I'll fill in the details now ;-)

Approval Request Comment
[Feature/Bug causing the regression]: Bug 387687
[User impact if declined]: Annoying "comb" effect when replying to e-mail in the so-called plain-text mode.
Read user comment #10.
[Is this code covered by automated tests?]: No. It affects the display only.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]:
As detailed above, we're changing the CSS for the display of a quoted text from |pre-wrap| to |pre-wrap| with |display: block; width:98vw|. We only do that when inserting at the top-most (body) level. It's a feature hardly ever used in Firefox but very frequently used in Thunderbird.
[String changes made/needed]: None.
Attachment #8818664 - Flags: approval-mozilla-aurora?
Comment on attachment 8818664 [details] [diff] [review]
1288911-css-to-avoid-wraps.patch (v2).

css tweak for quoted text, take in aurora52
Attachment #8818664 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I read about a WORKAROUND  on de.comm.software.mozilla.mailnews ("Seamonkey 2.46 - Kammquoting im Editor").

You will have to edit User Profile folder/chrome/userContent.css

Add:

span[_moz_quote=true] {
     display: block;
     width: 98vw;
   }

If you do not have a file userContent.css, you find a sample file "userContent-example.css" in the chrome folder.
That's the fix we landed on TB 52 and TB 53. Sadly it has discovered another problem, bug 1328093.
Depends on: 1328093
Depends on: 1330796
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: