Closed Bug 1263883 Opened 8 years ago Closed 8 years ago

DOM insertion point and caret wrong when pressing "enter" after image in paragraph when <tt> is used.

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

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)

Attached file Testpage showing the bug. (obsolete) —
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.
Blocks: 1250010, 1248971
Backed out https://hg.mozilla.org/mozilla-central/rev/f859932803e8 from bug 1250010 locally. That fixes the problem. So the regression is confirmed.
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
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.
(Oops, fixed closing tag of <tt>.)
Attachment #8741234 - Attachment is obsolete: true
Attached patch Fix. (obsolete) — Splinter Review
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?
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8741523 - Flags: review?(masayuki)
Blocks: 1253288
Comment on attachment 8741523 [details] [diff] [review]
Fix.

Could you write an automated test?
Attached patch Fix + test (v1). (obsolete) — Splinter Review
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 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)
(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)
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.
(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 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+
Keywords: checkin-needed
Please provide a Try link for this patch.
Keywords: checkin-needed
Blocks: 1265034
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
Keywords: checkin-needed
Green ;-)
(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+
https://hg.mozilla.org/mozilla-central/rev/105b3fba6454
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
This bug was landed in THUNDERBIRD_45_VERBRANCH, see bug 1265034
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.

Attachment

General

Created:
Updated:
Size: