Closed
Bug 1328093
Opened 8 years ago
Closed 8 years ago
HTMLEditor::InsertTextWithQuotations() should include the first line break into the <span> it creates.
Categories
(Core :: DOM: Editor, defect, P3)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
References
Details
Attachments
(1 file, 1 obsolete file)
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;">
> 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;">
> 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;">
> 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.
Assignee | ||
Comment 1•8 years ago
|
||
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?
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
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;">
> Line1<br></span>Line2<br></body>
instead of:
<span style="white-space: pre-wrap; display: block; width: 98vw;">
> 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.
Assignee | ||
Comment 4•8 years ago
|
||
There are some crazy failures in this run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cc994670229594e9db65613068ecec7b9e50ee2
No idea what they are about.
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 5•8 years ago
|
||
Any chance to get this reviewed and landed before the next branch date so I can request Aurora uplift?
Comment 6•8 years ago
|
||
(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.
Comment 7•8 years ago
|
||
(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 8•8 years ago
|
||
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+
Comment 9•8 years ago
|
||
Ah, inserting following line at start of the block might help to understand:
MOZ_ASSERT(*firstNewline == '\n');
Assignee | ||
Comment 10•8 years ago
|
||
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 11•8 years ago
|
||
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+
Comment 12•8 years ago
|
||
Or renaming firstNewLine to firstLineBreaker might be clearer.
Assignee | ||
Comment 13•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8825332 -
Flags: review?(masayuki) → review+
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 years ago
|
||
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
Comment 15•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Comment 16•8 years ago
|
||
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 17•8 years ago
|
||
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+
Comment 18•8 years ago
|
||
bugherder uplift |
status-firefox52:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•