Open Bug 1310913 Opened 8 years ago Updated 2 years ago

It required 2 stroke of [Backspace]or[Delete] to delete a char

Categories

(Core :: DOM: Editor, defect, P3)

Product:
Core
Component:
DOM: Editor
Platform:
x86
Windows 10
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox52 --- wontfix

People

(Reporter: alice0775, Assigned: m_kato)

References

(
URL
)

Details

(Keywords: parity-chrome, parity-edge, Whiteboard: [tpi:+])

Attachments

(3 files)

test case
398 bytes, text/html
Details
Bug 1310913 - Part 1. Allow backspace when current selection is empty text nodes only.
58 bytes, text/x-review-board-request
masayuki
: review-
Details
Bug 1310913 - Part 2. Add test.
58 bytes, text/x-review-board-request
masayuki
: review+
Details 
Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/94b0fddf96b43942bdd851a3275042909ea37e09
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0 ID:20161017030209

Using Windows10 home x64.
Problem occurs regardless of the e10s.

Reproducible : always

Steps To Reproduce:
1. Open https://keep.google.com/ and logged in
2. Click on input field [Take a note...]
3. Click on three dots menu(more) in the box
4. Chose "Show checkboxes"  (The problem does not occur if the checkboxes is not shown)
5. Type any text and Done

6. Click on the note
8. Attepmt to delete a char using [Backspace]or[Delete] key
   ---- Observe, It required 2 stroke to delete a char.

Actual Results:
It required 2 stroke to delete a char.


Expected Results:
no so
Flags: needinfo?(masayuki)
URL: https://keep.google.com/
I'd like to see a minimized testcase...
Flags: needinfo?(masayuki)
Keywords: testcase-wanted
Priority: -- → P2
Whiteboard: [parity-chrome][parity-edge] → [parity-chrome][parity-edge][tpi:+]
When using range.insertNode(), current caret is hidden state (collapsed is false).  So [backspace] isn't work correctly.
Chrome's behavior is different.  After range.insertNode, window.getSelection().getRangeAt(0) returns another object on Chrome.
Attached file test case — 
- Step
1. input "a" into div element
2. type [backspace] key

- Result
[backspace] doesn't work.  but type [a] works.
Component: Widget: Win32 → Editor
Smaug, I have a question for DOM's range spec.  Gecko and Blink are different behavior after range.insertNode() and I cannot find correct behavior from spec.

<!DOCTYPE html>
<html>
<head>
<script type="application/javascript">
function init() {
  document.getElementById("edit1").addEventListener("input", function(e) {
    let range = window.getSelection().getRangeAt(0);
    console.log("before -> collapsed: " + range.collapsed);
    range.insertNode(document.createTextNode(""));
    console.log("after -> collapsed: " + range.collapsed);
    console.log("new range's collapsed: " + window.getSelection().getRangeAt(0).collapsed);
  });
}
</script>
</head>
<body onload="init()">
<div contenteditable="true" id="edit1">edit me</div>
</body>
</html>

When typing any character on content editable, Gecko and Edge output the following to console

before -> collapsed: true
after -> collapsed: false
new range's collapsed: false

But using Blink, (it seems to create new Range object after range.insertNode)

before -> collapsed: true
after -> collapsed: false
new range's collapsed: true

Which correct behavior as spec?  Or, should I file a issue to https://github.com/whatwg/dom/issues/new?
Flags: needinfo?(bugs)
Isn't the issue not in Range here, but blink having broken
getSelection().getRangeAt().
It returns always a new Range, but it shouldn't.

http://w3c.github.io/selection-api/#widl-Selection-getRangeAt-Range-unsigned-long-index
https://bugs.chromium.org/p/chromium/issues/detail?id=613658#c84
Flags: needinfo?(bugs)
So, file a chromium bug?
And is this a tech evang issue?

