Quoted text mishandled with wrap_to_window_width

RESOLVED FIXED in Firefox 48

Status

()

Core
Editor
RESOLVED FIXED
13 years ago
a year ago

People

(Reporter: Mike Cowperthwaite, Assigned: Jorg K (GMT+2))

Tracking

Trunk
mozilla48
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

13 years ago
If the preference  mail.compose.wrap_to_window_width  is set to True, and I 
reply to a message (plain text), then (1) the quoted text is shown in black, 
rather than blue; and (2) the caret position in the body is placed at the 
beginning of the text, rather than after the quote.

Long quoted lines do wrap to the window (tho it's no longer clear to look at 
them whether they are quoted correctly).  In fact, this might be why symptom (1) 
exists.  And, quoted lines are correctly space-stuffed when the message is sent 
as format=flowed.

Setting this preference to True when send-as-f=f is enabled might reduce the 
number of complaints about "My text isn't wrapping!" since the text would reflow 
within the window.  However, people will surely complain about these two minor 
symptoms instead (except for those who want to turn off quote coloring in the 
compose window).

  Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113
Product: MailNews → Core
(Reporter)

Updated

11 years ago
Summary: Quoted text mishandled with wrap-to-window-width → Quoted text mishandled with wrap_to_window_width
(Reporter)

Comment 1

11 years ago
*** Bug 359633 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 2

