REGRESSION: Cursor jumps to the first line after deleting the last word.

[Enrica Casucci]

To reproduce the problem, use the following markup in an editable container:

<div>one<br>two</div>

Select the word 'two' and press delete.

The expected markup should be:

<one><br><br>

The actual markup is

<br>one<br> with the caret on the first line.

<rdar://problem/11210059>

[Enrica Casucci]

Created attachment 139723 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Andy Estes]

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.

[Enrica Casucci]

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.

[Enrica Casucci]

Created attachment 139871 [details]
Patch2

[Enrica Casucci]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Ryosuke Niwa]

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.

[Enrica Casucci]

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.

[Enrica Casucci]

Created attachment 139922 [details]
Patch3

[Enrica Casucci]

The new patch provides results for the failing platforms.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Enrica Casucci]

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.

[Enrica Casucci]

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.

[Ryosuke Niwa]

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.

[Enrica Casucci]

(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.

[Enrica Casucci]

Created attachment 140356 [details]
Patch5

[Ryosuke Niwa]

please fix the test before you land it.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Ryosuke Niwa]

Closing the bug since the patch appears to have been landed in http://trac.webkit.org/changeset/116192.