Does the site work in Edge?
Flags: needinfo?(kdubost)
Bug 1309587 sounds similar to this.
See Also: → 1309587
(In reply to Olli Pettay [:smaug] from comment #7)
> And is this a tech evang issue?
> 
> Does the site work in Edge?

The test case works the same in Edge as in Chrome (as well as the STR on keep.google.com).
Flags: needinfo?(kdubost)
(In reply to Mike Taylor [:miketaylr] from comment #10)
> (In reply to Olli Pettay [:smaug] from comment #7)
> > And is this a tech evang issue?
> > 
> > Does the site work in Edge?
> 
> The test case works the same in Edge as in Chrome (as well as the STR on
> keep.google.com).

[Backspace] key on Gecko doesn't work in HTMLEditRules::WillDeleteSelection when collapsed=false.  So by range.insertNode, it doesn't work.  Also, due to collapsed=false, caret becomes hidden state on Gecko.
When I type something in the testcase, selection start node is the text node which has the inputted character and offset is after it, selection end node is the parent node and the offset is the inserted empty text node. It seems that we should improve the logic to check if selection range is empty.
https://jsfiddle.net/d_toybox/Lt61x6qn/
Oops, it's wrong. When you type "a" at selection is: "ed[]it me", selection becomes 3 of "eda" and 2 of <div>. That means that selection range becomes "eda[it me]" but not painted as so. I don't understand what occurs. However, it seems that setting selection is wrong after Range.insertNode().
Assignee: nobody → m_kato
Google keep fixes this issue.
Keywords: testcase-wanted
Comment on attachment 8816377 [details]
Bug 1310913 - Part 1. Allow backspace when current selection is empty text nodes only.

https://reviewboard.mozilla.org/r/97124/#review97408

I think that the approach is really fine to me. But I don't think the utility method isn't useful because the implementation is really this case specific.

I think that it should return true if selection is _actually_ collapsed at a text node even if selection range pointed from its parent element. Perhaps, you should create another utility method which returns a text node at given DOM point. E.g.,

bool
EditorUtils::GetTextNodeAt(nsINode& aNode, int32_t aOffset)
{
  if (EditorBase::IsTextNode(&aNode)) {
    return &aNode;
  }
  if (!aNode.HasChildren() ||
      aNode.GetChildCount() >= static_cast<uint32_t>(aOffset)) {
    return nullptr;
  }
  nsINode* child = aNode.GetChildAt(aOffset);
  return EditorBase::IsTextNode(child) ? child : nullptr;
}

If one of start node or end node returns nullptr, it should return false.

And then, both text nodes should be children of same element node. If they're in different element, return false.

The start offset should be end of the node. The end offset should be start of the node. Otherwise, return false.

Finally, you can check all nodes in the range with simple for loop which checks children of the common parent only between the start offset + 1 and the end offset - 1. Then, all nodes should be empty text nodes.

# Although, when end of range is [div, 2] and it's non-empty text node, I'm not sure if it means start of the text node or end of the text node...

::: editor/libeditor/EditorUtils.cpp:180
(Diff revision 1)
>      aNode->HasChildNodes(&hasChildren);
>    return !hasChildren;
>  }
>  
> +bool
> +EditorUtils::IsSelectionEmtpyTextNodes(Selection* aSelection)

This name isn't clear at least to me what this does.

How about IsSelectionCollapsedAtTextNode() and use it instead of existing collapsed method?

::: editor/libeditor/EditorUtils.cpp:182
(Diff revision 1)
>  }
>  
> +bool
> +EditorUtils::IsSelectionEmtpyTextNodes(Selection* aSelection)
> +{
> +  if (!aSelection->RangeCount()) {

Perhaps, printing warning might help debug.

::: editor/libeditor/EditorUtils.cpp:186
(Diff revision 1)
> +  nsCOMPtr<nsINode> startNode = aSelection->GetRangeAt(0)->GetStartParent();
> +  nsCOMPtr<nsINode> endNode = aSelection->GetRangeAt(0)->GetEndParent();
> +
> +  // If start node isn't text node or offset isn't tail, selection won't be
> +  // empty text node
> +  if (!startNode || !endNode || !EditorBase::IsTextNode(startNode)) {
> +    return false;
> +  }
> +  int32_t startOffset = aSelection->GetRangeAt(0)->StartOffset();
> +  int32_t endOffset = aSelection->GetRangeAt(0)->EndOffset();

I'd like you to get the first range to a variable.

::: editor/libeditor/EditorUtils.cpp:191
(Diff revision 1)
> +  nsCOMPtr<nsINode> startNode = aSelection->GetRangeAt(0)->GetStartParent();
> +  nsCOMPtr<nsINode> endNode = aSelection->GetRangeAt(0)->GetEndParent();
> +
> +  // If start node isn't text node or offset isn't tail, selection won't be
> +  // empty text node
> +  if (!startNode || !endNode || !EditorBase::IsTextNode(startNode)) {

I think that when |!startNode| or |!endNode| case, this should print warning.

On the other hand, I feel odd about |!EditorBase::IsTextNode(startNode)|. Imagine this case, when there is following tree:

<div>
 <#text>abc</#text>
 <#text/>
 <#text>edf</#text>
</div>

empty text node can be selected with following ranges:

1. collapsed at 0 at the empty text node
2. collapsed at 1 at the div element
3. 3 at the first text node and 0 at the empty text node
4. 3 at the first text node and 0 at the last text node
5. 0 at the empty text node and 0 at the last text node

So, in the #2 case, this method won't work as expected, isn't it?

Additionally, we can think following cases:

<div>
  <#text/>
  <#text>abc</#text>
</div>

<div>
  <#text>abc</#text>
  <#text/>
</div>

<div>
  <#text/>
</div>

In these cases, start or end node can be div element.

::: editor/libeditor/EditorUtils.cpp:199
(Diff revision 1)
> +  if (startOffset != static_cast<int32_t>(startNode->Length())) {
> +    return false;
> +  }

If the range is started from empty text node and to the start of next text node, doesn't this work as expected?
Attachment #8816377 - Flags: review?(masayuki) → review-
Comment on attachment 8816378 [details]
Bug 1310913 - Part 2. Add test.

https://reviewboard.mozilla.org/r/97126/#review97426

::: editor/libeditor/tests/test_bug1310913.html:39
(Diff revision 1)
> +  synthesizeKey("VK_BACK_SPACE", {});
> +  window.getSelection().getRangeAt(0).insertNode(document.createTextNode(""));
> +  synthesizeKey("VK_BACK_SPACE", {});
> +
> +  ok(elm.textContent == "ABC", "'a' and 'b' should be removed by backspace");
> +
> +  sel.collapse(elm.childNodes[0], 0);
> +  window.getSelection().getRangeAt(0).insertNode(document.createTextNode(""));
> +  synthesizeKey("VK_BACK_SPACE", {});
> +
> +  ok(elm.textContent == "ABC", "no one is removed by backspace");

Could you check empty text node existence. If the result is different from both Edge and Chrome, mark the result is "todo".
Attachment #8816378 - Flags: review?(masayuki) → review+
Mass wontfix for bugs affecting firefox 52.
status-firefox52: affected → wontfix
Google Keep fixes this.  So down to P3.
Priority: P2 → P3
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-chrome, parity-edge
Whiteboard: [parity-chrome][parity-edge][tpi:+] → [tpi:+]
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: