Closed Bug 1257363 Opened 8 years ago Closed 8 years ago

Deleting an empty paragraph with backspace/delete doesn't place the caret correctly. Backspace should place into preceding text, delete into following text.

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

Crash Data

Attachments

(4 files, 6 obsolete files)

Attached file Page showing the bug
This was raised by Thunderbird users in bug 1248971. See attached sample page for a reproducible case.
Blocks: 1248971
Works OK in Chrome.
Attached patch WIP: (v1). (obsolete) — Splinter Review
This works for the simple text case. Let's see what it breaks:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42709485736e
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
This works and doesn't break anything, so let's ask for review.

Robert (:roc) kindly did the last review, but not his nickname says "Exited". His blog says that he left Mozilla to work on "rr".

So who's left to review this?

It can't be that unpaid volunteers work on this and then there's not even anyone to review.

Sorry about the r? and NI SPAM.
Attachment #8732642 - Attachment is obsolete: true
Flags: needinfo?(overholt)
Flags: needinfo?(bugs)
Attachment #8732653 - Flags: review?(masayuki)
Attachment #8732653 - Flags: review?(ehsan)
(Oops, accidentally changed an unrelated file, sorry)
Attachment #8732653 - Attachment is obsolete: true
Attachment #8732653 - Flags: review?(masayuki)
Attachment #8732653 - Flags: review?(ehsan)
Attachment #8732654 - Flags: review?(masayuki)
Attachment #8732654 - Flags: review?(ehsan)
Comment on attachment 8732654 [details] [diff] [review]
Possible solution (v1b) with test.

I don't review editor code any more.
Attachment #8732654 - Flags: review?(ehsan)
Although, I'm not an official reviewer around editor and I don't have enough knowledge around HTMLEditRules yet, but if there is no reviewer, I'll try to review it with reading around the patch. Is this okay to you, Ehsan?
Flags: needinfo?(ehsan)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #7)
> Although, I'm not an official reviewer around editor and I don't have enough
> knowledge around HTMLEditRules yet, but if there is no reviewer, I'll try to
> review it with reading around the patch. Is this okay to you, Ehsan?

I exchanged e-mails with Ehsan on this. That should be OK. Thanks, Masayuki!
Flags: needinfo?(overholt)
Flags: needinfo?(ehsan)
Flags: needinfo?(bugs)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #7)
> I'll try to review it with reading around the patch.

Let me explain how I came up with the patch.

I put a breakpoint on Collapse(). This way I could see that there was only one call to Collapse() under the circumstances of the testcase.

This Collapse() was unconditionally called like this:
// Adjust selection to be right after it.
res = aSelection->Collapse(blockParent, offset + 1);

In other words: Deleting an empty block via the delete key and via the backspace key were treated exactly the same. Obviously this is wrong since for backspace you want to move the caret up to the preceding text node, if any. So this is what the patch does. It works and I have a green try run for it.
I also have a try run (which I did for another bug) that contains the new test:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45b55ca763d3
Sorry for the delay to review. I'd like to review it tomorrow or Friday. But I'm going to be in a business trip until Sunday, so, I cannot say exactly when I can review it.
Blocks: 968733
OK, I've duped bug 968733 here, so the summary needs to change. Fix is coming in a minute, review cancelled for now since I need to add a test for the delete case.
Summary: Deleting an empty paragraph with backspace doesn't place the caret into the preceding paragraph but the following text not in paragraph → Deleting an empty paragraph with backspace/delete doesn't places the caret into incorrectly. Backspace should place into preceding text, delete into following text.
Here is the fix for the delete case. Will add a test as well, then request review again.
Attachment #8732654 - Attachment is obsolete: true
Attachment #8732654 - Flags: review?(masayuki)
OK, here are the complete fix and tests for both cases. Will add try run in the morning.
Attachment #8734111 - Attachment is obsolete: true
Attachment #8734157 - Flags: review?(masayuki)
(oops, cut/paste error in comment)
Attachment #8734157 - Attachment is obsolete: true
Attachment #8734157 - Flags: review?(masayuki)
Attachment #8734159 - Flags: review?(masayuki)
Comment on attachment 8734159 [details] [diff] [review]
Possible solution (v2) for backspace and delete case, tests for both cases

I'm very sorry for the long delay.

