Bug 71771 – Inserting empty html moves caret

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Reduction:
<button id="button">Click on the text below, place your cursor somewhere in the middle of the text and press me</button><div id="content">testtesttesttesttesttesttesttest</div>
<script>
    document.getElementById("content").contentEditable = true;
    document.getElementById("button").onclick = function(){
        document.execCommand('InsertHTML', false, "");
    };
</script>

http://crbug.com/97999

[Antaryami Pandia (apandia)]

To find the issue I tried to compare the scenerios when we add a and non-empty string and when add an empty string.Below is my analysis:- 

Case 1. Insert a non-empty string
When we add a non-empty string the method "createFragmentFromMarkup" creates a fragments with child having the value of the string to be inserted.And then "executeInsertFragment" method is called. This method in turns calls the "ReplaceSelectionCommand::performTrivialReplace" method, which actually insert the string directly into the text node. Then it calculates the caret rect accordingly. If I have placed the cursor in 16th pos and clicked the button then it adds the string and calculates the width till 17th character and places the caret. Point to note is that while calculating the caret position the start position of the text node (to which the string is inserted) is considered. So if the start pos is (0,0) then the caret position should be (0+width of 17 characters, 0).

Case 2. Insert empty string 
When we add a empty string the method "createFragmentFromMarkup" creates a fragments with the child being set to false. In "ReplaceSelectionCommand::performTrivialReplace" there is a check for child of the fragments. Since the child is not there it return false. And the flow progress to call the "CompositeEditCommand::splitTextNode" method, which goes on and creates another rendertext with 16 characters(I have placed the cursor in 16th position and click on the button).

Because of this new text run, while calculating rect for the caret rather then considering the start position as the start of the text node (to which the empty string is inserted), it takes the end position of this new text run as the start position. say if the width of 16 charactes is 80px then, it takes the start pos as (80, 0). Hence it cacluates the caret position as (80 + width of 16 characters, 0) or (160, 0) which should be actually (80, 0).

Hence the caret is drawn on the wrong position.

I think the "DocumentFragment" plays a role  in deciding the flow of inserting a empty/non-empty string. So can we have a check for the child of documentfragment before proceeding with the "InsertHTML" command, say in "executeInsertFragment" method.

[Antaryami Pandia (apandia)]

Got disconnected while discussing with rniwa on IRC. So puting some queries here.

When I insert a character no new rendertext is created, it simply inserted into the existing text.But when I click on the middle of the text run (say 16th pos) and insert the empty string by clicking on the button, a new text run of 16 characters is created. Is this expected?

This new rendertext is craeted by a call to "splitTextNode" method. The "splitTextNode" method is called because the "performTrivialReplace" return false when we add an empty string. We do have a check for fragment.firstChild(), but I think the call to "splitTextNode" changes the behavior.

There is a code to check firstChild in "performTrivialReplace":-
    if (!fragment.firstChild() || fragment.firstChild() != fragment.lastChild() || !fragment.firstChild()->isTextNode())
        return false;

Is there any other case when the firstchild be null other then the case when inserting empty string?

[Ryosuke Niwa]

(In reply to comment #2)
> There is a code to check firstChild in "performTrivialReplace":-
>     if (!fragment.firstChild() || fragment.firstChild() != fragment.lastChild() || !fragment.firstChild()->isTextNode())
>         return false;
> 
> Is there any other case when the firstchild be null other then the case when inserting empty string?

Maybe when the inserted text wasn't empty but some event listener modified the text to be inserted? e.g. <input maxlength=0>

[Antaryami Pandia (apandia)]

Created attachment 137281 [details]
Patch for discussion.

As I have mentioned earlier, when we try to insert an empty string, the method "ReplaceSelectionCommand::performTrivialReplace" return false. And the flow progress to call the "CompositeEditCommand::splitTextNode" method. This method the calls "SplitTextNodeCommand::doApply" to perform next step. It is in this method that the DOm tree is modified.

In "SplitTextNodeCommand::doApply" first a substring is created till the current caret position. Means if the caret was at posion 16 when I click the button to insert empty string, then a substring is created with first 16 characters. Code snippet:-
    String prefixText = m_text2->substringData(0, m_offset, ec); 

After that a text node is craeted with that substring. Code snippet:-
    m_text1 = Text::create(document(), prefixText);

After crating the text node, it calls the "insertText1AndTrimText2" method. This method inserts the new text node before the actual text node.
    m_text2->parentNode()->insertBefore(m_text1.get(), m_text2.get(), ec);

The call to "splitTextNode" was added in http://wkbug.com/65824. It replaces the call to "splitTextNodeContainingElement", which was added in http://wkbug.com/56874.
And this block (in http://wkbug.com/56874 containing "splitTextNodeContainingElement") was placed before the check for fragment.

As per my understanding if we place the cursor in the middle of a text node and try to insert some text between, then we split text node. But in this case we have nothing to insert i.e the string to be inserted is empty. So I think check for fragments should precedes the call to code block containing splitTextNode. I am attaching the code change for discussion.

[Antaryami Pandia (apandia)]

Created attachment 137853 [details]
Patch.

[Ryosuke Niwa]

Comment on attachment 137853 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=137853&action=review

> LayoutTests/editing/inserting/insert-empty-html.html:6
> +<head>
> +<script src="../../fast/js/resources/js-test-pre.js"></script>
> +</head>
> +

You don't need to have a head for this. You can just move js-test-pre.js to body.

> LayoutTests/editing/inserting/insert-empty-html.html:19
> +    var sel = window.getSelection();

Please don't use abbreviations like sel.

> LayoutTests/editing/inserting/insert-empty-html.html:23
> +    // Set the caret position

This comment repeats the code. Please remove.

> LayoutTests/editing/inserting/insert-empty-html.html:26
> +    // Insert the empty html

Ditto.

[Antaryami Pandia (apandia)]

Created attachment 138045 [details]
Updated patch.

Patch with review comments.

[WebKit Review Bot]

Comment on attachment 138045 [details]
Updated patch.

Clearing flags on attachment: 138045

Committed r114718: <http://trac.webkit.org/changeset/114718>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.