11 years ago
(In reply to comment #0)
> (2) the caret position in the body is placed at the 
> beginning of the text, rather than after the quote.

As I realized reading the dupe, this is only an issue if the replying identity is configured to put reply below the quote; above-the-quote replies place the caret correctly.
(Reporter)

Comment 3

10 years ago
I've discovered that this pref is used to initialize the compose window in the first two lines of nsHTMLEditor::InsertAsPlaintextQuotation(), completely bypassing the smart quoting -- the quoted text is just part of the message body, broken into individual lines so that they can't even take intelligent advantage of wrap-to-window-width.

This has an implication for f=f -- it's possible, with this pref, to simply type in a line beginning with '>' and have it treated as a quote when it gets sent out; the line isn't space-stuffed.  (However, space-stuffing of lines beginning with spaces still occurs.)

See also bug 368758.
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
Filter on "Nobody_NScomTLD_20080620"
QA Contact: esther → composition
Product: Core → MailNews Core

Comment 6

5 years ago
Yes, too bad quote colorization doesn't work with mail.compose.wrap_to_window_width . It would be a very useful feature otherwise for those who often have to reply to mails from Apple Mail and Webmail users.

Comment 7

5 years ago
TB 16 -- If I set the pref mentioned to TRUE, the cursor is placed at the top of the reply window above the attribution line which includes the quoted text below, etc. Change the pref to FALSE and cursor is placed below the quoted text as per the compose preference.

Comment 8

a year ago
This is also present in Thunderbird 38 and 45b on MacOS X, and on Windows 7.

(In reply to Mike Cowperthwaite from comment #0)
> If the preference  mail.compose.wrap_to_window_width  is set to True, and I 
> reply to a message (plain text), then (1) the quoted text is shown in black, 
> rather than blue;

Actually it's changed from blue to whatever the default color set in Preferences--> Display--> Formatting--> Fonts & Colors--> Colors--> Text is, i.e. browser.display.foreground_color.

See also bug 578657, which should probably be marked a duplicate of this one.

Updated

a year ago
Duplicate of this bug: 1246479

Updated

a year ago
Duplicate of this bug: 578657

Updated

a year ago
Duplicate of this bug: 931774
The issue, as mentioned in bug 931774, is here: <http://hg.mozilla.org/mozilla-central/annotate/95130f72756e/editor/libeditor/html/nsHTMLDataTransfer.cpp#l1776>. Quoth Magnus: "we don't go on to set the _moz_quote attribute for the span (_moz_quote is used to ignore)".

Jorg, you like editor stuff. What do you think we should do here? I've half a mind to remove the "mail.compose.wrap_to_window_width" pref, since I think we should be trying to cut back on the number of codepaths we need to test in the editor, but I'm not totally clear on why people would set this pref in the first place...
Flags: needinfo?(mozilla)

Comment 13

a year ago
(In reply to Jim Porter (:squib) from comment #12)
> but I'm not totally clear on why people would set this pref
> in the first place...

I gave one use case in comment #6 : when replying to mails sent by broken clients which put their entire mail into a single line. Without mail.compose.wrap_to_window_width, you have to scroll back and forth to read the other guy's text. Of course, in a perfect world, everybody would stick to a reasonable line length, but the world is not perfect, unfortunately :-(
(Assignee)

Comment 14

a year ago
I'm not sure where the interest for a bug from 2004 comes from ;-) The serialisers are unowned and it will be hard to get a review, even it someone proposes a patch. But I'll answer it anyway.

I did an experiment: Reply to a mail with a very long line (990 chars long) with and without (default) the preference set and then looked at the DOM with a the DOM inspector (and the very handy Element inspector).

Without the preference set, the quote is wrapped to 72 characters and wrapped into a <span _moz_quote="true" style="white-space: pre;">. This causes the blue colour and it causes attachment reminder keywords to be ignored:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4775

With the preference set, the quote isn't wrapped in a span, so the colour is wrong (bug 578657) and attachment reminders show (bug 931774). The reply is wrapped to 72 characters just the same.

So setting the preference doesn't do any good so far.

As I said, my long line e-mail was always wrapped to
> long line long line long line long line long line long line long line
> long line long line long line long line long line long line long line
and this wrapping happens here:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#2894

So before discussing it any further, I'd like to see an e-mail where replying with this preference set actually helps. A long line alone cannot be the problem, since I can replay to an e-mail with a 990 character long line and it still gets wrapped in the reply.

So Alain, can you please attach such a message for further investigation. You said in comment #6 that they originate from Apple Mail or webmail users.
Flags: needinfo?(mozilla) → needinfo?(mozilla)

Comment 15

a year ago
Created attachment 8736335 [details]
Example mail with long lines that shows the problem

I quickly cooked up an example to show the problem.

Replying to it without mail.compose.wrap_to_window_width shows the replied-to text in blue, but indeed in one single long unwrapped line.

Replying to it with mail.compose.wrap_to_window_width wraps the replied to line, but text is no longer highlighted.

As this mail was entered from scratch, it is highly unlikely that something other than the long line triggered the issue.
Flags: needinfo?(mozilla)

Comment 16

a year ago
O, and where does the interest come from? Maybe because the bug is still *not fixed*, like so many others. If you don't like activity on old bugs, maybe, just maybe... fix them? :-)
(Assignee)

Comment 17

a year ago
Thanks. I can see it now. My test was with
  Content-Type: text/plain; charset=windows-1252; format=flowed
but your test doesn't have the flowed bit
  Content-Type: text/plain; charset=utf-8

I can indeed see the long ugly line, in fact, I've seen cases like this before but very paid much attention to them.

I think the option is badly named and I agree with Jim that it would be best to remove it.

The problem you mention should be fixed some other way. The reply with the option set doesn't result in a long line because the reply is not wrapped into the said <span _moz_quote="true" style="white-space: pre;">. Preformatted blocks are not wrapped to the screen, non-preformatted blocks are.

I'd much rather find a different solution to this problem in a new bug with the summary:
  When replying to a non-flowed plaintext message with a long line, the reply is not wrapped.
(Assignee)

Comment 18

a year ago
I bet that this complains about the same issue: Bug 458746 comment #3.

As for fixing the long lines in the reply: What's wrong with selecting the quote and using "Edit > Rewrap" (or <ctrl>R).
(Assignee)

Comment 19

a year ago
And here another one: Bug 387687.
So conclusion: mail.compose.wrap_to_window_width should be removed.
Immediate solution for long lines: Rewrap.
Otherwise: Lobby bug 387687.
Jorg K, problem with Edit | Rewrap: The user has to know about it (but same with a pref). More importantly Edit | Rewrap has the same bug. Reply to the example message attached here, then try the command. You will see that there is only one > for the first line, but not for following lines.

Compare comment 3, first paragraph.

Simply put, neither of these options can properly handle quotes. That's the bug here.

Of course, removing this feature is still an option.
IMHO, when we convert a plaintext (not f=f) message for the plaintext editor, or when sending, we should always break it down to 80 chars per line, and insert > after each inserted linebreak (if the line has a > marker).

"wrap to window width" only makes sense in HTML and f=f, where that's the natural behavior, but it makes no sense in non-f=f plaintext.
(Assignee)

Comment 22

a year ago
Created attachment 8736422 [details]
Left: Manual rewrap, right: with wrap_to_window_width set.

Ben, sadly I don't get what you're saying. On the left you see the manually rewrapped reply with wrap_to_window_width set to 'false'. On the right you see the reply that gets generated with wrap_to_window_width set to 'true'. As we know, that's missing the span tag.

I much prefer the left option.

IMHO bug 387687 is still valid, so we should have the discussion there. This bug here is about fixing the problems with wrap_to_window_width or removing that option.
(Assignee)

Comment 23

a year ago
Ben replied to me in a private message and said:
===
I think I'm trying to say the same thing as you do. I agree with you: the one on the left side of your screenshot is correct, the right side is a bug. If the whole purpose of the pref is to produce the right side result, then the pref should be removed.
===

So we all agree. The pref produces the right side and should therefore be removed.

Jim, are you planning on doing some clean-up here?
Jorg: I don't know a whole lot about the composer, and I'm not likely to have much free time in the near future, but if I got my other TB patches finished, I might take a look at this. I was mainly bringing this up due to a post about the bug in mozilla.support.thunderbird, and thought you might have some insight, since you've been making a lot of improvements to composition lately.
(Assignee)

Comment 25

a year ago
Jim, I didn't know much about this preference, but as you saw, I smartened up and even Ben chipped in. As you know, but editor and serialisers are unowned and it's hard to get reviews. Sadly the serialisers live in M-C but a large part of the code is only used by C-C. That makes changing things even harder, since there is no M-C interest.

A "dormant" unused preference doesn't cause any harm, unless of course people start using it and complain.
(Assignee)

Comment 26

a year ago
Created attachment 8736479 [details] [diff] [review]
Removing wrap_to_window_width and friends. RIP.

Jim, a patch is dead easy, here you go. Now find a reviewer ;-)

Yes. There is a C-C part to it, you'd need to remove the preference.
Well, I dunno if I'd land that patch yet. I think we should try to fix the issue that it works around (bug 387687?) first and then remove the pref.
(Assignee)

Comment 28

a year ago
(In reply to Jim Porter (:squib) from comment #27)
> Well, I dunno if I'd land that patch yet. I think we should try to fix the
> issue that it works around (bug 387687?) first and then remove the pref.
Well, people could use the rewrap. I'd have to try whether this could also be a fix:

nsPlaintextEditor.cpp:
  if (aWrapColumn > 0)        // Wrap to a fixed column
  {
    styleValue.AppendLiteral("white-space: pre-wrap; width: ");
    styleValue.AppendInt(aWrapColumn);
    styleValue.AppendLiteral("ch;");
  }
  else
    styleValue.AppendLiteral("white-space: pre-wrap;");  <<=== use pre-wrap here.

I can look into bug 387687 on the weekend.
(Assignee)

Comment 29

a year ago
Created attachment 8736591 [details]
Picture showing effect of tweak described in comment #29

That didn't work, bug switching to "pre-wrap" here does:
https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLDataTransfer.cpp#1798

No idea why the wrapping was turned off originally.
(Assignee)

Updated

a year ago
Depends on: 387687

Updated

a year ago
Assignee: nobody → mozilla
Comment on attachment 8736479 [details] [diff] [review]
Removing wrap_to_window_width and friends. RIP.

I think removing the pref is the right thing to do.
And this code patch seems good to me.
r+ from me, FWIW (I've been implementing a good portion of the plaintext handling in TB, 15 years ago(
Attachment #8736479 - Flags: review+

Updated

a year ago
Attachment #8736479 - Flags: feedback+
(In reply to Jim Porter (:squib) from comment #27)
> Well, I dunno if I'd land that patch yet. I think we should try to fix the
> issue that it works around (bug 387687?) first and then remove the pref.

I don't see the relation. If anything, users who set this pref here are even more (!) affected by bug 387687, or not? Meaning that if we remove the pref, it will get better for them. Either way I think they are separate issues.

The pref here was just misguided and produces bad results. I don't think many people use it, either. We should just get rid of it, IMHO.
I could be wrong, but I think the reason people flip this pref is because it makes quotes of poorly-formatted emails easier to read while composing at reply. Specifically, it seems to prevent horizontal scrollbars. I'm not sure if it fouls things up when you send the reply, though.

If it's possible for us to prevent horizontal scrollbars in plain text replies, I think we should do that in all cases (probably just for display in the composer). I'd be worried about messing with the actual formatting we send out, since it could cause problems for code patches sent inline in bodies (as the Linux kernel devs do).
(Assignee)

Comment 33

a year ago
The proper fix is to fix this bug (remove the preference) and bug 387687.

Removing the faulty preference will do not harm but will stop complaints. Fixing bug 387687 will help users who previously used the preference causing unwanted side-effects.

We won't mess with any formatting, we will still send the exact same content, only during the composition phase, fixing bug 387687 will do a wrap, as was originally intended (!!!), see bug 387687 comment #26.
(Assignee)

Comment 34

a year ago
(In reply to Jim Porter (:squib) from comment #32)
> I could be wrong, but I think the reason people flip this pref is because it
> makes quotes of poorly-formatted emails easier to read while composing at
> reply. Specifically, it seems to prevent horizontal scrollbars.
Yes. But those people will get the correct result when bug 387687 is fixed at the same time.
Then I'm happy, and I leave it in your capable hands. I just wanted to be sure we weren't regressing anyone who had some reason to flip the pref. Thanks for the details!
(Assignee)

Comment 36

a year ago
Sorry, I've changed my mind 100%.

As per bug 387687 comment #28 we need this option since there is no way to fix the other bug in a way that will keep everyone happy.

A new patch is coming to do the wrapping to the screen width correctly; that will involve a change as mentioned in comment #29.
> I think the reason people flip this pref is because it makes quotes of poorly-formatted emails easier to read while composing at reply.

Agreed. And notably only in the plaintext composer, because the HTML composer already wraps the same line.

> If it's possible for us to prevent horizontal scrollbars in plain text replies, I think we
> should do that ... for display in the [plaintext] composer).

Agreed.

As mentioned in bug 387687 comment 31, I'd propose:
1) we fix up the display in our plaintext composer
2) remove the pref, and
3) otherwise leave things as-is.
(Assignee)

Comment 38

a year ago
Created attachment 8737267 [details] [diff] [review]
Fixing wrap_to_window_width: Not set ==> long lines. Set ==> long lines wrapped to screen.

OK, here I fixed wrap_to_window_width so it does what is says it does.

If not set, you get a long line in the reply, since the quote is wrapped in a "white-space: pre;".

If set, you get the line wrapped to the screen, since the quote is wrapped in a "white-space: pre-wrap;". Attachment 8736591 [details] shows the effect.

So the option will do 100% what is says that it will do. The user can choose whether to conserve the long lines even in the screen display, or whether they prefer to have them wrapped.

In both cases they are wrapped into a span so the quote looks blue and attachment reminder keywords in the quote are ignored.

The sent content won't be changed, only the representation when editing.
Attachment #8736479 - Attachment is obsolete: true
Attachment #8737267 - Flags: feedback?(squibblyflabbetydoo)
Attachment #8737267 - Flags: feedback?(ben.bucksch)
(Assignee)

Comment 39

a year ago
(In reply to Ben Bucksch (:BenB) from comment #37)
> 1) we fix up the display in our plaintext composer
Yes. But only if the pref is set so we don't force new behaviour onto everyone. People who are already using the preference, like Alain (comment #6, comment #13, etc.), will be delighted to get the correct colour now.

> 2) remove the pref, and
No, we need it to drive the display in the plaintext composer. And it will do exactly what it says.

> 3) otherwise leave things as-is.
Yes.
I'm getting a little confused about all the different scenarios (different source emails, the prefs, manual rewrap, and plaintext vs. HTML editor). But:

