Created attachment 199169 [details]
HTML page, shows unexpected behavior
Created attachment 199407 [details]
HTML page, shows unexpected behavior
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.
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? (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. (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? Created attachment 199823 [details]
Bug quick fix patch to be discussed.
Please refer to this solution and comment it.
Thank you in advance.
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? Created attachment 200902 [details]
Invalid insertion position computation - to discuss
Created attachment 201183 [details]
Layout test testing the issue.
Created attachment 201184 [details]
Fix placement position when inserting as the last child of a node
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 201189 [details]
Fix placement position when inserting as the last child of a node
Style correction
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 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
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 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
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 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
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 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
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 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
I have no option to test it with MAC platform. Could someone take a look does the patch is acceptable? 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 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. Is there any progress on this? Created attachment 225874 [details]
Patch
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. Created attachment 226484 [details]
Patch
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 226654 [details]
Patch
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 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 226672 [details]
Patch
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 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
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 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
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 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
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 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 226874 [details]
Patch
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 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
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 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
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 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
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.
The new test in the patch itself looks really good! |
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