First, the if condition of the |else| block is checking if the empty block is a list item. If it's list item (i.e., changing <p></p> in your test to <li></li>), I can reproduce same bug but probably your patch won't fix this case. But I think that this is different bug if we take your approach.

>@@ -5011,19 +5012,44 @@ nsHTMLEditRules::CheckForEmptyBlock(nsIN
>           // Adjust selection to be right before it
>           res = aSelection->Collapse(listParent, listOffset);
>           NS_ENSURE_SUCCESS(res, res);
>         }
>         // Else just let selection percolate up.  We'll adjust it in
>         // AfterEdit()
>       }
>     } else {
>-      // Adjust selection to be right after it
>-      res = aSelection->Collapse(blockParent, offset + 1);
>-      NS_ENSURE_SUCCESS(res, res);
>+      if (aAction == nsIEditor::eNext) {
>+        // Adjust selection to be right after it.
>+        res = aSelection->Collapse(blockParent, offset + 1);
>+        NS_ENSURE_SUCCESS(res, res);
>+
>+        // Move to the start of the next node if it's a text.
>+        nsCOMPtr<nsIContent> nextNode = mHTMLEditor->GetNextNode(blockParent,
>+                                                                 offset + 1, true);
>+        nsCOMPtr<nsIDOMText> textNode = do_QueryInterface(nextNode);
>+        if (textNode) {
>+          res = aSelection->Collapse(textNode, 0);
>+          NS_ENSURE_SUCCESS(res, res);
>+        }
>+      } else {
>+        // Move to the end of the previous node if it's a text.
>+        nsCOMPtr<nsIContent> priorNode = mHTMLEditor->GetPriorNode(blockParent,
>+                                                                   offset,
>+                                                                   true);
>+        nsCOMPtr<nsIDOMText> textNode = do_QueryInterface(priorNode);
>+        if (textNode) {
>+          nsAutoString tempString;
>+          textNode->GetData(tempString);

Why don't you use nsIContent::TextLength()?

>+          res = aSelection->Collapse(priorNode, tempString.Length());

I think that NS_ENSURE_SUCCESS(res, res) should be inserted here

>+        } else {
>+          res = aSelection->Collapse(blockParent, offset + 1);

and here

>+        }
>+        NS_ENSURE_SUCCESS(res, res);

Instead of here because the line number of the warning message tells us if the failure occurred in text node.

>+<div id="backspaceCSS" contenteditable><p style="color:red;">12345</p>67</div>
>+<div id="backspace" contenteditable><p><font color="red">12345</font></p>67</div>
>+<div id="deleteCSS" contenteditable><p style="color:red;">x</p></div>
>+<div id="delete" contenteditable><p><font color="red">y</font></p></div>
>+
>+<pre id="test">
>+</pre>
>+
>+<script class="testbody" type="application/javascript">
>+
>+/** Test for Bug 1257363 **/
>+SimpleTest.waitForExplicitFinish();
>+SimpleTest.waitForFocus(function() {
>+
>+  // ***** Backspace test *****
>+  var div = document.getElementById("backspaceCSS");
>+  div.focus();
>+  synthesizeMouse(div, 100, 2, {}); /* click behind and down */

You specify the pixels directly here. I think that you should specify font-size to the <div> elements. Then, this becomes safer.


>+  // Return and backspace should take us to the following line.
>+  synthesizeKey("VK_RETURN", {});
>+  synthesizeKey("VK_DELETE", {});

The comment says "Return and backspace", but this is Delete key.

>+  // Return and backspace should take us to the following line.
>+  synthesizeKey("VK_RETURN", {});
>+  synthesizeKey("VK_DELETE", {});

The comment says "Return and backspace", but this is Delete key.

>+  // ***** Delete test *****
>+  div = document.getElementById("deleteCSS");
>+  div.focus();
>+  synthesizeMouse(div, 100, 2, {}); /* click behind and down */
>+
>+  var sel = window.getSelection();
>+  var selRange = sel.getRangeAt(0);
>+  is(selRange.endContainer.nodeName, "#text", "selection should be at the end of text node");
>+  is(selRange.endOffset, 1, "offset should be 1");
>+
>+  // left, enter, up-arrow and delete should take us to where we started.
>+  synthesizeKey("VK_LEFT", {});
>+  synthesizeKey("VK_RETURN", {});
>+  synthesizeKey("VK_UP", {});

I hope that you test the caret position after each synthesizeKey() because it's easier to investigate for other developers when this test detects a regression.

And you tested text node name in a lot of place. But I think that you should test its parent element's tag name too.

>+  // left, enter, up-arrow and delete should take us to where we started.
>+  synthesizeKey("VK_LEFT", {});
>+  synthesizeKey("VK_RETURN", {});
>+  synthesizeKey("VK_UP", {});
>+  synthesizeKey("VK_DELETE", {});

Same here.


Thank you for your work, looks almost good except above nits. I'd like to check the new patch, so, r-'ing for now.
Attachment #8734159 - Flags: review?(masayuki) → review-
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #19)
> I'm very sorry for the long delay.
No problem, I was busy with other stuff in the meantime. Thank you so much for the thorough review, much appreciated.

> the if condition of the |else| block is checking if the empty block
> is a list item.
Correct. I won't touch list processing for now.

> Why don't you use nsIContent::TextLength()?
Didn't know it, using it now.

> I think that NS_ENSURE_SUCCESS(res, res) should be inserted here
Done.

> >+  synthesizeMouse(div, 100, 2, {}); /* click behind and down */
> You specify the pixels directly here. I think that you should specify
> font-size to the <div> elements. Then, this becomes safer.
This is common practise:
https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/tests/test_bug772796.html#165 + two more.
https://dxr.mozilla.org/mozilla-central/source/layout/generic/test/test_bug756984.html#42 + one more.

> The comment says "Return and backspace", but this is Delete key.
Comment fixed.

> I hope that you test the caret position after each synthesizeKey() because
> it's easier to investigate for other developers when this test detects a
> regression.
Done. I added more testing after each keystroke.
Attachment #8734159 - Attachment is obsolete: true
Attachment #8736236 - Flags: review?(masayuki)
Comment on attachment 8736236 [details] [diff] [review]
Possible solution (v2b) for backspace and delete case, tests for both casesb

Thank you very much!
Attachment #8736236 - Flags: review?(masayuki) → review+
Keywords: checkin-needed
Fixing terrible grammar in summary ;-)
Summary: Deleting an empty paragraph with backspace/delete doesn't places the caret into incorrectly. Backspace should place into preceding text, delete into following text. → Deleting an empty paragraph with backspace/delete doesn't place the caret correctly. Backspace should place into preceding text, delete into following text.
Blocks: 1260975
https://hg.mozilla.org/mozilla-central/rev/36888eb8c404
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Sadly this crashed under certain circumstances since next and previous aren't checked for null.

Patch coming.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1261602
Masayuki-san, we created a crash that was noted in TB testing in bug 1261602.

Please review asap to fix this.
No longer depends on: 1261602
Depends on: 1261602
(In reply to Jorg K (GMT+2) from comment #26)
> Created attachment 8737543 [details] [diff] [review]
> Add null check to prevent crash.
> 
> Masayuki-san, we created a crash that was noted in TB testing in bug 1261602.
> 
> Please review asap to fix this.

Forgot to r? to me?
Comment on attachment 8737543 [details] [diff] [review]
Add null check to prevent crash.

Sorry.
Attachment #8737543 - Flags: review?(masayuki)
Comment on attachment 8737543 [details] [diff] [review]
Add null check to prevent crash.

np ;-)
Attachment #8737543 - Flags: review?(masayuki) → review+
Dear Sheriff,
please land the second patch "Add null check to prevent crash".
Thanks.
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/823040191adc
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Blocks: 1261602
No longer depends on: 1261602
> https://hg.mozilla.org/mozilla-central/rev/823040191adc

I just added the crash signature for the crashes fixed by this commit, so that crash reports will be linked to this bug.
Crash Signature: [@ nsEditor::IsTextNode]
There's only a very brief interval of the faulty code being live:
https://hg.mozilla.org/mozilla-central/rev/36888eb8c404 - broken, landed 1st April.
https://hg.mozilla.org/mozilla-central/rev/823040191adc - fixed, landed 3rd April.
Since I happened to stumble over this crasher in a Nightly that happened to contain it, I can happily verify that it's now gone :)
Good work Jorg, thanks!
This bug was landed in THUNDERBIRD_45_VERBRANCH (see bug 1260975)
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.