Closed Bug 1328093 Opened 7 years ago Closed 7 years ago

HTMLEditor::InsertTextWithQuotations() should include the first line break into the <span> it creates.

Categories

(Core :: DOM: Editor, defect, P3)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

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

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

Attachments

(1 file, 1 obsolete file)

1328093-put-br-into-span.patch (v1a) - changed comment
1.53 KB, patch
masayuki
: review+
jcristau
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
This is a regression of bug 1288911.

There we changed HTMLEditor::InsertAsPlaintextQuotation() to wrap the quote into <span style="white-space: pre-wrap; display: block; width: 98vw;"> ... </span>

So say the user replies in plain-text to an e-mail which contains one line:
Line1

They will be presented with a window containing:
<body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;">
<span style="white-space: pre-wrap; display: block; width: 98vw;">
&gt; Line1<br></span><br></body>

They type into the following line:

<body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;">
<span style="white-space: pre-wrap; display: block; width: 98vw;">
&gt; Line1<br></span>Line2<br></body>


They save the draft. This is saved:
> Line1
Line 2

User edits the draft and gets:

<body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;">
<span style="white-space: pre-wrap; display: block; width: 98vw;">
&gt; Line1</span><br>Line2<br></body>

See the difference? Yes, the <br> has moved from inside the span to outside.

Inside the span the <br> was invisible since the span was displayed as block.

Now there appears to be an empty line between Line1 and Line2.

So the idea is to include the first <br> into the span where it was originally.

Sorry, this is a very e-mail-centric bug.
Masayuki-san, happy New Year.

Please let me know what you think. Would you like a test, a try run or anything else? Do you compile Thunderbird to see this problem?
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8822977 - Flags: feedback?(masayuki)
It'd be great if we'd have a mochitest for this. Although, I'm not sure if it's testable with XPCOM interface or something.

You should run tests at least once on tryserver (I guess that mochitests is usually enough).

Anyway, I need to understand what lineStart affects.

I'll be back on this Friday. Please wait until then.
I outlined the following in bug 1288911 comment #23 (repeating it here):

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()

If you enable "middle button paste" with pref middlemouse.paste you can trigger PasteAsQuotation()
http://searchfox.org/mozilla-central/rev/36bfd0a8da5490274ca49c100957244253845e52/editor/libeditor/EditorEventListener.cpp#729
in HandleMiddleClickPaste(). There are also calls in PasteQuotationCommand::DoCommand():
http://searchfox.org/mozilla-central/search?q=PasteAsQuotation&path=

HTMLEditor::InsertTextWithQuotations() doesn't appear to be called in M-C at all
http://searchfox.org/mozilla-central/search?q=InsertTextWithQuotations&path=
so I won't break Firefox. Or am I missing something?
TextEditor::InsertTextWithQuotations() has one call in TextEditor.cpp.

So unless I invest a lot of work, I can't write a Mochitest to exercise this function, simply because it's not called currently. I'd have to wire something up to it first.

The next thing I was thinking about was to include it into a static test, like TestPlainTextSerializer.cpp or something similar. But that doesn't work either, since the function has no output, it simply inserts into the DOM.

So here's a limited Mochitest run on Linux64 which shouldn't break simply since this function isn't called. I'll prove it with a runtime abort, OK ;-)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cc994670229594e9db65613068ecec7b9e50ee2
https://hg.mozilla.org/try/rev/d0396c3c5561194ecfa56a0c4499cbf932e2d161

If I break anything, it would be in TB, and there my testing shows that the patch does what I need.

HTMLEditor::InsertTextWithQuotations("> Line1\nLine2")
will now give
<span style="white-space: pre-wrap; display: block; width: 98vw;">
&gt; Line1<br></span>Line2<br></body>

instead of:
<span style="white-space: pre-wrap; display: block; width: 98vw;">
&gt; Line1</span><br>Line2<br></body>

