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

RESOLVED FIXED in Firefox 48

Status

()

Core
Editor
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

a year ago
Created attachment 8740381 [details]
Testpage showing the bug.

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

a year ago
Blocks: 1250010, 1248971
(Assignee)

Comment 1

a year 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

a year ago
Created attachment 8741234 [details]
Testpage showing the bug - much simpler example

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

a year 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

a year ago
Created attachment 8741235 [details]
Testpage showing the bug - much simpler example

(Oops, fixed closing tag of <tt>.)
Attachment #8741234 - Attachment is obsolete: true
(Assignee)

Comment 4

a year ago
Created attachment 8741523 [details] [diff] [review]
Fix.

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

Updated

a year ago
Blocks: 1253288
Comment on attachment 8741523 [details] [diff] [review]
Fix.

Could you write an automated test?
(Assignee)

Comment 6

a year ago
Created attachment 8741648 [details] [diff] [review]
Fix + test (v1).

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

Comment 8

a year 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)
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

a year ago
Created attachment 8741800 [details] [diff] [review]
Fix (v1) + test (v2).

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

Updated

a year ago
Keywords: checkin-needed
Please provide a Try link for this patch.
Keywords: checkin-needed

Updated

a year ago
Blocks: 1265034
(Assignee)

Comment 13

a year 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

a year ago
Keywords: checkin-needed
(Assignee)

Comment 14

a year ago
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+

Comment 16

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/105b3fba6454
Keywords: checkin-needed

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/105b3fba6454
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
This bug was landed in THUNDERBIRD_45_VERBRANCH, see bug 1265034

Comment 19

11 months 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.