Bug 115023

Summary: Editing: wrong text position when you click enter on the text behind the image
Product: WebKit Reporter: Krzysztof Wolanski <k.wolanski>
Component: WebCore Misc.Assignee: Arpita Bahuguna <arpitabahuguna>
Status: RESOLVED FIXED    
Severity: Normal CC: arpitabahuguna, buildbot, commit-queue, donggwan.kim, dw.im, eflews.bot, gtk-ews, gyuyoung.kim, mpakulavelrutka, rego+ews, rniwa, webkit-ews, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
HTML page, shows unexpected behavior
none
Patch
none
WIP Patch
none
Patch-using-editingIgnoresContent
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Patch
none
Patch
none
Patch none

Description Krzysztof Wolanski 2013-04-23 01:54:34 PDT
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
Comment 1 Krzysztof Wolanski 2013-04-23 01:55:12 PDT
Created attachment 199172 [details]
HTML page, shows unexpected behavior
Comment 2 Arpita Bahuguna 2013-05-07 07:12:11 PDT
Created attachment 200900 [details]
Patch
Comment 3 Ryosuke Niwa 2013-05-07 10:08:18 PDT
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.
Comment 4 Arpita Bahuguna 2013-05-13 03:37:09 PDT
(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.
Comment 5 Ryosuke Niwa 2013-05-13 09:03:09 PDT
(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.
Comment 6 Krzysztof Wolanski 2013-05-14 04:50:59 PDT
(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.
Comment 7 Arpita Bahuguna 2013-05-14 06:11:09 PDT
(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.
Comment 8 Arpita Bahuguna 2013-05-14 07:40:00 PDT
(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.
Comment 9 Krzysztof Wolanski 2013-05-15 03:22:45 PDT
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."
Comment 10 Arpita Bahuguna 2013-05-15 04:06:57 PDT
(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.
Comment 11 Ryosuke Niwa 2013-05-15 09:53:56 PDT
(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.
Comment 12 Ryosuke Niwa 2013-05-15 09:55:35 PDT
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.
Comment 13 Arpita Bahuguna 2013-05-16 06:29:28 PDT
Created attachment 201949 [details]
WIP Patch
Comment 14 Arpita Bahuguna 2013-05-16 07:00:12 PDT
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. :)
Comment 15 Arpita Bahuguna 2013-05-16 07:13:00 PDT
Created attachment 201952 [details]
Patch-using-editingIgnoresContent
Comment 16 Arpita Bahuguna 2013-05-16 07:18:34 PDT
Comment on attachment 201952 [details]
Patch-using-editingIgnoresContent

This patch uses editingIgnoresContent() along with a check for BR element.
Comment 17 Arpita Bahuguna 2013-05-16 07:25:43 PDT
(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.
Comment 18 Build Bot 2013-05-16 16:01:19 PDT
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
Comment 19 Build Bot 2013-05-16 16:01:21 PDT
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
Comment 20 Arpita Bahuguna 2013-05-17 07:37:20 PDT
Created attachment 202084 [details]
Patch
Comment 21 Early Warning System Bot 2013-05-17 07:53:04 PDT
Comment on attachment 202084 [details]
Patch

Attachment 202084 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/492349
Comment 22 Build Bot 2013-05-17 07:54:17 PDT
Comment on attachment 202084 [details]
Patch

Attachment 202084 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/495294
Comment 23 Early Warning System Bot 2013-05-17 07:55:50 PDT
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
Comment 24 EFL EWS Bot 2013-05-17 07:59:38 PDT
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
Comment 25 Build Bot 2013-05-17 08:02:30 PDT
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
Comment 26 EFL EWS Bot 2013-05-17 08:03:50 PDT
Comment on attachment 202084 [details]
Patch

Attachment 202084 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/480859
Comment 27 Build Bot 2013-05-17 08:14:53 PDT
Comment on attachment 202084 [details]
Patch

Attachment 202084 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/482811
Comment 28 kov's GTK+ EWS bot 2013-05-17 10:07:54 PDT
Comment on attachment 202084 [details]
Patch

Attachment 202084 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/481937
Comment 29 Arpita Bahuguna 2013-05-18 01:44:32 PDT
Created attachment 202196 [details]
Patch
Comment 30 Ryosuke Niwa 2013-05-21 01:13:31 PDT
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.
Comment 31 Arpita Bahuguna 2013-06-13 20:51:30 PDT
Created attachment 204666 [details]
Patch
Comment 32 Arpita Bahuguna 2013-06-14 06:22:18 PDT
(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.
Comment 33 WebKit Commit Bot 2013-06-14 13:14:10 PDT
Comment on attachment 204666 [details]
Patch

Clearing flags on attachment: 204666

Committed r151604: <http://trac.webkit.org/changeset/151604>
Comment 34 WebKit Commit Bot 2013-06-14 13:14:14 PDT
All reviewed patches have been landed.  Closing bug.