Bug 115023 – Editing: wrong text position when you click enter on the text behind the image

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Krzysztof Wolanski]

Steps to Reproduce:
  1)Open the website that contains:
    <DIV>
    <SPAN style="color:#FF0000">text1<IMG src="black.jpg"/>text2</SPAN>
    </DIV>
  2) Press enter in front of "text2".

Actual Results: 
  "ext2" goes the line below, "t" stays in the same line as image.

Expected Results:
  "text2" goes the line below.

Build Date & Platform:
  Build 2013-04-22 on Ubuntu 12.04 LTS

[Krzysztof Wolanski]

Created attachment 199172 [details]
HTML page, shows unexpected behavior

[Arpita Bahuguna]

Created attachment 200900 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 200900 [details]
Patch

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

> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:175
>          || isTableCell(startBlock.get())
>          || startBlock->hasTagName(formTag)
>          // FIXME: If the node is hidden, we don't have a canonical position so we will do the wrong thing for tables and <hr>. https://bugs.webkit.org/show_bug.cgi?id=40342
> -        || (!canonicalPos.isNull() && canonicalPos.deprecatedNode()->renderer() && canonicalPos.deprecatedNode()->renderer()->isTable())
> +        || (!canonicalPos.isNull() && canonicalPos.deprecatedNode()->renderer()
> +            && (canonicalPos.deprecatedNode()->renderer()->isTable() || canonicalPos.deprecatedNode()->renderer()->isImage()))

It looks like we just want to call isAtomicNode here.

[Arpita Bahuguna]