1) If we currently show 1000 char lines in the plaintext editor, with horizontal scrollbars, that's no good and we should break these lines. For everybody. We should not encourage people to set the pref, rather just make it the default. Everybody hates horizontal scrolling.

Given that you're not changing the output format in the sent email, I see no problem with enabling that. We're essentially fixing a bug.

Just remove the pref. We need to get rid of old nonsense.

2) Please make extra sure that the code you use is used only in this scenario, only for quotes inserted in the plaintext editor. The function is called InsertAsPlaintextQuotation(), so that should be fine, but please double-check.

Ben
(Assignee)

Comment 41

a year ago
(In reply to Ben Bucksch (:BenB) from comment #40)
> 1) If we currently show 1000 char lines in the plaintext editor, with
> horizontal scrollbars, that's no good and we should break these lines. For
> everybody. We should not encourage people to set the pref, rather just make
> it the default. Everybody hates horizontal scrolling.
OK.
 
> Given that you're not changing the output format in the sent email, I see no
> problem with enabling that. We're essentially fixing a bug.
> 
> Just remove the pref. We need to get rid of old nonsense.
I'm OK with setting the preference by default. Then people who don't want the new, no-nonsense behaviour can switch it off and return to the "old nonsense". We can do this in the other bug and we can even expose the preference in the user interface.

