Closed
Bug 233705
Opened 21 years ago
Closed 9 years ago
Quoted text mishandled with wrap_to_window_width
Categories
(Core :: DOM: Editor, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: mcow, Assigned: jorgk-bmo)
References
Details
Attachments
(4 files, 4 obsolete files)
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
Updated•20 years ago
|
Product: MailNews → Core
Reporter | ||
Updated•18 years ago
|
Summary: Quoted text mishandled with wrap-to-window-width → Quoted text mishandled with wrap_to_window_width
Reporter | ||
Comment 1•18 years ago
|
||
*** Bug 359633 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 2•18 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•18 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.
Comment 4•17 years ago
|
||
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
Updated•16 years ago
|
Product: Core → MailNews Core
Comment 6•13 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•12 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•9 years 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.
Comment 12•9 years ago
|
||
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•9 years 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•9 years 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•9 years ago
|
||
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•9 years 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•9 years 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•9 years 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•9 years 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.
Comment 20•9 years ago
|
||
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.
Comment 21•9 years ago
|
||
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•9 years ago
|
||
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•9 years 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?
Comment 24•9 years ago
|
||
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•9 years 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•9 years ago
|
||
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.
Comment 27•9 years ago
|
||
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•9 years 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•9 years ago
|
||
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.
Updated•9 years ago
|
Assignee: nobody → mozilla
Comment 30•9 years ago
|
||
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•9 years ago
|
Attachment #8736479 -
Flags: feedback+
Comment 31•9 years 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.
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.
Comment 32•9 years ago
|
||
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•9 years 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•9 years 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.
Comment 35•9 years ago
|
||
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•9 years 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.
Comment 37•9 years ago
|
||
> 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•9 years ago
|
||
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•9 years 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.
Comment 40•9 years ago
|
||
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•9 years 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.
Comment 42•9 years ago
|
||
(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•9 years 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?
Comment 44•9 years ago
|
||
Sounds good to me!
Comment 45•9 years ago
|
||
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•9 years ago
|
||
Thanks for the f+, yes, as per comment #42 etc. I'll slash some more. Coming up right now.
Assignee | ||
Comment 47•9 years ago
|
||
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 48•9 years ago
|
||
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+
Comment 49•9 years ago
|
||
> 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•9 years 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•9 years 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•9 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #51)
> Core::Serialisers and Core::Editor only used by TB,
... and SeaMonkey. ;-)
Comment 53•9 years ago
|
||
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•9 years ago
|
||
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•9 years 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•9 years ago
|
Component: Composition → Editor
Product: MailNews Core → Core
Assignee | ||
Updated•9 years ago
|
Severity: minor → normal
Comment 56•9 years ago
|
||
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-
Comment 57•9 years ago
|
||
(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•9 years 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•9 years ago
|
||
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•9 years ago
|
Keywords: checkin-needed
Comment 60•9 years ago
|
||
Keywords: checkin-needed
Comment 61•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years 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.
Description
•