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

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

Firefox Tracking Flags

(firefox48 fixed)

Details

(crash signature)

Attachments

(4 attachments, 6 obsolete attachments)

(Assignee)

Description

a year ago
Created attachment 8731471 [details]
Page showing the bug

This was raised by Thunderbird users in bug 1248971. See attached sample page for a reproducible case.
(Assignee)

Updated

a year ago
Blocks: 1248971
(Assignee)

Comment 1

a year ago
Works OK in Chrome.
(Assignee)

Comment 2

a year ago
Created attachment 8732642 [details] [diff] [review]
WIP: (v1).

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

Comment 3

a year ago
Wrong try syntax, again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53213d4f93dc
(Assignee)

Comment 4

a year ago
Created attachment 8732653 [details] [diff] [review]
Possible solution (v1) with test.

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

Comment 5

a year ago
Created attachment 8732654 [details] [diff] [review]
Possible solution (v1b) with test.

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

Comment 9

a year ago
(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.
(Assignee)

Comment 10

a year ago
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.
(Assignee)

Updated

a year ago
Blocks: 968733
(Assignee)

Updated

a year ago
Duplicate of this bug: 968733
(Assignee)

Comment 13

a year ago
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.
(Assignee)

Comment 14

a year ago
Created attachment 8734111 [details] [diff] [review]
Possible solution (v2) for backspace and delete case, test only for backspace.

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

Comment 15

a year ago
Created attachment 8734122 [details]
Here's the delete example for manual testing
(Assignee)

Comment 16

a year ago
Created attachment 8734157 [details] [diff] [review]
Possible solution (v2) for backspace and delete case, tests for both cases

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

Comment 17

a year ago
Created attachment 8734159 [details] [diff] [review]
Possible solution (v2) for backspace and delete case, tests for both cases

(oops, cut/paste error in comment)
Attachment #8734157 - Attachment is obsolete: true
Attachment #8734157 - Flags: review?(masayuki)
Attachment #8734159 - Flags: review?(masayuki)
(Assignee)

Comment 18

a year ago
New try with with latest patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=484fe22f2c34
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-
(Assignee)

Comment 20

a year ago
Created attachment 8736236 [details] [diff] [review]
Possible solution (v2b) for backspace and delete case, tests for both casesb

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

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Comment 22

a year ago
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.
(Assignee)

Updated

a year ago
Blocks: 1260975

Comment 23

a year ago
bugherderlanding
https://hg.mozilla.org/integration/mozilla-inbound/rev/36888eb8c404
Keywords: checkin-needed

Comment 24

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/36888eb8c404
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Comment 25

a year ago
Sadly this crashed under certain circumstances since next and previous aren't checked for null.

Patch coming.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

a year ago
Depends on: 1261602
(Assignee)

Comment 26

a year ago
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.
(Assignee)

Updated

a year ago
No longer depends on: 1261602
(Assignee)

Updated

a year ago
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?
(Assignee)

Comment 28

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

Comment 30

a year ago
Dear Sheriff,
please land the second patch "Add null check to prevent crash".
Thanks.
Keywords: checkin-needed

Comment 31

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/823040191adc78b25f1c8ec6d0523c209b97598f
Bug 1257363 - add null check so it doesn't crash. r=masayuki
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 32

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/823040191adc
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
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]
(Assignee)

Comment 34

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

Comment 37

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.