I'm also almost OK with removing the pref, but:
You mentioned ASCII art. If I now wrap the ASCII art to the window, some people won't be delighted. You must admit that attachment 8736591 [details] doesn't look great. People should have the option to see the long lines with a long scrollbar.

So why would I remove a preference and get rid of the option of having the best of both worlds.

If you look at the patch, I'm already removing mDontWrapAnyQuotes, the nonsense that permitted quotes not to be wrapped in <span>s, that I agree is a bug that should go. But the preference can be maintained for the well-defined "wrap to screen".

> 2) Please make extra sure that the code you use is used only in this
> scenario, only for quotes inserted in the plaintext editor. The function is
> called InsertAsPlaintextQuotation(), so that should be fine, but please
> double-check.
I'll check.
(In reply to Jorg K (GMT+1) from comment #41)
> I'm also almost OK with removing the pref, but:
> You mentioned ASCII art. If I now wrap the ASCII art to the window, some
> people won't be delighted. You must admit that attachment 8736591 [details]
> doesn't look great. People should have the option to see the long lines with
> a long scrollbar.

This only affects you when you're composing a message with a big piece of ASCII art in a reply, right? If you're merely reading the message, it should still look good. If you *send* the quoted ASCII art, it should still look good. If you're trying to create your own big ASCII art, we wrap your text no matter what (right?).

So the only part we'd be changing is display of big ASCII art in a quote *while composing*. This seems like a small enough issue that I'd still be ok with removing the pref. It's true that there's a legitimate reason to use the old way, but it's such a small use case that I'm not convinced it's worth the maintenance burden.
(Assignee)

Comment 43

a year ago
OK, you convinced me.

When you view a long line before answering, it gets wrapped.
If you write a plain text e-mail, it also gets wrapped.
The only case that doesn't get wrapped is a reply to an e-mail with a long line that was sent non f=f. But only the quoted part. The actual reply gets wrapped.

I'll attach another patch, same as the one before, but with the "white-space: pre-wrap;".

So we're doing Ben's approach:
1) we fix up the display in our plaintext composer
2) remove the pref, and
3) otherwise leave things as they are.

Let's do it all in the one bug, so I don't have to go and beg for review twice, OK?

All happy?
Sounds good to me!
Comment on attachment 8737267 [details] [diff] [review]
Fixing wrap_to_window_width: Not set ==> long lines. Set ==> long lines wrapped to screen.

Review of attachment 8737267 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see anything wrong with this patch, although I think the plan is to go a different route with this, correct?
Attachment #8737267 - Flags: feedback?(squibblyflabbetydoo) → feedback+
(Assignee)

Comment 46

a year ago
Thanks for the f+, yes, as per comment #42 etc. I'll slash some more. Coming up right now.
(Assignee)

Comment 47

a year ago
Created attachment 8737377 [details] [diff] [review]
Removing wrap_to_window_width and friends. Also wrap long lines in replies.

OK, this is now the combination of both previous patches.

mDontWrapAnyQuotes, mWrapToWindow and pref wrap_to_window_width are all removed.

Additionally, quotes are now wrapped with "white-space: pre-wrap;"

I've tested these cases:
mailnews.wraplength = 72 (default):
The plaintext body has "white-space: pre-wrap; width: 72ch;".
Any quote sitting inside has "white-space: pre-wrap;" and will also be wrapped to 72.

mailnews.wraplength = 0:
The plaintext body has "white-space: pre-wrap;" and will be wrapped to the screen.
Any quote sitting inside has "white-space: pre-wrap;" and will also be wrapped to the screen.

