Summary: | REGRESSION: Cursor jumps to the first line after deleting the last word. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Enrica Casucci <enrica> | ||||||||||||||||||||
Component: | HTML Editing | Assignee: | Enrica Casucci <enrica> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | dglazkov, rniwa, webkit.review.bot | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Enrica Casucci
2012-05-01 18:28:18 PDT
Created attachment 139723 [details]
Patch
Comment on attachment 139723 [details] Patch Attachment 139723 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12590735 New failing tests: editing/pasteboard/4076267-3.html editing/deleting/delete-block-merge-contents-005.html editing/pasteboard/do-not-copy-unnecessary-styles.html editing/pasteboard/merge-end-3.html editing/deleting/5546763.html editing/pasteboard/paste-table-002.html editing/deleting/delete-block-merge-contents-006.html editing/pasteboard/paste-line-endings-009.html editing/execCommand/format-block-multiple-paragraphs.html editing/pasteboard/copy-paste-bidi.html editing/deleting/delete-block-merge-contents-008.html Created attachment 139729 [details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 139723 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139723&action=review I didn't review this, but I did have a few nit-picky comments. > Source/WebCore/ChangeLog:9 > + This regression was introduced with the work t remove redundant divs. typo: "work t remove" > Source/WebCore/ChangeLog:10 > + When we decide to remove a DIV, we need toadjust the selection, if it is typo: "toadjust" > Source/WebCore/ChangeLog:11 > + expressed in terms of the node being removed. The new positions was computed typo: "positions was" > Source/WebCore/editing/DeleteSelectionCommand.cpp:743 > + CompositeEditCommand::updatePositionForNodeRemovalPreservingChildren(m_endingPosition, node); You shouldn't need to qualify this with 'CompositeEditCommand::'. updatePositionForNodeRemovalPreservingChildren() is protected in the base class and isn't hidden by another function, so you should be able to call it normally. Andy, thanks for the comments. I'll fix the typos and the CompositeEditCommand reference. I discovered an issue with one part of the patch. I'll fix that and post a new one soon. Created attachment 139871 [details]
Patch2
Comment on attachment 139871 [details] Patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=139871&action=review > Source/WebCore/ChangeLog:20 > + (WebCore::CompositeEditCommand::isRemovableBlock): We should check that the parent This comment is obsolete. I'll fix it. Comment on attachment 139871 [details] Patch2 Attachment 139871 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12595926 New failing tests: editing/pasteboard/paste-line-endings-009.html Created attachment 139887 [details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 139871 [details] Patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=139871&action=review > Source/WebCore/editing/CompositeEditCommand.cpp:307 > + if (!node->hasTagName(divTag)) Maybe we should also remove pTag? Regardless, we should check the padding, margin, etc... to make sure we don't accidentally remove block nodes with style. > Source/WebCore/editing/CompositeEditCommand.cpp:313 > + while (firstChild && firstChild->isTextNode() && !toText(firstChild)->renderer()) > + firstChild = firstChild->nextSibling(); What if we had <div><div><br></div></div>? > Source/WebCore/editing/CompositeEditCommand.cpp:315 > + Node* sibling = (firstChild) ? firstChild->nextSibling() : 0; No parentheses are needed around firstNode. > Source/WebCore/editing/CompositeEditCommand.cpp:319 > + if (sibling || toElement(node)->hasAttributes()) We should probably check that the node doesn't have padding, margin, etc... specified. > Source/WebCore/editing/CompositeEditCommand.cpp:416 > + int offset = (position.anchorType() == Position::PositionIsOffsetInAnchor) ? position.offsetInContainerNode() : 0; You should also take care of the cases where Position is before/after children. > LayoutTests/ChangeLog:19 > + * editing/execCommand/format-block-multiple-paragraphs-expected.txt: > + * editing/pasteboard/copy-paste-bidi-expected.txt: > + * editing/pasteboard/do-not-copy-unnecessary-styles-expected.txt: > + * editing/pasteboard/paste-table-002-expected.txt: > + * platform/mac/editing/pasteboard/paste-line-endings-009-expected.txt: Could you convert some of these tests to dump-as-markup tests? Outputs are hard to understand. Comment on attachment 139871 [details] Patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=139871&action=review I will post a new patch with the new results for the tests that is failing on Chromium. >> Source/WebCore/editing/CompositeEditCommand.cpp:307 >> + if (!node->hasTagName(divTag)) > > Maybe we should also remove pTag? Regardless, we should check the padding, margin, etc... to make sure we don't accidentally remove block nodes with style. In another patch. This fix here was only aimed at making sure we don't skip divs that have multiple children of which all but one are unrendered text nodes. >> Source/WebCore/editing/CompositeEditCommand.cpp:313 >> + firstChild = firstChild->nextSibling(); > > What if we had <div><div><br></div></div>? this loop skips only unrendered text nodes. In the case you're considering, both divs will be removed and we'll have only <br> >> Source/WebCore/editing/CompositeEditCommand.cpp:315 >> + Node* sibling = (firstChild) ? firstChild->nextSibling() : 0; > > No parentheses are needed around firstNode. Sure. >> Source/WebCore/editing/CompositeEditCommand.cpp:319 >> + if (sibling || toElement(node)->hasAttributes()) > > We should probably check that the node doesn't have padding, margin, etc... specified. It the node has attributes it doesn't get removed. The method returns false. > LayoutTests/ChangeLog:20 > + will do later. Created attachment 139922 [details]
Patch3
The new patch provides results for the failing platforms. Attachment 139922 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1
Traceback (most recent call last):
File "Tools/Scripts/check-webkit-style", line 48, in <module>
sys.exit(CheckWebKitStyle().main())
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 154, in main
patch_checker.check(patch)
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 71, in check
self._text_file_reader.process_file(file_path=path, line_numbers=None)
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 130, in process_file
self._processor.process(lines, file_path, **kwargs)
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 844, in process
min_confidence)
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 619, in dispatch
min_confidence)
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 593, in _create_checker
checker = PNGChecker(file_path, handle_style_error)
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/png.py", line 43, in __init__
self._fs = host.filesystem
AttributeError: 'NoneType' object has no attribute 'filesystem'
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 139922 [details] Patch3 Attachment 139922 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12545129 New failing tests: editing/pasteboard/paste-line-endings-009.html editing/pasteboard/paste-line-endings-010.html editing/deleting/delete-word-from-unstyled-div.html Created attachment 139937 [details]
Archive of layout-test-results from ec2-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 140297 [details]
Patch4
New patch that skips the further optimization. Will do that in a separate patch, since it is not required to fix this bug and avoids the need for updated results.
When I do that I can also update some of the tests to be dumpAsText.
Comment on attachment 140297 [details] Patch4 View in context: https://bugs.webkit.org/attachment.cgi?id=140297&action=review > Source/WebCore/ChangeLog:2568 > +>>>>>>> .r116139 I've removed it already. Comment on attachment 139922 [details] Patch3 View in context: https://bugs.webkit.org/attachment.cgi?id=139922&action=review > Source/WebCore/editing/CompositeEditCommand.cpp:416 > + int offset = (position.anchorType() == Position::PositionIsOffsetInAnchor) ? position.offsetInContainerNode() : 0; We need to take care of the case when the position is after children. > LayoutTests/editing/deleting/delete-word-from-unstyled-div.html:13 > +<script> You need to include dump-as-markup.js > LayoutTests/editing/deleting/delete-word-from-unstyled-div.html:18 > + //Markup.dump('test'); Please uncomment this and rebaseline the test. Also, it'll be good to add Markup.description() and explain what you're testing. (In reply to comment #19) > (From update of attachment 139922 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139922&action=review > > > Source/WebCore/editing/CompositeEditCommand.cpp:416 > > + int offset = (position.anchorType() == Position::PositionIsOffsetInAnchor) ? position.offsetInContainerNode() : 0; > > We need to take care of the case when the position is after children. > You're right, you've mentioned it before, but I had forgotten. > > LayoutTests/editing/deleting/delete-word-from-unstyled-div.html:13 > > +<script> > > You need to include dump-as-markup.js > > > LayoutTests/editing/deleting/delete-word-from-unstyled-div.html:18 > > + //Markup.dump('test'); > > Please uncomment this and rebaseline the test. > Also, it'll be good to add Markup.description() and explain what you're testing. Will do both. Thanks for the feedback. Created attachment 140356 [details]
Patch5
please fix the test before you land it. Comment on attachment 140356 [details] Patch5 Attachment 140356 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12626295 New failing tests: editing/deleting/delete-word-from-unstyled-div.html Created attachment 140365 [details]
Archive of layout-test-results from ec2-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Closing the bug since the patch appears to have been landed in http://trac.webkit.org/changeset/116192. |