Closed
Bug 1263883
Opened 9 years ago
Closed 9 years ago
DOM insertion point and caret wrong when pressing "enter" after image in paragraph when <tt> is used.
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla48
| Tracking | Status | |
|---|---|---|
| firefox48 | --- | fixed |
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 4 obsolete files)
|
(deleted),
text/html
|
Details | |
|
(deleted),
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
DOM insertion point and caret wrong after image pasting in paragraph when <tt> is used.
It's best seen in the attached example, just follow the steps.
It doesn't fail if the <tt></tt> is removed.
I can't reproduce this with FF 45. So it must be a regression from my latest changes in paragraph mode in bug 1250010 (or less likely bug 1257363).
Most likely the first bug is the culprit, since it had to do with splitting paragraphs.
|
Assignee | |
Updated•9 years ago
|
|
|
Assignee | |
Comment 1•9 years ago
|
||
Backed out https://hg.mozilla.org/mozilla-central/rev/f859932803e8 from bug 1250010 locally. That fixes the problem. So the regression is confirmed.
|
|
Assignee | |
Comment 2•9 years ago
|
||
Here is a simpler example. No image insertion necessary, the image is already contained in the test. Just click to the right of the image and press enter. You will get an unexpected result.
Attachment #8740381 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•9 years ago
|
Summary: DOM insertion point and caret wrong after image pasting in paragraph when <tt> is used. → DOM insertion point and caret wrong when pressing "enter" after image in paragraph when <tt> is used.
|
|
Assignee | |
Comment 3•9 years ago
|
||
(Oops, fixed closing tag of <tt>.)
Attachment #8741234 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 4•9 years ago
|
||
Here is a very obvious fix for a silly mistake made in bug 1250010.
Bug 1250010 was about splitting empty paragraphs, but there can be paragraphs, that are not empty and do not contain text (which is a different code path). An image does the trick to let the faulty logic fall over.
Masayuki-san, would you like a test for this?
|
||
Comment 5•9 years ago
|
||
Comment on attachment 8741523 [details] [diff] [review]
Fix.
Could you write an automated test?
|
|
Assignee | |
Comment 6•9 years ago
|
||
Now with a test, in fact, I modified the existing test.
Attachment #8741523 -
Attachment is obsolete: true
Attachment #8741523 -
Flags: review?(masayuki)
Attachment #8741648 -
Flags: review?(masayuki)
|
|
||
Comment 7•9 years ago
|
||
Comment on attachment 8741648 [details] [diff] [review]
Fix + test (v1).
># HG changeset patch
># User Jorg K
># Parent 49d7fb650c9dde7cf6e4b2c7aa578a4a11e83f83
>Bug 1263883 - don't assume offset==0 when splitting paragraph. r=masayuki
>
>diff --git a/editor/libeditor/nsHTMLEditRules.cpp b/editor/libeditor/nsHTMLEditRules.cpp
>--- a/editor/libeditor/nsHTMLEditRules.cpp
>+++ b/editor/libeditor/nsHTMLEditRules.cpp
>@@ -6564,17 +6564,17 @@ nsHTMLEditRules::ReturnInParagraph(Selec
> NS_ENSURE_STATE(mHTMLEditor);
> res = mHTMLEditor->GetNextHTMLNode(aNode, aOffset, address_of(nearNode));
> NS_ENSURE_SUCCESS(res, res);
> NS_ENSURE_STATE(mHTMLEditor);
> if (!nearNode || !mHTMLEditor->IsVisBreak(nearNode) ||
> nsTextEditUtils::HasMozAttr(nearNode)) {
> newBRneeded = true;
> parent = aNode;
>- offset = 0;
>+ offset = aOffset;
> newSelNode = true;
> }
> }
> if (!newBRneeded) {
> sibling = nearNode;
> }
> }
> if (newBRneeded) {
>@@ -6583,17 +6583,17 @@ nsHTMLEditRules::ReturnInParagraph(Selec
>
> nsCOMPtr<nsIDOMNode> brNode;
> NS_ENSURE_STATE(mHTMLEditor);
> res = mHTMLEditor->CreateBR(parent, offset, address_of(brNode));
> sibling = brNode;
> if (newSelNode) {
> // We split the parent after the br we've just inserted.
> selNode = parent;
>- selOffset = 1;
>+ selOffset = offset + 1;
> }
> }
> *aHandled = true;
> return SplitParagraph(aPara, sibling, aSelection, address_of(selNode), &selOffset);
> }
>
> ///////////////////////////////////////////////////////////////////////////
> // SplitParagraph: split a paragraph at selection point, possibly deleting a br
>diff --git a/editor/libeditor/tests/test_bug1250010.html b/editor/libeditor/tests/test_bug1250010.html
>--- a/editor/libeditor/tests/test_bug1250010.html
>+++ b/editor/libeditor/tests/test_bug1250010.html
>@@ -8,27 +8,30 @@ https://bugzilla.mozilla.org/show_bug.cg
> <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css"/>
> <script type="text/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>
> </head>
> <body>
> <div id="display">
> </div>
>
>-<div id="content" contenteditable><p><b><font color="red">1234567890</font></b></p></div>
>+<div id="test1" contenteditable><p><b><font color="red">1234567890</font></b></p></div>
>+<div id="test2" contenteditable><p><tt>xyz</tt></p><p><tt><img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAYAAAAGCAIAAABvrngfAAAAFklEQVQImWMwjWhCQwxECoW3oCHihAB0LyYv5/oAHwAAAABJRU5ErkJggg=="></tt></p>
Hmm, the base encoded image data URI is ugly. Cannot you use <input> or something similar empty element instead?
>+ // The resulting HTML is sadly less than optimal:
>+ // A <br> gets inserted after the image and the "A" is followed by an empty <tt></tt>.
>+ is(div.innerHTML, originalHTML.replace(/<\/tt><\/p>$/, "<br></tt></p>") +
>+ "<p><tt>A</tt><br><tt></tt></p>", "unexpected HTML");
>+ todo_is(div.innerHTML, originalHTML.replace(/<\/tt><\/p>$/, "<br></tt></p>") +
>+ "<p><tt>A</tt><br></p>", "unexpected HTML");
If so, the 2nd arguments of them are easier to read (you can write it directly).
Flags: needinfo?(mozilla)
|
|
Assignee | |
Comment 8•9 years ago
|
||
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #7)
> Hmm, the base encoded image data URI is ugly. Cannot you use <input> or
> something similar empty element instead?
Well, the bug was detected with and image since when editing in Thunderbird we insert text and paste images. So an image is the natural choice. I tried a smaller image (2x2 pixels), but the data URI has about the same length.
> >+ // The resulting HTML is sadly less than optimal:
> >+ // A <br> gets inserted after the image and the "A" is followed by an empty <tt></tt>.
> >+ is(div.innerHTML, originalHTML.replace(/<\/tt><\/p>$/, "<br></tt></p>") +
> >+ "<p><tt>A</tt><br><tt></tt></p>", "unexpected HTML");
> >+ todo_is(div.innerHTML, originalHTML.replace(/<\/tt><\/p>$/, "<br></tt></p>") +
> >+ "<p><tt>A</tt><br></p>", "unexpected HTML");
> If so, the 2nd arguments of them are easier to read (you can write it
> directly).
You mean, if I have a shorter input HTML, I can write the expected HTML (2nd argument) directly. Yes, but with an image it will always be messy, hence the replace.
The tests are not meant to be pretty, the are meant to reflect reality.
Flags: needinfo?(mozilla)
|
|
||
Comment 9•9 years ago
|
||
Okay, then, could you use this approach?
function getImageDataURI()
{
return document.getElementsByTagName("img")[0].getAttribute("src");
}
is(div.innerHTML, "<p><tt>xyz</tt></p><p><tt><img src=\"" + getImageDataURI() + "\"><br></tt></p>...",
"unexpected HTML");
todo(...);
And also, I don't like the message like "unexpected HTML" which is displayed at failing. Could you explain what should occur. E.g., "Pressing Enter key should separate the paragraph in <tt> and insert <br> the end of <tt> in the former paragraph". Then, it's easier to read for other people.
FYI: Otherwise, looks good.
|
|
Assignee | |
Comment 10•9 years ago
|
||
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #9)
> Okay, then, could you use this approach?
I incorporated your suggestion.
> And also, I don't like the message like "unexpected HTML" which is displayed
> at failing. Could you explain what should occur. E.g., "Pressing Enter key
> should separate the paragraph in <tt> and insert <br> the end of <tt> in the
> former paragraph". Then, it's easier to read for other people.
I disagree. If the test fails one day, the person who broke it will see
unexpected HTML - got "<p><tt>...", expected "<p><tt>...".
This person will have to understand the test and what they did to make it fail. I can't write a long explanation into the third parameter of the is() function. However, I added a comment to explain what we expect.
Attachment #8741648 -
Attachment is obsolete: true
Attachment #8741648 -
Flags: review?(masayuki)
Attachment #8741800 -
Flags: review?(masayuki)
|
|
||
Comment 11•9 years ago
|
||
Comment on attachment 8741800 [details] [diff] [review]
Fix (v1) + test (v2).
I still don't like the third argument issue of is(), but it's okay for now. (If everybody should understand whole tests in new orange, why we need the third argument?)
Attachment #8741800 -
Flags: review?(masayuki) → review+
|
|
Assignee | |
Updated•9 years ago
|
Keywords: checkin-needed
|
|
Assignee | |
Comment 13•9 years ago
|
||
Not really necessary. This makes a few changes to what was implemented in bug 1250010. That area of the editor is not well tested, so the test that goes with the bug is the only testing we do. And the test succeeds locally.
Besides, I have level 3 rights and could push it to inbound myself.
Anyway, here's your try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=834ed3430eb4
|
|
Assignee | |
Updated•9 years ago
|
Keywords: checkin-needed
|
|
Assignee | |
Comment 14•9 years ago
|
||
Green ;-)
|
||
Comment 15•9 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #13)
> Besides, I have level 3 rights and could push it to inbound myself.
Feel free to do so in the future then. Standard practice is that checkin-needed requests need to have a Try link accompanying them.
Flags: in-testsuite+
|
||
Comment 16•9 years ago
|
||
Keywords: checkin-needed
|
||
Comment 17•9 years ago
|
||
| bugherder | ||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
|
||
Comment 18•9 years ago
|
||
This bug was landed in THUNDERBIRD_45_VERBRANCH, see bug 1265034
|
|
||
Comment 19•8 years ago
|
||
Pushed to a release branch on mozilla-beta for Thunderbird 47.0b1
You need to log in
before you can comment on or make changes to this bug.
Description
•