If we all agree now, I will run the necessary M-C try server runs and then look for a willing reviewer, either Neil Deakin or Masayuki Nakano.
Attachment #8737267 - Attachment is obsolete: true
Attachment #8737267 - Flags: feedback?(ben.bucksch)
Attachment #8737377 - Flags: feedback?(squibblyflabbetydoo)
Attachment #8737377 - Flags: feedback?(ben.bucksch)
Comment on attachment 8737377 [details] [diff] [review]
Removing wrap_to_window_width and friends. Also wrap long lines in replies.

Looks good to me. Even better than the first.

r+. This is under the condition that only (the pref is removed and) the plaintext editor display changes, by wrapping long lines to window width (for all users), but that the output that we send on the wire does not change at all.

This might be simply because the output serializer does not support pre-wrap. That would mean we'd regress as soon as such support is added. Please verify whether that's the case, and it would be nice to have a strategy in place for that, e.g. to distinguish between editor and send, or to have the editor reformat on send, or to add a special class to the span that can be a trigger for the serializer (and add a comment there) etc.. I don't know whether any of this is true, just mentioning it in case.
Attachment #8737377 - Flags: review+
Attachment #8737377 - Flags: feedback?(ben.bucksch)
Attachment #8737377 - Flags: feedback+
> When you view a long line before answering, it gets wrapped.
> If you write a plain text e-mail, it also gets wrapped.
> The only case that doesn't get wrapped is a reply to an e-mail with a long line that was
> sent non f=f. But only the quoted part.

That nice summary makes clear that our old behavior didn't make sense.
(Assignee)

Comment 50

a year ago
(In reply to Ben Bucksch (:BenB) from comment #49)
> That nice summary makes clear that our old behavior didn't make sense.
Indeed.

I have confirmed that despite wrapping to the window, (quote) "the output that we send on the wire does not change at all".

Although I doubt that this interferes with anything in Firefox, I sent off this try run. Better safe than sorry:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=04909537dd05

Jim, who would be an adequate reviewer for this? I suggested Neil Deakin or Masayuki Nakano. Masayuki has an interest in e-mail, especially plain text e-mail since Japanese use it a lot.
(Assignee)

Comment 51

a year ago
Comment on attachment 8737377 [details] [diff] [review]
Removing wrap_to_window_width and friends. Also wrap long lines in replies.

Masayuki-san, would you be able to review this? We're basically removing the mail.compose.wrap_to_window_width preference from the plain text serialiser and at the same time tweaking nsHTMLDataTransfer.cpp.

This code has traditionally lived in M-C Core::Serialisers and Core::Editor although it is not used by FF.

I've read about the refactoring you're planning in bug 1260651. What would be more beneficial for the code base would be to identify the parts of Core::Serialisers and Core::Editor only used by TB, remove them from M-C Core and transfer them into the responsibility of the TB team.
Attachment #8737377 - Flags: review?(masayuki)

Comment 52

a year ago
(In reply to Jorg K (GMT+2) from comment #51)
> Core::Serialisers and Core::Editor only used by TB,

... and SeaMonkey.  ;-)
Comment on attachment 8737377 [details] [diff] [review]
Removing wrap_to_window_width and friends. Also wrap long lines in replies.

>-    // turn off wrapping on spans
>+    // Allow wrapping on spans so long lines get wrapped to the screen.
>     newNode->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
>-                     NS_LITERAL_STRING("white-space: pre;"), true);
>+                     NS_LITERAL_STRING("white-space: pre-wrap;"), true);

Although, this seems a risky change but I support this change because I don't like the non-wrapped text.

>   // and now we're ready to set the new whitespace/wrapping style.
>-  if (aWrapColumn > 0 && !mWrapToWindow)        // Wrap to a fixed column
>+  if (aWrapColumn > 0)        // Wrap to a fixed column

I'd like you to move following line's |{| to the end of this line.  This is not our coding rule, although, it's not your fault.

>   {
>     styleValue.AppendLiteral("white-space: pre-wrap; width: ");
>     styleValue.AppendInt(aWrapColumn);
>     styleValue.AppendLiteral("ch;");
>   }
>-  else if (mWrapToWindow || aWrapColumn == 0)
>+  else
>     styleValue.AppendLiteral("white-space: pre-wrap;");
>-  else
>-    styleValue.AppendLiteral("white-space: pre;");

And here too. use |} else {|.

But is aWropColum never negative? I don't think so. This is exposed to chrome script. So, I don't agree with this change. Could you keep current behavior if aWrapColumn is nagative?


And I don't have rights to review dom/base/nsPlainTextSerializer.*...
Attachment #8737377 - Flags: review?(masayuki) → review-
(Assignee)

Comment 54

a year ago
Created attachment 8737614 [details] [diff] [review]
Removing wrap_to_window_width and friends. Also wrap long lines in replies. (v2)

I've fixed the braces as requested by Masayuki in comment #53. I've also maintained the previous behaviour for wrapWidth == -1, since it's documented here:
http://mxr.mozilla.org/mozilla-central/source/editor/nsIPlaintextEditor.idl#77

Although the code lives in dom/base and editor/libeditor, it is not executed in FF.

Changes in dom/base:
The preference (mail.compose.wrap_to_window_width) the TB team would like to remove is not even present in FF. Its removal has no impact on FF without the preference all the code runs with the previous default value 'false'. See try run from comment #50.

Changes in editor/libeditor/nsHTMLDataTransfer.cpp:
nsHTMLEditor::InsertTextWithQuotations
  http://mxr.mozilla.org/mozilla-central/source/editor/nsIEditorMailSupport.idl#40

and nsHTMLEditor::InsertAsPlaintextQuotation
  called from nsHTMLEditor::InsertAsCitedQuotation (from nsIEditorMailSupport.idl)
  called from nsHTMLEditor::InsertAsQuotation (from nsIEditorMailSupport.idl)
  called from nsHTMLEditor::InsertTextWithQuotations (from nsIEditorMailSupport.idl)
  called from nsHTMLEditor::PasteAsPlaintextQuotation/nsHTMLEditor::PasteAsQuotation
  (from nsIEditorMailSupport.idl)

are not used in FF. They should one day be removed from core and moved into the responsibility of the mailnews maintainers.

Change to nsPlaintextEditor::SetWrapWidth:
Previous behaviour was maintained as if mail.compose.wrap_to_window_width set to 'false', which is the FF default.

Conclusion: FF not affected by any of the changes.
Attachment #8737377 - Attachment is obsolete: true
Attachment #8737377 - Flags: feedback?(squibblyflabbetydoo)
Attachment #8737614 - Flags: review?(ehsan)
(Assignee)

Comment 55

a year ago
Comment on attachment 8737614 [details] [diff] [review]
Removing wrap_to_window_width and friends. Also wrap long lines in replies. (v2)

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #53)
> And I don't have rights to review dom/base/nsPlainTextSerializer.*...

Actually, this is mainly editor/

Surely you can check that the few changes in the plain text serializer don't do any damage to Firefox. The logic surrounding mDontWrapAnyQuotes, which is 'false' for Firefox is removed, that's all.
Attachment #8737614 - Flags: review?(masayuki)
(Assignee)

Updated

a year ago
Component: Composition → Editor
Product: MailNews Core → Core
(Assignee)

Updated

a year ago
Severity: minor → normal
Comment on attachment 8737614 [details] [diff] [review]
Removing wrap_to_window_width and friends. Also wrap long lines in replies. (v2)

Review of attachment 8737614 [details] [diff] [review]:
-----------------------------------------------------------------

The removal of the pref looks fine, but this bug claims that this change is only about removing the pref, whereas the patch is changing the behavior in the place noted below.  r- based on that.  r+ with that change reverted, no need to ask for another review!

::: dom/base/nsPlainTextSerializer.cpp
@@ +1582,1 @@
>            && mEmptyLines >= 0 && str.First() == char16_t('>'))) {

Nit: please remove the braces around |mSpanLevel > 0| and move the next line up to the continuation of this one.

::: editor/libeditor/nsHTMLDataTransfer.cpp
@@ +1792,2 @@
>      newNode->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
> +                     NS_LITERAL_STRING("white-space: pre-wrap;"), true);

Why this change?
Attachment #8737614 - Flags: review?(ehsan) → review-
(Also if multiple people need to look at a patch, it would be *really* helpful if you mention clearly who needs to look at what when asking for review.  I looked at the whole patch...)
(Assignee)

Comment 58

a year ago
Thanks for the review.

(In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment #56)
> ::: editor/libeditor/nsHTMLDataTransfer.cpp
> @@ +1792,2 @@
> >      newNode->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
> > +                     NS_LITERAL_STRING("white-space: pre-wrap;"), true);
> 
> Why this change?
Since we remove the wrap_to_window_width preference which lets the user wrap long lines to the screen in a crude and erroneous way, we now need to change the wrapping behaviour in general. This hunk was for bug 387687. I know you hate mixing issues, so I will present it again in the other bug.

(In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment #57)
> (Also if multiple people need to look at a patch, it would be *really*
> helpful if you mention clearly who needs to look at what when asking for
> review.  I looked at the whole patch...)
Thanks for looking at the whole thing.
(Assignee)

Comment 59

a year ago
Created attachment 8738046 [details] [diff] [review]
Removing wrap_to_window_width and friends (v3).

Carrying forward Ehsan's conditional r+.
I fulfilled the condition and removed the hunk he questioned from the patch.
Attachment #8737614 - Attachment is obsolete: true
Attachment #8737614 - Flags: review?(masayuki)
Attachment #8738046 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 60

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/26faa5d26ab7
Keywords: checkin-needed

Comment 61

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/26faa5d26ab7
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.