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
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.
(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?
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?
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.
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
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
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
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
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
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
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.
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.
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.
(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.
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.
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.
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
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
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
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
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
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'.
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
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
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
2013-04-23 01:19 PDT, Krzysztof Wolanski
2013-04-24 02:30 PDT, Krzysztof Wolanski
2013-04-26 06:19 PDT, Lukasz Gajowy
2013-05-07 07:29 PDT, Wojciech Bielawski
2013-05-09 05:44 PDT, Lukasz Gajowy
buildbot: commit-queue-
2013-05-09 06:04 PDT, Wojciech Bielawski
2013-05-09 07:03 PDT, Wojciech Bielawski
buildbot: commit-queue-
2013-05-09 08:11 PDT, Build Bot
2013-05-09 08:12 PDT, Build Bot
2013-05-09 08:40 PDT, Build Bot
2013-05-09 12:21 PDT, Build Bot
2013-05-09 12:55 PDT, Build Bot
2013-05-13 16:06 PDT, Build Bot
2014-03-05 05:12 PST, Sudarshan C P
2014-03-12 02:06 PDT, Sudarshan C P
2014-03-13 23:29 PDT, Sudarshan C P
2014-03-14 02:25 PDT, Build Bot
2014-03-14 02:51 PDT, Sudarshan C P
2014-03-14 03:52 PDT, Build Bot
2014-03-14 04:15 PDT, Build Bot
2014-03-14 04:41 PDT, Build Bot
2014-03-14 04:55 PDT, Build Bot
2014-03-16 21:06 PDT, Sudarshan C P
buildbot: commit-queue-
2014-03-16 21:35 PDT, Build Bot
2014-03-16 22:26 PDT, Build Bot
2014-03-16 23:39 PDT, Build Bot