Bug 114960 – Wrong text position when you click backspace on the line below the image

• Status: UNCONFIRMED
• Component: HTML Editing

[Krzysztof Wolanski]

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

Actual Results: 
  The "text" goes in front of the IMG:
    <DIV><SPAN style=\94color:#FF0000">text</SPAN>
    <SPAN style=\94color:#FF0000"><IMG src=\94xxx\94/></SPAN>
    </DIV>

Expected Results:
  The "text" goes behind the image:
    <DIV><SPAN style=\94color:#FF0000"><IMG src=\94xxx\94/></SPAN>
    <SPAN style=\94color:#FF0000">text</ SPAN >
    </DIV>

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

[Krzysztof Wolanski]

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

[Alexey Proskuryakov]

I cannot reproduce this with r148978.

[Krzysztof Wolanski]

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

[Krzysztof Wolanski]

In description there is a mistake, should be:
Steps to Reproduce:
  1)Open the website that contains:
    <DIV><SPAN style="color:#FF0000"><IMG src="black.jpg"></SPAN></DIV>
    <DIV><SPAN style="color:#FF0000">text</SPAN>
    </DIV>
  2) Press backspace in front of "text".

Sorry for misleading.

[Lukasz Gajowy]

Changing m_upstreamStart to m_upstreamEnd in:

https://trac.webkit.org/browser/trunk/Source/WebCore/editing/DeleteSelectionCommand.cpp#L607

fixes this issue. What do you think of this solution?

[Ryosuke Niwa]