(In reply to comment #3)
> (From update of attachment 200900 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200900&action=review
> 
> > Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:175
> >          || isTableCell(startBlock.get())
> >          || startBlock->hasTagName(formTag)
> >          // FIXME: If the node is hidden, we don't have a canonical position so we will do the wrong thing for tables and <hr>. https://bugs.webkit.org/show_bug.cgi?id=40342
> > -        || (!canonicalPos.isNull() && canonicalPos.deprecatedNode()->renderer() && canonicalPos.deprecatedNode()->renderer()->isTable())
> > +        || (!canonicalPos.isNull() && canonicalPos.deprecatedNode()->renderer()
> > +            && (canonicalPos.deprecatedNode()->renderer()->isTable() || canonicalPos.deprecatedNode()->renderer()->isImage()))
> 
> It looks like we just want to call isAtomicNode here.

Hi Ryosuke, thanks for the review and apologize for the delayed reply.

I did try using isAtomicNode() (or editingIgnoresContent()) but get failures for <br> and <object> tags.
[Fails: editing/inserting/return-with-object-element.html, editing/inserting/insert-div-004.html, editing/inserting/insert-div-005.html among others.]
Rectyfying this would again result in additional condition checks.

Would it be better to stick with the patch/fix? Would appreciate your thoughts on the same.

[Ryosuke Niwa]

(In reply to comment #4)
>
> I did try using isAtomicNode() (or editingIgnoresContent()) but get failures for <br> and <object> tags.
> [Fails: editing/inserting/return-with-object-element.html, editing/inserting/insert-div-004.html, editing/inserting/insert-div-005.html among others.]
> Rectyfying this would again result in additional condition checks.

What happens if we replaced object and br elements in those tests with img/table?

It seems odd that we have to special case a table & an image here.

[Krzysztof Wolanski]

(In reply to comment #2)
> Created an attachment (id=200900) [details]
> Patch

This patch in fact moves the text below when you click enter. But if in the next step, you click backspace in front of this text, it will do nothing.

[Arpita Bahuguna]

(In reply to comment #6)
> (In reply to comment #2)
> > Created an attachment (id=200900) [details] [details]
> > Patch
> 
> This patch in fact moves the text below when you click enter. But if in the next step, you click backspace in front of this text, it will do nothing.

Hi Krzysztof,

Am aware of the issue you've mentioned but perhaps it should be pursued as a separate bug.(??)

It appears to be an existing issue which can be simulated with the following markup as well:
<div contenteditable="true">
<span>text1<input type="text"/><br>text2</span>
</div>
Placing the cursor before "text2" and then trying to backspace back to the previous line would replicate (and is) the precise scenario you have mentioned.
[works well on FF]

I will try and check this issue post this fix.

[Arpita Bahuguna]

(In reply to comment #5)
> (In reply to comment #4)
> >
> > I did try using isAtomicNode() (or editingIgnoresContent()) but get failures for <br> and <object> tags.
> > [Fails: editing/inserting/return-with-object-element.html, editing/inserting/insert-div-004.html, editing/inserting/insert-div-005.html among others.]
> > Rectyfying this would again result in additional condition checks.
> 
> What happens if we replaced object and br elements in those tests with img/table?
> 
> It seems odd that we have to special case a table & an image here.

We definitely do need to use the editingIgnoresContent() check here, since it would also cover scenarios that include <input> elements etc. but we'd also have to special case for <br> and <table> elements.
Changing (or in the case of <table>, adding) the value returned by canContainRangeEndPoint() doesn't seem to solve the problem as well.

A working patch would look something like:
    if (!startBlock
        || !startBlock->nonShadowBoundaryParentNode()
        || isTableCell(startBlock.get())
        || startBlock->hasTagName(formTag)
        || (!canonicalPos.isNull() && (canonicalPos.deprecatedNode()->renderer()->isTable()
            || (!canonicalPos.deprecatedNode()->renderer()->isBR() && editingIgnoresContent(canonicalPos.deprecatedNode()))))) {
        applyCommandToComposite(InsertLineBreakCommand::create(document()));
        return;
    }

However, even with this we get a failure for <object> element (editing/inserting/return-with-object-element.html) which I need to investigate further.

[Krzysztof Wolanski]

Hi Arpita, 
You are right, this is next bug. But your solution is not consistent with W3C standard. Look at: 
https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#the-insertparagraph-command 
"The general rule is that we find the nearest single-line container ancestor, clone it and insert the clone after it, and then move all the contents after the cursor (along with the cursor itself) to the clone"
"If we can't find a single-line container to use, first we wrap the current line in a new container with the default single-line container name."

[Arpita Bahuguna]

(In reply to comment #9)
> Hi Arpita, 
> You are right, this is next bug. But your solution is not consistent with W3C standard. Look at: 
> https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#the-insertparagraph-command 
> "The general rule is that we find the nearest single-line container ancestor, clone it and insert the clone after it, and then move all the contents after the cursor (along with the cursor itself) to the clone"
> "If we can't find a single-line container to use, first we wrap the current line in a new container with the default single-line container name."

Hi Krzysztof,

Thanks for pointing me to that link. What you say is indeed correct. I need to modify the fix, see why we are missing out on inserting the cloned container. However this same issue occurs with other elements as well and not just <img>.

Perhaps we should also figure out why <table> (and <hr>) are being special cased in the current implementation.
As per the spec, a line-break ought to be executed only if we do not intend to break out of the current block.

[Ryosuke Niwa]

(In reply to comment #9)
> Hi Arpita, 
> You are right, this is next bug. But your solution is not consistent with W3C standard. Look at: 
> https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#the-insertparagraph-command 
> "The general rule is that we find the nearest single-line container ancestor, clone it and insert the clone after it, and then move all the contents after the cursor (along with the cursor itself) to the clone"
> "If we can't find a single-line container to use, first we wrap the current line in a new container with the default single-line container name."

That specification is almost irrelevant to our implementation.  Trying to match the behavior in the specification is a boring exercise at the moment.

[Ryosuke Niwa]

Comment on attachment 200900 [details]
Patch

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

>>> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:175
>>> +            && (canonicalPos.deprecatedNode()->renderer()->isTable() || canonicalPos.deprecatedNode()->renderer()->isImage()))
>> 
>> It looks like we just want to call isAtomicNode here.
> 
> Hi Ryosuke, thanks for the review and apologize for the delayed reply.
> 
> I did try using isAtomicNode() (or editingIgnoresContent()) but get failures for <br> and <object> tags.
> [Fails: editing/inserting/return-with-object-element.html, editing/inserting/insert-div-004.html, editing/inserting/insert-div-005.html among others.]
> Rectyfying this would again result in additional condition checks.
> 
> Would it be better to stick with the patch/fix? Would appreciate your thoughts on the same.

Can I see the version of the patch that uses isAtomicNode or editingIgnoresContent and test failures?
It's possible that such a change reveals a bug existing elsewhere in our codebase.

[Arpita Bahuguna]

Created attachment 201949 [details]
WIP Patch

[Arpita Bahuguna]

Hi Ryosuke,

Have uploaded a WIP patch that takes a different approach towards fixing this issue, although perhaps a little hacky. :)

The insertionPosition computed for the point between the element which editing ignores (image) and the following text, is a position which is at the end of the non-editable node (image), i.e. with anchorNode as the image element and and anchorType set as PositionIsAfterAnchor.

It's downstream position would thus point to the start of the following text.

Thus we have insertionPosition pointing to the start of the start of the text after the following statement:
insertionPosition = insertionPosition.downstream();

Now when we compute the VisiblePosition for this, it again returns the upstreamed position i.e. at the end of the image element.
After this statement:
insertionPosition = positionOutsideTabSpan(VisiblePosition(insertionPosition).deepEquivalent());

Perhaps this position being computed is incorrect or should be considered invalid for this scenario and we should either move upstream or downstream (depending on whether we were at the last editing position or at the start) and use that for further processing.

This approach has perhaps stemmed from the FIXME comments mentioned in Position::atLastEditingPositionForNode() and Position::atFirstEditingPositionForNode() which say that the position after or before anchor (respectively) shouldn't be considered. These methods are called form isCandidate() while trying to obtain the upstream position when computing the VisiblePosition.

This approach too has it's share of problems, for example what to do when an image is followed by another image and we try to break in-between.

Would appreciate your thoughts on this. :)

[Arpita Bahuguna]

Created attachment 201952 [details]
Patch-using-editingIgnoresContent

[Arpita Bahuguna]

Comment on attachment 201952 [details]
Patch-using-editingIgnoresContent

This patch uses editingIgnoresContent() along with a check for BR element.

[Arpita Bahuguna]

(In reply to comment #12)
> (From update of attachment 200900 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200900&action=review
> 
> >>> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:175
> >>> +            && (canonicalPos.deprecatedNode()->renderer()->isTable() || canonicalPos.deprecatedNode()->renderer()->isImage()))
> >> 
> >> It looks like we just want to call isAtomicNode here.
> > 
> > Hi Ryosuke, thanks for the review and apologize for the delayed reply.
> > 
> > I did try using isAtomicNode() (or editingIgnoresContent()) but get failures for <br> and <object> tags.
> > [Fails: editing/inserting/return-with-object-element.html, editing/inserting/insert-div-004.html, editing/inserting/insert-div-005.html among others.]
> > Rectyfying this would again result in additional condition checks.
> > 
> > Would it be better to stick with the patch/fix? Would appreciate your thoughts on the same.
> 
> Can I see the version of the patch that uses isAtomicNode or editingIgnoresContent and test failures?
> It's possible that such a change reveals a bug existing elsewhere in our codebase.

Hi Ryosuke, have uploaded a patch (Patch-using-editingIgnoresContent). I suspect that this shall fail only the test-case editing/inserting/return-with-object-element.html since i've added a check for BR element.

[Build Bot]

Comment on attachment 201952 [details]
Patch-using-editingIgnoresContent

Attachment 201952 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/492053

New failing tests:
editing/inserting/return-with-object-element.html

[Build Bot]

Created attachment 201997 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Arpita Bahuguna]

Created attachment 202084 [details]
Patch

[Early Warning System Bot]

Comment on attachment 202084 [details]
Patch

Attachment 202084 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/492349

[Build Bot]

Comment on attachment 202084 [details]
Patch

Attachment 202084 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/495294

[Early Warning System Bot]

Comment on attachment 202084 [details]
Patch

Attachment 202084 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/482810

[EFL EWS Bot]

Comment on attachment 202084 [details]
Patch

Attachment 202084 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/487426

[Build Bot]

Comment on attachment 202084 [details]
Patch

Attachment 202084 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/495291

[EFL EWS Bot]

Comment on attachment 202084 [details]
Patch

Attachment 202084 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/480859

[Build Bot]

Comment on attachment 202084 [details]
Patch

Attachment 202084 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/482811

[kov's GTK+ EWS bot]

Comment on attachment 202084 [details]
Patch

Attachment 202084 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/481937

[Arpita Bahuguna]

Created attachment 202196 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 202196 [details]
Patch

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

> LayoutTests/editing/inserting/insert-paragraph-after-non-editable-node-before-text.html:6
> +<span id="imgTest">text1<img src="broken-image"/>text2</span>

Please use abe.png or something and wait for load event on the image. Otherwise this test can be flaky.

> LayoutTests/editing/inserting/insert-paragraph-after-non-editable-node-before-text.html:34
> +var test = document.getElementById('imgTest');
> +test.focus();
> +var selection = window.getSelection();
> +selection.collapse(test, test.childNodes.length - 1);
> +document.execCommand("InsertParagraph");
> +
> +test = document.getElementById('inputTest');
> +test.focus();
> +selection = window.getSelection();
> +selection.collapse(test, test.childNodes.length - 1);
> +document.execCommand("InsertParagraph");
> +
> +test = document.getElementById('objectTest');
> +test.focus();
> +selection = window.getSelection();
> +selection.collapse(test, test.childNodes.length - 1);
> +document.execCommand("InsertParagraph");
> +
> +

It seems like there are 3 test cases. Yet, we only dump the last one. r- because of this.

[Arpita Bahuguna]

Created attachment 204666 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #30)
> (From update of attachment 202196 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202196&action=review
> 
Hi Ryosuke, thanks for the review. Have uploaded another patch incorporating the specified layout test changes.

> > LayoutTests/editing/inserting/insert-paragraph-after-non-editable-node-before-text.html:6
> > +<span id="imgTest">text1<img src="broken-image"/>text2</span>
> 
> Please use abe.png or something and wait for load event on the image. Otherwise this test can be flaky.
> 
> > LayoutTests/editing/inserting/insert-paragraph-after-non-editable-node-before-text.html:34
> > +var test = document.getElementById('imgTest');
> > +test.focus();
> > +var selection = window.getSelection();
> > +selection.collapse(test, test.childNodes.length - 1);
> > +document.execCommand("InsertParagraph");
> > +
> > +test = document.getElementById('inputTest');
> > +test.focus();
> > +selection = window.getSelection();
> > +selection.collapse(test, test.childNodes.length - 1);
> > +document.execCommand("InsertParagraph");
> > +
> > +test = document.getElementById('objectTest');
> > +test.focus();
> > +selection = window.getSelection();
> > +selection.collapse(test, test.childNodes.length - 1);
> > +document.execCommand("InsertParagraph");
> > +
> > +
> 
> It seems like there are 3 test cases. Yet, we only dump the last one. r- because of this.

[WebKit Commit Bot]

Comment on attachment 204666 [details]
Patch

Clearing flags on attachment: 204666

Committed r151604: <http://trac.webkit.org/changeset/151604>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.