The effect of lineStart++ is that the next line after the quoted hunk will start after the \n, so the \n is counted as belonging to the quoted hunk and is therefore inserted into the <span> tag.
There are some crazy failures in this run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cc994670229594e9db65613068ecec7b9e50ee2
No idea what they are about.
Priority: -- → P3
Any chance to get this reviewed and landed before the next branch date so I can request Aurora uplift?
(In reply to Jorg K (GMT+1) from comment #4)
> There are some crazy failures in this run:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2cc994670229594e9db65613068ecec7b9e50ee2
> No idea what they are about.

These error seems to be tier-2 build.  You can ignore this failure.
(In reply to Makoto Kato [:m_kato] from comment #6)
> (In reply to Jorg K (GMT+1) from comment #4)
> > There are some crazy failures in this run:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=2cc994670229594e9db65613068ecec7b9e50ee2
> > No idea what they are about.
> 
> These error seems to be tier-2 build.  You can ignore this failure.

Yeah, and you should run automated tests both on opt/debug build because they run different path (e.g., MOZ_ASSERT), detecting memory leak only on debug build, etc.
Comment on attachment 8822977 [details] [diff] [review]
1328093-put-br-into-span.patch (v1)

>diff --git a/editor/libeditor/HTMLEditorDataTransfer.cpp b/editor/libeditor/HTMLEditorDataTransfer.cpp
>@@ -1770,16 +1770,20 @@ HTMLEditor::InsertTextWithQuotations(con
>       // But if the current hunk is quoted, then we want to make sure
>       // that any extra newlines on the end do not get included in
>       // the quoted section: blank lines flaking a quoted section
>       // should be considered unquoted, so that if the user clicks
>       // there and starts typing, the new text will be outside of
>       // the quoted block.
>       if (curHunkIsQuoted) {
>         lineStart = firstNewline;
>+
>+        // Ensure that the first line break goes into the hunk
>+        // since quoted hunks can be displayed as blocks.
>+        lineStart++;
>       }

Okay, perhaps, I got what you want to do. So, you want to do this when lineStart is *firstNewline is a line breaker, no following character, the block is already quoted.

Then, I don't like this change because it *looks* like hacky. So, |lineStart = firstNewline + 1;| looks better, however, nsReadingIterator doesn't have |size_type operator+(difference_type)|... So, I don't have better idea to write this... So, looks okay to me, sigh.
Attachment #8822977 - Flags: feedback?(masayuki) → feedback+
Ah, inserting following line at start of the block might help to understand:
MOZ_ASSERT(*firstNewline == '\n');
Comment on attachment 8822977 [details] [diff] [review]
1328093-put-br-into-span.patch (v1)

Yes, I tried the |lineStart = firstNewline + 1;| and that didn't compile ;-(

I don't think the MOZ_ASSERT() is necessary or helpful, since ...

    bool found = FindCharInReadable('\n', lineStart, strEnd);
    bool quoted = false;
    if (found) {
      nsAString::const_iterator firstNewline (lineStart);

So lineStart is advanced to the first \n and firstNewLine will contain the pointer to that first new line, as the name suggests.

So why test it again later?
Attachment #8822977 - Flags: review?(masayuki)
Comment on attachment 8822977 [details] [diff] [review]
1328093-put-br-into-span.patch (v1)

(In reply to Jorg K (GMT+1) from comment #10)
> I don't think the MOZ_ASSERT() is necessary or helpful, since ...
> 
>     bool found = FindCharInReadable('\n', lineStart, strEnd);
>     bool quoted = false;
>     if (found) {
>       nsAString::const_iterator firstNewline (lineStart);
> 
> So lineStart is advanced to the first \n and firstNewLine will contain the
> pointer to that first new line, as the name suggests.
> 
> So why test it again later?

Yes, it's NOT necessary but it ensures that the value is always '\n' at least on debug builds even if the code you quoted is modified by other developers. And I want it for easier to read/understand.

Another options is, inserting |MOZ_ASSERT(*lineStart == '\n');| before incrementing lineStart.

Or could you update the comment as:

// firstNewline points the first '\n', so, next line start is next character of it.

Or something similar.
Attachment #8822977 - Flags: review?(masayuki) → review+
Or renaming firstNewLine to firstLineBreaker might be clearer.
I think a better comment makes it clear.

In case you agree, please r+ and add "checkin-needed" (since I'm AFK for a few hours now).
Attachment #8822977 - Attachment is obsolete: true
Attachment #8825332 - Flags: review?(masayuki)
Attachment #8825332 - Flags: review?(masayuki) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5001b49ef3f6
HTMLEditor::InsertTextWithQuotations() should include the first line break into the <span> it creates. r=masayuki
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5001b49ef3f6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8825332 [details] [diff] [review]
1328093-put-br-into-span.patch (v1a) - changed comment

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1288911 which uncovered (not caused) this.
[User impact if declined]: In Thunderbird, using a saved plaintext draft inserts unwanted blank lines and confuses the user.
[Is this code covered by automated tests?]: No, since this code does not execute in Firefox. In fact, I have a try run with a "run time abort" in the function to prove this.
[Has the fix been verified in Nightly?]: Yes, on a Thunderbird Daily.
[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?]: Not for Firefox since the code is not used in Firefox. I'm the Thunderbird release manager and I request this to be uplifted to mozilla52 since bug 1288911 kindly got uplifted as well upon my request.
[Why is the change risky/not risky?]: Not risky for Firefox, since code is not run in Firefox.
[String changes made/needed]: None.
Attachment #8825332 - Flags: approval-mozilla-aurora?
Comment on attachment 8825332 [details] [diff] [review]
1328093-put-br-into-span.patch (v1a) - changed comment

editor regression fix for thunderbird, aurora52+
Attachment #8825332 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1288911
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: