Closed Bug 1265800 Opened 8 years ago Closed 8 years ago

Caret in wrong location upon backspace, follows inserting inserts extra <br>

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed

People

(Reporter: jorgk-bmo, Assigned: ayg)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

DOM: Left Thunderbird, right Firefox
5.36 KB, image/png
Details
0001-Bug-1265800-Small-unrelated-cleanup-patch.patch
3.28 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
Two test cases.
559 bytes, text/html
Details
0002-Bug-1265800-part-1-Don-t-place-cursor-after-invisibl.patch
14.72 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
0003-Bug-1265800-part-2-Move-cursor-into-all-adjacent-nod.patch
3.04 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
Attached file wrong-br-inserted.html (obsolete) — 
Caret in wrong location upon backspace, follows inserting inserts extra <br>:

Position behind the x, hit "enter" twice, then backspace.

In FF, after hitting backspace, the cursor jumps to the top right corner. In TB I don't see that.

After typing x, another <br> gets inserted, so the result is:
<p><tt>x</tt></p><p><tt><br></tt></p><tt>x</tt>

This is wrong in two ways. The <br> shouldn't have been inserted and the x should be inside the <tt> tag.

I'm a little confused since TB acts differently, there I get
<p><tt>x</tt></p><p><tt><br>x</tt>

So in TB at least the second x is wrapped in a <tt> tag.
I just noticed this bug today. I'm confused since the TB editing window reacts differently. Can I get you interested?
Flags: needinfo?(ayg)
Using the DOM inspector I see this in TB and FF.
More minimal test-case:

data:text/html,<!doctype html>
<div contenteditable><p><br><p><br></p></div>

Put the cursor in the second <p> and backspace.  The selection is now after the <br> in the first <p>.  It should not be possible to have a selection endpoint at this location -- it needs to be before.
Flags: needinfo?(ayg)
Thanks for minimising the test case, are you interested in maximising a solution ;-)
So basically, after joining blocks in deletion, the editor puts the cursor someplace that isn't displayed (after a trailing <br> inside a block).  I could special-case blocks that end with a <br>, but the right way to do this is probably to ask layout code if the cursor is in a position that's not actually displayed, and move it backwards until it gets to a place that is displayed.  Is there any good way for the editor to ask layout code if a given point in the DOM is suitable to place the cursor?  If so, could you give me some sample code?

Also, is the editor able to freely talk to layout to begin with?  I've seen some code that takes an aSafeToAskFrames boolean parameter, and I don't know why it wouldn't be safe to ask frames.
Flags: needinfo?(dbaron)
Aryeh, we have two bugs open on paragraph editing: Bug 1265800 (this one here) and bug 1263357. Can we share the work? I'd really like to have both bugs fixed in the 49 cycle.

This one here looks more like an editor bug to me since an undesired <br> is inserted.

Or maybe I should read your comment #3 and comment #5 more carefully. You are saying that this is caused by a bad selection and wouldn't be caused if the selection were placed better after deletion.