(In reply to comment #5)
> Changing m_upstreamStart to m_upstreamEnd in:
> 
> https://trac.webkit.org/browser/trunk/Source/WebCore/editing/DeleteSelectionCommand.cpp#L607
> 
> fixes this issue. What do you think of this solution?

That doesn't sound like a good solution without a more detailed description as to why that's the right fix.

[Lukasz Gajowy]

(In reply to comment #6)
> (In reply to comment #5)
> > Changing m_upstreamStart to m_upstreamEnd in:
> > 
> > https://trac.webkit.org/browser/trunk/Source/WebCore/editing/DeleteSelectionCommand.cpp#L607
> > 
> > fixes this issue. What do you think of this solution?
> 
> That doesn't sound like a good solution without a more detailed description as to why that's the right fix.

As i was debugging the code I assumed that m_upstreamEnd is the Position of the end of an image (according to the test page included). I came up to a conclusion, that the merge destination should be set to the m_upstreamEnd, because when we hit backspace, we want to have the text at the end of the image, not at the start of it. I am not sure that I understand 
this code well. Please correct me if I am wrong.

Another thesis is that the problem stems from wrong behaviour of mergeParagraphs() method, when in m_upstreamStart field (it is a Position class object):
 - m_offset = 0
 - m_anchorNode = 2 (AnchorType = PositionIsAfterAnchor)

What do you think of that?

[Lukasz Gajowy]

Created attachment 199823 [details]
Bug quick fix patch to be discussed.

Please refer to this solution and comment it.
Thank you in advance.

[Wojciech Bielawski]

I see suspicious behaviour in ReplaceSelectionCommand::doApply() 
When insertion position points after last child of a container node then new insertion position is computed (lines 1046-1054) incorrectly. 
I prepared a patch, but it breaks 2 other layout tests. Any suggestions how the computation should looks like?

[Wojciech Bielawski]

Created attachment 200902 [details]
Invalid insertion position computation - to discuss

[Lukasz Gajowy]

Created attachment 201183 [details]
Layout test testing the issue.

[Wojciech Bielawski]

Created attachment 201184 [details]
Fix placement position when inserting as the last child of a node

[WebKit Commit Bot]

Attachment 201184 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/editing/ReplaceSelectionCommand.cpp']" exit_code: 1
Source/WebCore/editing/ReplaceSelectionCommand.cpp:1052:  Declaration has space between type name and * in Node *insertAfterNode  [whitespace/declaration] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wojciech Bielawski]

Created attachment 201189 [details]
Fix placement position when inserting as the last child of a node

Style correction

[Build Bot]

Comment on attachment 201189 [details]
Fix placement position when inserting as the last child of a node

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

New failing tests:
editing/pasteboard/display-block-on-spans.html

[Build Bot]

Created attachment 201235 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

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

[Build Bot]

Comment on attachment 201183 [details]
Layout test testing the issue.

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

New failing tests:
editing/deleting/merge-image-and-text.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 201189 [details]
Fix placement position when inserting as the last child of a node

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

New failing tests:
editing/pasteboard/display-block-on-spans.html

[Build Bot]

Created attachment 201242 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

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

[Build Bot]

Comment on attachment 201183 [details]
Layout test testing the issue.

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

New failing tests:
editing/deleting/merge-image-and-text.html

[Build Bot]

Created attachment 201263 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

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

[Build Bot]

Comment on attachment 201183 [details]
Layout test testing the issue.

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

New failing tests:
editing/deleting/merge-image-and-text.html

[Build Bot]

Created attachment 201266 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

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

[Wojciech Bielawski]

I have no option to test it with MAC platform. Could someone take a look does the patch is acceptable?

[Build Bot]

Comment on attachment 201189 [details]
Fix placement position when inserting as the last child of a node

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

New failing tests:
editing/pasteboard/display-block-on-spans.html

[Build Bot]

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

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

[Ryosuke Niwa]

Comment on attachment 201189 [details]
Fix placement position when inserting as the last child of a node

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

> Source/WebCore/ChangeLog:8
> +        Layout test added in separate patch: https://bug-114960-attachments.webkit.org/attachment.cgi?id=201183

Please combine that patch into this.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1054
> +                    if (!insertAfterNode)
> +                        insertAfterNode = nodeToSplitTo.get();

This doesn't look right. We shouldn't be using the position after the nodeToSplit ever.
We probably need to get the position before the nodeToSplitTo in this case.

[Ryosuke Niwa]

Comment on attachment 201183 [details]
Layout test testing the issue.

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

We don't add a test by itself. This patch needs to be merged with the actual fix.

> LayoutTests/editing/deleting/merge-image-and-text.html:13
> +Markup.description('Testcase for bug https://webkit.org/b/114960: Wrong text position when you click backspace on the line below the image.\n'+
> +'The test passes if "text1" appears on the right side of the image.');

Since everyone can look at trac or svn blame to figure out for which bug this test was added, it's not that interesting to repeat the bug URL and title.
Instead you should describe what this test is doing; the specific condition under which deletion occurs.

> LayoutTests/editing/deleting/merge-image-and-text.html:16
> +var testedHTML = document.getElementById('editableContent');

We should name this variable like container or editableContent to be more descriptive.

[KyungTae Kim]

Is there any progress on this?

[Sudarshan C P]

Created attachment 225874 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 225874 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        While finding new position for inserting element after deleting/pasting,
> +        included image element also for find the proper position to merge the split node.

This description should be appear before "Test:" line but after "Reviewed by" line.
But this doesn't really describe what was causing the bug and how we're fixing it.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:119
>      // If we're already on a break, it's probably a placeholder and we shouldn't change our position.
> -    if (editingIgnoresContent(pos.deprecatedNode()))
> +    if (editingIgnoresContent(pos.deprecatedNode()) && !pos.deprecatedNode()->hasTagName(imgTag))

The problem here is that editingIgnoresContent returns true on more than just a placeholder.
We should be checking whether the deprecatedNode is br or not instead.
What's weird is that lineBreakExistsAtPosition below should already be checking this condition.

r- because special-casing img can't be the right fix. e.g. it'll fail for other elements such as object and hr that are ignored by editing.

[Sudarshan C P]

(In reply to comment #32)
> (From update of attachment 225874 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225874&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        While finding new position for inserting element after deleting/pasting,
> > +        included image element also for find the proper position to merge the split node.
> 
> This description should be appear before "Test:" line but after "Reviewed by" line.
> But this doesn't really describe what was causing the bug and how we're fixing it.
> 
i will modify the above comments.

> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:119
> >      // If we're already on a break, it's probably a placeholder and we shouldn't change our position.
> > -    if (editingIgnoresContent(pos.deprecatedNode()))
> > +    if (editingIgnoresContent(pos.deprecatedNode()) && !pos.deprecatedNode()->hasTagName(imgTag))
> 
> The problem here is that editingIgnoresContent returns true on more than just a placeholder.
> We should be checking whether the deprecatedNode is br or not instead.
> What's weird is that lineBreakExistsAtPosition below should already be checking this condition.
> 
> r- because special-casing img can't be the right fix. e.g. it'll fail for other elements such as object and hr that are ignored by editing.

root cause for this issue is nodeToSplitToAvoidPastingIntolineNodeWithStyle() is trying to find the highestEnclosingNodeType(), so elementIsStyleOrHTMLEquivalent() matchedAttributes<=element->attributeount() condition satisfies and wrong position gives in case of image element to split and merge with new node, hence added the image condition to over come this error.

Thanks for your comments i will try to find the new set of patch as per comments and update.

[Sudarshan C P]

Created attachment 226484 [details]
Patch

[Darin Adler]

Comment on attachment 226484 [details]
Patch

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

This patch is OK, but I’d like to see a better test. We shouldn’t land with a test that we know will fail on some platforms.

> LayoutTests/editing/deleting/merge-image-and-text-expected.txt:11
> +            RenderText {#text} at (4,0) size 31x17

This is non-portable, so this test is going to fail on some platforms. The text might not be 31 pixels wide on every platform due to font differences.

> LayoutTests/editing/deleting/merge-image-and-text.html:1
> +<!DOCTYPE html>

It’s much better for editing tests to dump markup rather than a render tree. Look at a recently added editing tests such as editing/execCommand/indent-img-twice.html to see how to do this. It makes a much useful test for the WebKit project. And deals with the portability issue I mentioned above.

[Darin Adler]

Comment on attachment 226484 [details]
Patch

I’d like to see test cases covering more. Ryosuke pointed out that <object> and <br> are other cases that we might have broken if we did this wrong. We should use his insight to make test cases. We want test cases that would have shown the problems with earlier incorrect versions of this patch.

[Sudarshan C P]

Created attachment 226654 [details]
Patch

[Build Bot]

Comment on attachment 226654 [details]
Patch

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

New failing tests:
editing/deleting/merge-image-and-text.html

[Build Bot]

Created attachment 226669 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

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

[Sudarshan C P]

Created attachment 226672 [details]
Patch

[Build Bot]

Comment on attachment 226672 [details]
Patch

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

New failing tests:
editing/pasteboard/5065605.html
editing/pasteboard/paste-and-sanitize.html
editing/pasteboard/paste-after-inline-style-element.html
editing/pasteboard/merge-end-1.html
editing/pasteboard/paste-with-redundant-style.html
editing/pasteboard/paste-text-with-style-3.html
editing/pasteboard/paste-text-011.html
editing/pasteboard/merge-end-2.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 226672 [details]
Patch

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

New failing tests:
editing/pasteboard/5065605.html
editing/pasteboard/paste-and-sanitize.html
editing/pasteboard/paste-after-inline-style-element.html
editing/pasteboard/merge-end-1.html
editing/pasteboard/paste-with-redundant-style.html
editing/pasteboard/paste-text-with-style-3.html
editing/pasteboard/paste-text-011.html
editing/pasteboard/merge-end-2.html

[Build Bot]

Created attachment 226679 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

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

[Build Bot]

Comment on attachment 226672 [details]
Patch

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

New failing tests:
editing/pasteboard/5065605.html
editing/pasteboard/paste-and-sanitize.html
editing/pasteboard/paste-after-inline-style-element.html
editing/pasteboard/merge-end-1.html
editing/pasteboard/paste-with-redundant-style.html
editing/pasteboard/paste-text-with-style-3.html
editing/pasteboard/paste-text-011.html
editing/pasteboard/merge-end-2.html

[Build Bot]

Created attachment 226684 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

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

[Build Bot]

Comment on attachment 226672 [details]
Patch

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

New failing tests:
editing/pasteboard/5065605.html
editing/pasteboard/paste-and-sanitize.html
editing/pasteboard/paste-after-inline-style-element.html
editing/pasteboard/merge-end-1.html
editing/pasteboard/paste-with-redundant-style.html
editing/pasteboard/paste-text-with-style-3.html
editing/pasteboard/paste-text-011.html
editing/pasteboard/merge-end-2.html

[Build Bot]

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

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

[Ryosuke Niwa]

Comment on attachment 226672 [details]
Patch

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

> LayoutTests/editing/deleting/merge-image-and-text.html:6
> +<div><span><img src="some_img" style="color:#FF0000"></span></div>

Can't we use ../abe.png instead?  Why do we need to have style attribute?  Is that required?

> LayoutTests/editing/deleting/merge-image-and-text.html:7
> +<div><span id="textSpan" style="color:#FF0000">text</span></div>

Ditto about style attribute.

> LayoutTests/editing/deleting/merge-image-and-text.html:11
> +Markup.description('Wrong text position when you click backspace on the line below the image.');

This description isn't descriptive enough.  We should explain what kind of behavior this test is testing.

> LayoutTests/editing/deleting/merge-image-and-text.html:15
> +var selection = window.getSelection();

We don't need this local variable.

> LayoutTests/editing/deleting/merge-image-and-text.html:16
> +Markup.dump(editElement, 'Before Delete');

I don't see a value in dumping the content without selection since that's basically identical to the content with caret in it.

> LayoutTests/editing/deleting/merge-image-and-text.html:18
> +selection.collapse(text, text.childNodes.length -1);

Just call getSelecrtion() here.
Nit: text.childNodes.length - 1.

> LayoutTests/editing/deleting/merge-image-and-text.html:19
> +Markup.dump(text, 'Place the cursor at');

It's useless to dump the content of text because we don't know where "text" is located at in the original content.
We should dump the whole editElement here and label this one as 'Before deletion' instead.

> LayoutTests/editing/deleting/merge-image-and-text.html:22
> +Markup.dump(editElement, 'After Delete');

'After deletion'.

[Sudarshan C P]

Created attachment 226874 [details]
Patch

[Build Bot]

Comment on attachment 226874 [details]
Patch

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

New failing tests:
editing/pasteboard/5065605.html
editing/pasteboard/paste-and-sanitize.html
editing/pasteboard/paste-after-inline-style-element.html
editing/pasteboard/merge-end-1.html
editing/pasteboard/paste-with-redundant-style.html
editing/pasteboard/paste-text-with-style-3.html
editing/pasteboard/paste-text-011.html
editing/pasteboard/merge-end-2.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 226874 [details]
Patch

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

New failing tests:
editing/pasteboard/5065605.html
editing/pasteboard/paste-and-sanitize.html
editing/pasteboard/paste-after-inline-style-element.html
editing/pasteboard/merge-end-1.html
editing/pasteboard/paste-with-redundant-style.html
editing/pasteboard/paste-text-with-style-3.html
editing/pasteboard/paste-text-011.html
editing/pasteboard/merge-end-2.html

[Build Bot]

Created attachment 226881 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

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

[Build Bot]

Comment on attachment 226874 [details]
Patch

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

New failing tests:
editing/pasteboard/5065605.html
editing/pasteboard/paste-and-sanitize.html
editing/pasteboard/paste-after-inline-style-element.html
editing/pasteboard/merge-end-1.html
editing/pasteboard/paste-with-redundant-style.html
editing/pasteboard/paste-text-with-style-3.html
editing/pasteboard/paste-text-011.html
editing/pasteboard/merge-end-2.html

[Build Bot]

Created attachment 226888 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

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

[Darin Adler]

Comment on attachment 226874 [details]
Patch

The EWS bot shows 8 regression tests failing.

  editing/pasteboard/5065605.html [ Failure ]
  editing/pasteboard/merge-end-1.html [ Failure ]
  editing/pasteboard/merge-end-2.html [ Failure ]
  editing/pasteboard/paste-after-inline-style-element.html [ Failure ]
  editing/pasteboard/paste-and-sanitize.html [ Failure ]
  editing/pasteboard/paste-text-011.html [ Failure ]
  editing/pasteboard/paste-text-with-style-3.html [ Failure ]
  editing/pasteboard/paste-with-redundant-style.html [ Failure ]

The patch needs to include new results for those tests if the “failures” are actually progressions, or somehow resolve the failures.

[Darin Adler]

The new test in the patch itself looks really good!