I hope David gives us some hints here.
(In reply to :Aryeh Gregor (working until May 8) from comment #5)
> So basically, after joining blocks in deletion, the editor puts the cursor
> someplace that isn't displayed (after a trailing <br> inside a block).  I
> could special-case blocks that end with a <br>, but the right way to do this
> is probably to ask layout code if the cursor is in a position that's not
> actually displayed, and move it backwards until it gets to a place that is
> displayed.  Is there any good way for the editor to ask layout code if a
> given point in the DOM is suitable to place the cursor?  If so, could you
> give me some sample code?

I think you'd need to define "suitable to place the cursor" first; I don't think we have a generic function for such a thing.

Probably the best approach is to start from what currently happens and try to fix the "not suitable to place the cursor" case that you're dealing with.  Then you can write a function that answers the question you want to answer.

(This is assuming that there isn't *already* a function about "suitable to place the cursor".  Just because I don't know if it doesn't mean it's not there.)

> Also, is the editor able to freely talk to layout to begin with?  I've seen
> some code that takes an aSafeToAskFrames boolean parameter, and I don't know
> why it wouldn't be safe to ask frames.

Generally, yes, although there might be times where editor wants up-to-date information but it's not safe to trigger a flush.  I don't know what the aSafeToAskFrames was added for, but you can look at version control history (if it's too old for the history in https://hg.mozilla.org/mozilla-central/ , use https://github.com/mozilla/gecko-dev/ ).
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) from comment #7)
> I think you'd need to define "suitable to place the cursor" first; I don't
> think we have a generic function for such a thing.

Do you know how we handle picking the location for the selection in the DOM when the user clicks or presses the arrow keys?  Wherever that code is, it seems to already correctly handle this case -- the selection goes before the invisible <br> here, not after.

If you don't know, is there anyone who would know more?

> Generally, yes, although there might be times where editor wants up-to-date
> information but it's not safe to trigger a flush.  I don't know what the
> aSafeToAskFrames was added for, but you can look at version control history
> (if it's too old for the history in https://hg.mozilla.org/mozilla-central/
> , use https://github.com/mozilla/gecko-dev/ ).

Bug 22227 and bug 46209 add what seems like the first aSafeToAskFrames, and a reference to "but we aren't supposed to talk to frames".  I guess I'll assume that the editor talking to frames is fine until proven otherwise.
Flags: needinfo?(dbaron)
(In reply to :Aryeh Gregor (working until May 8) from comment #8)
> Do you know how we handle picking the location for the selection in the DOM
> when the user clicks or presses the arrow keys?  Wherever that code is, it
> seems to already correctly handle this case -- the selection goes before the
> invisible <br> here, not after.

For arrow keys, I *think* it goes through nsFrameSelection::MoveCaret.  (There's stuff related to "hints" that I don't understand.)

For clicks, I think it goes through nsFrameSelection::HandleDrag.  At least, that seems to be the relevant place where the selection code calls nsIFrame::GetContentOffsetsFromPoint.

> If you don't know, is there anyone who would know more?

Maybe Ehsan?
Flags: needinfo?(dbaron)
So it looks like we already do this sort of adjustment in nsHTMLEditRules::AdjustSelection, and the patch is simple.  I'll post it for review once I have a green try run.
Two adjacent blocks of line-for-line identical code.  :/
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #8748580 - Flags: review?(masayuki)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6e32f6a6244

Looks like I'll be gone by the time you get back, so if anyone wants it landed before a few months from now, someone else probably needs to land it.
Attachment #8748582 - Flags: review?(masayuki)
I can see it through. When are you back? Any chance you can look at the <shift><enter> bug before you leave? Bug 1263357.
I'll be back August 15.
Comment on attachment 8748580 [details] [diff] [review]
0001-Bug-1265800-Small-unrelated-cleanup-patch.patch

> +            // XXX What's special about these three types of block?

I guess that the original author tried to minimize the risk of the change.
Attachment #8748580 - Flags: review?(masayuki) → review+
Comment on attachment 8748582 [details] [diff] [review]
0002-Bug-1265800-Don-t-place-cursor-after-invisible-break.patch

># HG changeset patch
># User Aryeh Gregor <ayg@aryeh.name>
>
>Bug 1265800 - Don't place cursor after invisible break
>
>If the user tries to insert text without moving the cursor, the
>invisible break will become visible, which from the user's perspective
>means an extra line break was inserted for no reason.
>
>diff --git a/editor/libeditor/nsHTMLEditRules.cpp b/editor/libeditor/nsHTMLEditRules.cpp
>index aa8a204..c9c706e 100644
>--- a/editor/libeditor/nsHTMLEditRules.cpp
>+++ b/editor/libeditor/nsHTMLEditRules.cpp
>@@ -7636,17 +7636,24 @@ nsHTMLEditRules::AdjustSelection(Selection* aSelection,
>       // put selection in right place:
>       if (aAction == nsIEditor::ePrevious)
>         textNode->GetLength((uint32_t*)&offset);
>       res = aSelection->Collapse(nearNode,offset);
>     }
>     else  // must be break or image
>     {
>       selNode = nsEditor::GetNodeLocation(nearNode, &selOffset);
>-      if (aAction == nsIEditor::ePrevious) selOffset++;  // want to be beyond it if we backed up to it
>+      NS_ENSURE_STATE(mHTMLEditor);
>+      if (aAction == nsIEditor::ePrevious &&
>+          (!nsTextEditUtils::IsBreak(nearNode) ||
>+           mHTMLEditor->IsVisBreak(nearNode))) {

nsHTMLEditor::IsVisBreak() returns false if the given node is a <br>.
https://dxr.mozilla.org/mozilla-central/rev/e5a10bc7dac4ee2453d8319165c1f6578203eac7/editor/libeditor/nsHTMLEditor.cpp#956,959-960

So, looks like you're ignoring all <br> elements. Does this work as expected with <p><br><br></p>, etc?
>+function doTest(callback, expected) {

nit: callback sounds like it's called after running the test for notifying the caller of result. Perhaps, aTest or something is better?

>+SimpleTest.waitForFocus(function() {
>+  // Put the selection on the second line, backspace, and enter a character.
>+  // It should be before the <br> on the first line.
>+  doTest(function() {
>+    getSelection().collapse(div.lastChild, 0);
>+    synthesizeKey("VK_BACK_SPACE", {});
>+  });
>+
>+  // Some variants for completeness: forward delete, non-collapsed selection
>+  // selected both forwards and backwards with both forward and backward delete
>+  doTest(function() {
>+    getSelection().collapse(div.firstChild, 0);
>+    synthesizeKey("VK_DELETE", {});
>+  });
>+
>+  // These all remove the <p> as well, which seems wrong, so they're todo.
>+  doTest(function() {
>+    getSelection().collapse(div.firstChild, 0);
>+    getSelection().extend(div.lastChild, 0);
>+    synthesizeKey("VK_BACK_SPACE", {});
>+  }, "x<br>");
>+  doTest(function() {
>+    getSelection().collapse(div.firstChild, 0);
>+    getSelection().extend(div.lastChild, 0);
>+    synthesizeKey("VK_DELETE", {});
>+  }, "x<br>");
>+
>+  doTest(function() {
>+    getSelection().collapse(div.lastChild, 0);
>+    getSelection().extend(div.firstChild, 0);
>+    synthesizeKey("VK_BACK_SPACE", {});
>+  }, "x<br>");
>+  doTest(function() {
>+    getSelection().collapse(div.lastChild, 0);
>+    getSelection().extend(div.firstChild, 0);
>+    synthesizeKey("VK_DELETE", {});
>+  }, "x<br>");

I want some tests which check the behavior with continuous <br> nodes. (I guess that <br style="display:block"> works odd too, but I think that it's not in the scope of this bug.)
Attachment #8748582 - Flags: review?(masayuki) → review-
Attached file Two test cases. — 
While the test passes, the patch does actually *not* fix the problem I reported and I can't see an improvement during manual testing at all. Please take a look at the two examples I'm submitting here.

I tried the "minimal testcase" from comment #3
  data:text/html,<!doctype html><div contenteditable><p><br><p><br></p></div>
and that works fine now.

So while this seems to be going into the right direction it needs to go much further ;-)
Attachment #8742918 - Attachment is obsolete: true
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 4/29-5/8) from comment #16)
> nsHTMLEditor::IsVisBreak() returns false if the given node is a <br>.
> https://dxr.mozilla.org/mozilla-central/rev/
> e5a10bc7dac4ee2453d8319165c1f6578203eac7/editor/libeditor/nsHTMLEditor.
> cpp#956,959-960

Those lines say it returns false if the given node is *not* a <br>.

> nit: callback sounds like it's called after running the test for notifying
> the caller of result. Perhaps, aTest or something is better?

The word "callback" just means "a function that's passed as a function argument", according to Wikipedia.

> I want some tests which check the behavior with continuous <br> nodes.

I should also test <p>x<br></p>.  But if it actually behaves correctly in these cases, it seems like it could be landed now and I could do all this in a follow-up, if there are no other problems.

> (I
> guess that <br style="display:block"> works odd too, but I think that it's
> not in the scope of this bug.)

All the editing code ignores CSS 'display'.  E.g., the functions IsBlockNode() and IsInlineNode(), which are used in a ton of places.  It just goes by the element name.  I think this is wrong, but it's definitely not in the scope of this bug (and doesn't make a big difference in real life anyway).

(In reply to Jorg K (PTO during summer, NI me) from comment #17)
> While the test passes, the patch does actually *not* fix the problem I
> reported and I can't see an improvement during manual testing at all.

Whoops!  I'll have to take another look when I get back.
Attachment #8748582 - Flags: review- → review?(masayuki)
What's the point of reviewing/landing this if it clearly doesn't fix the real life cases?
Even if your currently not working for Mozilla, perhaps you have a local build and can just spend five minutes with attachment 8750251 [details] to see what I mean.

(You had a whole stack of patches in the editor clean-up hibernating for a while, so I don't see the rush with these few lines here. Just MHO.)
I should add: If it's not fixed here, we need another bug and track this other bug, etc.
(In reply to :Aryeh Gregor (away until August 15) from comment #18)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 4/29-5/8)
> from comment #16)
> > nsHTMLEditor::IsVisBreak() returns false if the given node is a <br>.
> > https://dxr.mozilla.org/mozilla-central/rev/
> > e5a10bc7dac4ee2453d8319165c1f6578203eac7/editor/libeditor/nsHTMLEditor.
> > cpp#956,959-960
> 
> Those lines say it returns false if the given node is *not* a <br>.

Ah, yes.

> > nit: callback sounds like it's called after running the test for notifying
> > the caller of result. Perhaps, aTest or something is better?
> 
> The word "callback" just means "a function that's passed as a function
> argument", according to Wikipedia.

Okay, but I've never seen the usage like your test with "callback", though.

> > I want some tests which check the behavior with continuous <br> nodes.
> 
> I should also test <p>x<br></p>.  But if it actually behaves correctly in
> these cases, it seems like it could be landed now and I could do all this in
> a follow-up, if there are no other problems.

I worry about |<p><br><br></p>|, |<p><br><br><br></p>|, etc, especially when caret is between <br> elements. Therefore, I want some tests for them.
(In reply to Jorg K (PTO during summer, NI me) from comment #19)
> What's the point of reviewing/landing this if it clearly doesn't fix the
> real life cases?

It may fix other real-life cases.  Yes, of course this means a new bug to fix the actual original reported bug.

> Even if your currently not working for Mozilla, perhaps you have a local
> build and can just spend five minutes with attachment 8750251 [details] to
> see what I mean.

I don't have a working development machine, only e-mail/web browsing.

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 4/29-5/8) from comment #21)
> I worry about |<p><br><br></p>|, |<p><br><br><br></p>|, etc, especially when
> caret is between <br> elements. Therefore, I want some tests for them.

Okay, then I'll write some more tests when I get back.
Comment on attachment 8748582 [details] [diff] [review]
0002-Bug-1265800-Don-t-place-cursor-after-invisible-break.patch

(In reply to :Aryeh Gregor (away until August 15) from comment #22)
> Yes, of course this means a new bug to
> fix the actual original reported bug.
Aryeh, I really appreciate that you've been looking into the bug I reported. I would appreciate it even more if the bug got fixed as it was reported and I didn't have to report a new bug with exactly the same problem. If you want to fix something you detected, why don't you land this in a separate bug?

More to the point:
Your fix works in this case
<div contenteditable=""><p><br></p><p><br></p></div>
but it already doesn't work if you insert a few newlines, like so:
<div contenteditable="">
<p><br></p><p><br></p>
</div>

So I suggest to fix the problem property instead of fixing some partial aspect of it and forcing the reporter to go away and report it again in a new bug.

Or let me say it some other way. Masayuki wants another test, something with more <br>s.
I'd also like another test, as indicated above with newlines after/before the <div>/</div>.
Attachment #8748582 - Flags: review?(masayuki) → feedback-
So the trailing newline between the </p> and </div> in the test-case is actually essential to reproduce the bug somehow.  :/  Well, now I have a test, so let me work on fixing it.
I tried getting MozReview to work but haven't succeeded yet, sorry.

This patch takes a much better approach, reusing the existing GetGoodSelPointForNode method.  I had to fix a bug in IsVisBreak, which was claiming the second <br> in <p><br><br></p> was visible due to an error in ordering.  (It correctly considers all trailing <br>s in a block to be invisible, but incorrectly returns true earlier if it's preceded by another <br>.)  If you're not comfortable with this, I can move it to GetGoodSelPointForNode, which should be safe.

I also changed the test to wpt so that other UAs can use it for regression testing (although there's no real spec).  I relied on the fact the execCommand("delete") is the same as hitting backspace, which I think is always true for us.

Anyway, the original reported bug is reproduced by the last two of the six failures, which are fixed in the next patch.
Attachment #8748582 - Attachment is obsolete: true
Attachment #8782466 - Flags: review?(masayuki)
Yes, I tested, this fixes the originally reported test-case.  :)  I just started a try run for this version, but based on previous try runs I expect it to be green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=753757a6984f
Attachment #8782467 - Flags: review?(masayuki)
(In reply to :Aryeh Gregor (working until September 2) from comment #26)
> Yes, I tested, this fixes the originally reported test-case.  :)
That's great, thanks.

I think you need to refresh attachment 8748580 [details] [diff] [review].
I already rebased it locally, I don't need to post it for another review.
Comment on attachment 8782467 [details] [diff] [review]
0003-Bug-1265800-part-2-Move-cursor-into-all-adjacent-nod.patch

>@@ -4828,20 +4828,22 @@ HTMLEditRules::CheckForEmptyBlock(nsINode* aStartNode,
>         res = aSelection->Collapse(blockParent, offset + 1);

Hmm, I'd like you to move this into else block of following if block because it might fail if the point isn't good for selection and I don't like to do same thing in such close area.

>         NS_ENSURE_SUCCESS(res, res);
> 
>-        // Move to the start of the next node if it's a text.
>+        // Move to the start of the next node
>         nsCOMPtr<nsIContent> nextNode = mHTMLEditor->GetNextNode(blockParent,
>                                                                  offset + 1, true);
>-        if (nextNode && mHTMLEditor->IsTextNode(nextNode)) {
>-          res = aSelection->Collapse(nextNode, 0);
>+        if (nextNode) {
>+          EditorDOMPoint pt = GetGoodSelPointForNode(*nextNode, aAction);
>+          res = aSelection->Collapse(pt.node, pt.offset);
>           NS_ENSURE_SUCCESS(res, res);
>         }
Attachment #8782467 - Flags: review?(masayuki) → review+
Comment on attachment 8782466 [details] [diff] [review]
0002-Bug-1265800-part-1-Don-t-place-cursor-after-invisibl.patch

(In reply to :Aryeh Gregor (working until September 2) from comment #25)
> Created attachment 8782466 [details] [diff] [review]
> 0002-Bug-1265800-part-1-Don-t-place-cursor-after-invisibl.patch
> 
> I tried getting MozReview to work but haven't succeeded yet, sorry.

What's error blocking you??

> This patch takes a much better approach, reusing the existing
> GetGoodSelPointForNode method.  I had to fix a bug in IsVisBreak, which was
> claiming the second <br> in <p><br><br></p> was visible due to an error in
> ordering.  (It correctly considers all trailing <br>s in a block to be
> invisible, but incorrectly returns true earlier if it's preceded by another
> <br>.)  If you're not comfortable with this, I can move it to
> GetGoodSelPointForNode, which should be safe.

No, sounds reasonable.

>Bug 1265800 part 1 - Don't place cursor after invisible break; r?masayuki

Oh, the previous patch becomes part.0? Or becoming obsolete? If the latter, please mark it as obsolete.

>@@ -2486,7 +2486,8 @@ HTMLEditRules::InsertBRIfNeeded(Selection* aSelection)

Hmm, it's very hard to review with "-U 3 p". Please use "-U 8 p" instead.

>@@ -2497,7 +2498,8 @@ HTMLEditRules::GetGoodSelPointForNode(nsINode& aNode,
>                                       nsIEditor::EDirection aAction)
> {
>   NS_ENSURE_TRUE(mHTMLEditor, EditorDOMPoint());
>-  if (aNode.GetAsText() || mHTMLEditor->IsContainer(&aNode)) {
>+  if (aNode.GetAsText() || mHTMLEditor->IsContainer(&aNode) ||
>+      !aNode.GetParentNode()) {

Should be wrapped by NS_WARN_IF()? Or does this occur usually?

>@@ -7360,22 +7362,8 @@ HTMLEditRules::AdjustSelection(Selection* aSelection,
> 
>   if (nearNode)
>   {

nit: I hope you move this |{| to the end of previous line because you're touching this block.

>-    // is the nearnode a text node?
>-    textNode = do_QueryInterface(nearNode);
>-    if (textNode)
>-    {
>-      int32_t offset = 0;
>-      // put selection in right place:
>-      if (aAction == nsIEditor::ePrevious)
>-        textNode->GetLength((uint32_t*)&offset);
>-      res = aSelection->Collapse(nearNode,offset);
>-    }
>-    else  // must be break or image
>-    {
>-      selNode = EditorBase::GetNodeLocation(nearNode, &selOffset);
>-      if (aAction == nsIEditor::ePrevious) selOffset++;  // want to be beyond it if we backed up to it
>-      res = aSelection->Collapse(selNode, selOffset);
>-    }
>+    EditorDOMPoint pt = GetGoodSelPointForNode(*nearNode, aAction);
>+    res = aSelection->Collapse(pt.node, pt.offset);
>   }
>   return res;

Okay, then, please use early return style here.

if (!nearNode) {
  return NS_OK;
}
EditorDOMPoint pt = GetGoodSelPointForNode(*nearNode, aAction);
res = aSelection->Collapse(pt.node, pt.offset);
if (NS_WARN_IF(NS_FAILED(res))) {
  return res;
}
return NS_OK;

And also putting warning might help to debug if pt is bad position.

>diff --git a/editor/libeditor/HTMLEditor.cpp b/editor/libeditor/HTMLEditor.cpp
>index 196c969..f4d20e9 100644
>--- a/editor/libeditor/HTMLEditor.cpp
>+++ b/editor/libeditor/HTMLEditor.cpp
>@@ -953,10 +953,6 @@ HTMLEditor::IsVisBreak(nsINode* aNode)

As I filed bug 1296506, GetPriorHTMLNode() and GetNextHTMLNode() return an editable node. So, e.g., in following case, looks like this method won't work as expected:

<div contenteditable>foo<br><span contenteditable="false">bar</span></div>

I don't mind if you don't touch this issue in this bug, though (I've never seen partially uneditable editor in the real world).

>     return false;
>   }
>   // Check if there is a later node in block after br
>-  nsCOMPtr<nsINode> priorNode = GetPriorHTMLNode(aNode, true);
>-  if (priorNode && TextEditUtils::IsBreak(priorNode)) {
>-    return true;
>-  }
>   nsCOMPtr<nsINode> nextNode = GetNextHTMLNode(aNode, true);
>   if (nextNode && TextEditUtils::IsBreak(nextNode)) {
>     return true;
>@@ -972,6 +968,13 @@ HTMLEditor::IsVisBreak(nsINode* aNode)
>     return false;
>   }
> 
>+  // If there's an inline node after this one that's not a break, and also a
>+  // prior break, this break must be visible.
>+  nsCOMPtr<nsINode> priorNode = GetPriorHTMLNode(aNode, true);
>+  if (priorNode && TextEditUtils::IsBreak(priorNode)) {
>+    return true;
>+  }

Hmm, I couldn't understand why |<br><br></div>| causes its second <br> being visible until I write a testcase.

Could you explain the 2nd <br> representing empty line at the end of a block element in the comment?

>+var tests = [
>+  ["<p><br></p><p><br></p>", "1,0", "<p><br></p>", "0,0", "delete"],

How about to add id for testing which <p> is removed? (Same for the others.)


>+for (var i = 0; i < tests.length; i++) {
>+  test(function() {
>+    var test = tests[i];
>+    div.innerHTML = test[0];
>+    setSelection(test[1]);
>+
>+    document.execCommand("delete", false, "");

Not test[4]? Do I misunderstand?

>+function getPointFromArray(offsets) {
>+  var retNode = div, retOffset;
>+  var offset;
>+  while (offset = offsets.shift()) {
>+    if (!offsets.length) {
>+      retOffset = offset;
>+    } else {
>+      retNode = retNode.childNodes[offset];

Hmm, making this text aware might help the other developers who want to add some tests but up to you.
Attachment #8782466 - Flags: review?(masayuki) → review+
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #30)
> What's error blocking you??

Bug 1213668 (my network setup is unfortunately weird).

> >+  if (aNode.GetAsText() || mHTMLEditor->IsContainer(&aNode) ||
> >+      !aNode.GetParentNode()) {
> 
> Should be wrapped by NS_WARN_IF()? Or does this occur usually?

It would be very strange, so I wrapped it.

> And also putting warning might help to debug if pt is bad position.

Do you prefer if(NS_WARN_IF(NS_FAILED(res))) { return res; } to NS_ENSURE_SUCCESS(res, res)?  editor currently consistently uses the latter style, which Ehsan prefers, although I understand the style guide these days says to not use NS_ENSURE_* because they obscure control flow if you're not used to them.

> As I filed bug 1296506, GetPriorHTMLNode() and GetNextHTMLNode() return an
> editable node. So, e.g., in following case, looks like this method won't
> work as expected:
> 
> <div contenteditable>foo<br><span contenteditable="false">bar</span></div>
> 
> I don't mind if you don't touch this issue in this bug, though (I've never
> seen partially uneditable editor in the real world).

In theory authors sometimes want chunks of content to be indivisible units within the contenteditable area -- e.g., some sort of widget like a video player that you don't want the user to be able to edit internally.  In practice I don't know if anyone does this.  (In practice most contenteditable users write their own code for user input handling, so all this is moot.)

I agree that this method is buggy, though.

> >+var tests = [
> >+  ["<p><br></p><p><br></p>", "1,0", "<p><br></p>", "0,0", "delete"],
> 
> How about to add id for testing which <p> is removed? (Same for the others.)

It looks like we keep the first element, but we strip all attributes from it for some reason.  This seems like a bug, but I don't want to these tests to test for it because the failures obscure what we're actually testing for.  It's probably already tested in editing/run/delete.html (although I'm not totally sure).

> >+function getPointFromArray(offsets) {
> >+  var retNode = div, retOffset;
> >+  var offset;
> >+  while (offset = offsets.shift()) {
> >+    if (!offsets.length) {
> >+      retOffset = offset;
> >+    } else {
> >+      retNode = retNode.childNodes[offset];
> 
> Hmm, making this text aware might help the other developers who want to add
> some tests but up to you.

What do you mean "text aware"?  .childNodes isn't used for the last offset in the sequence, which is the only one that can be a text node.
Flags: needinfo?(masayuki)
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3b15fe52734
part 1 - Small unrelated cleanup patch; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/bba587e03e0b
part 2 - Don't place cursor after invisible break; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/da1cdd000993
part 3 - Move cursor into all adjacent nodes after delete; r=masayuki
Backed out for failing added wpt /XMLHttpRequest/event-error.html (which looks unrelated to the patch):

https://hg.mozilla.org/integration/mozilla-inbound/rev/f90ad145c5fcb401b4e182d691bf9e84f10e80dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/67481703d1103cad368284fc820192e6c54947ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc62f0119558c5d6dfad7b727b94f595e999e2ac

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=da1cdd0009930b9c2b70e50709ae01b53a75fa04
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=34353168&repo=mozilla-inbound

09:52:48     INFO - PROCESS | 1910 | 1471798368656	Marionette	TRACE	conn6 <- [1,328,null,{"value":["/XMLHttpRequest/event-error.html",0,null,null,[["XMLHttpRequest Test: event - error",1,"An invalid or illegal string was specified","@http://web-platform.test:8000/XMLHttpRequest/event-error.html:21:3\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1403:20\nasync_test@http://web-platform.test:8000/resources/testharness.js:518:13\n@http://web-platform.test:8000/XMLHttpRequest/event-error.html:13:1\n"]]]}]
09:52:48     INFO - 
09:52:48     INFO - TEST-UNEXPECTED-FAIL | /XMLHttpRequest/event-error.html | XMLHttpRequest Test: event - error - An invalid or illegal string was specified
09:52:48     INFO - @http://web-platform.test:8000/XMLHttpRequest/event-error.html:21:3
09:52:48     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1403:20
09:52:48     INFO - async_test@http://web-platform.test:8000/resources/testharness.js:518:13
09:52:48     INFO - @http://web-platform.test:8000/XMLHttpRequest/event-error.html:13:1
Flags: needinfo?(ayg)
(In reply to :Aryeh Gregor (working until September 2) from comment #31)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #30)
> > What's error blocking you??
> 
> Bug 1213668 (my network setup is unfortunately weird).

Thanks. Looks like it'll be fixed soon.

> > And also putting warning might help to debug if pt is bad position.
> 
> Do you prefer if(NS_WARN_IF(NS_FAILED(res))) { return res; } to
> NS_ENSURE_SUCCESS(res, res)?  editor currently consistently uses the latter
> style, which Ehsan prefers, although I understand the style guide these days
> says to not use NS_ENSURE_* because they obscure control flow if you're not
> used to them.

Yeah, I like NS_WARN_IF() better because new code in other modules already start to use NS_WARN_IF(). So, for consistency between other modules, I'd like to use NS_WARN_IF() under editor too (but not need to replace existing NS_ENSURE_* at fixing a bug).

> > >+var tests = [
> > >+  ["<p><br></p><p><br></p>", "1,0", "<p><br></p>", "0,0", "delete"],
> > 
> > How about to add id for testing which <p> is removed? (Same for the others.)
> 
> It looks like we keep the first element, but we strip all attributes from it
> for some reason.  This seems like a bug, but I don't want to these tests to
> test for it because the failures obscure what we're actually testing for. 
> It's probably already tested in editing/run/delete.html (although I'm not
> totally sure).

Okay fine for now. But sounds like that removing attributes may cause changing the style (may be pointed by attribute selector for modifying its style). So, we should fix it later.

> > >+function getPointFromArray(offsets) {
> > >+  var retNode = div, retOffset;
> > >+  var offset;
> > >+  while (offset = offsets.shift()) {
> > >+    if (!offsets.length) {
> > >+      retOffset = offset;
> > >+    } else {
> > >+      retNode = retNode.childNodes[offset];
> > 
> > Hmm, making this text aware might help the other developers who want to add
> > some tests but up to you.
> 
> What do you mean "text aware"?  .childNodes isn't used for the last offset
> in the sequence, which is the only one that can be a text node.

Oh, I meant "text node aware" because if the childNodes[offset] shouldn't be called with text node.

But indeed, the last offset should not be here. Okay, fine.
Flags: needinfo?(masayuki)
Sorry, the problem was bug 1295987.  I hope it will be fixed soon (or I'll find a workaround), but I don't know.
Flags: needinfo?(ayg)
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b0690c27a15
part 1 - Small unrelated cleanup patch; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/7297b724b661
part 2 - Don't place cursor after invisible break; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/27c7692478d9
part 3 - Move cursor into all adjacent nodes after delete; r=masayuki
Comment on attachment 8782466 [details] [diff] [review]
0002-Bug-1265800-part-1-Don-t-place-cursor-after-invisibl.patch

Review of attachment 8782466 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/meta/MANIFEST.json
@@ +37628,4 @@
>          ]
>        },
>        "testharness": {
> +        "XMLHttpRequest/event-error.html": [

This was the problem.  I ran the wpt script that rebuilds the manifest because I added a new test, and it added this other test to the manifest also.  But the expected failure wasn't recorded.  I removed this bit now.  (I meant to include it in the updated patch itself, but got confused by my VCS, so I didn't, and pushed as a separate patch afterwards.)
That actually landed at Mon Aug 22 15:28:39 2016 +0000, early enough to cause some Thunderbird Mozmill failures which I haven't analysed, see bug 1297175.

That test digs around the DOM of a document. Somehow something has subtly changed ;-) not that I'm complaining since I'm grateful that the bug I reported got fixed.
Do we want this to ride the train?
status-firefox48: affected → wontfix
status-firefox49: --- → affected
status-firefox50: --- → affected
The fix is not high-impact, and has relatively high regression risk (like all editor changes), so I don't see any reason to backport it, unless I'm missing something.
Thanks for fixing this, much appreciated!!
Depends on: 1314790
Depends on: 1355792
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: