Bug 85334

Summary: REGRESSION: Cursor jumps to the first line after deleting the last word.
Product: WebKit Reporter: Enrica Casucci <enrica>
Component: HTML EditingAssignee: 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 Flags
Patch
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02
none
Patch2
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03
none
Patch3
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-04
none
Patch4
none
Patch5
rniwa: review+, webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-04 none

Description Enrica Casucci 2012-05-01 18:28:18 PDT
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>
Comment 1 Enrica Casucci 2012-05-01 18:38:50 PDT
Created attachment 139723 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-01 19:40:17 PDT
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
Comment 3 WebKit Review Bot 2012-05-01 19:40:22 PDT
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 4 Andy Estes 2012-05-02 03:02:36 PDT
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.
Comment 5 Enrica Casucci 2012-05-02 11:19:50 PDT
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.
Comment 6 Enrica Casucci 2012-05-02 13:33:37 PDT
Created attachment 139871 [details]
Patch2
Comment 7 Enrica Casucci 2012-05-02 13:35:45 PDT
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 8 WebKit Review Bot 2012-05-02 14:50:43 PDT
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
Comment 9 WebKit Review Bot 2012-05-02 14:50:49 PDT
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 10 Ryosuke Niwa 2012-05-02 15:06:59 PDT
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 11 Enrica Casucci 2012-05-02 16:04:26 PDT
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.
Comment 12 Enrica Casucci 2012-05-02 17:13:43 PDT
Created attachment 139922 [details]
Patch3
Comment 13 Enrica Casucci 2012-05-02 17:14:19 PDT
The new patch provides results for the failing platforms.
Comment 14 WebKit Review Bot 2012-05-02 17:22:03 PDT
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 15 WebKit Review Bot 2012-05-02 18:40:44 PDT
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
Comment 16 WebKit Review Bot 2012-05-02 18:40:50 PDT
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
Comment 17 Enrica Casucci 2012-05-04 12:29:01 PDT
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 18 Enrica Casucci 2012-05-04 12:48:45 PDT
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 19 Ryosuke Niwa 2012-05-04 15:34:27 PDT
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.
Comment 20 Enrica Casucci 2012-05-04 15:37:41 PDT
(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.
Comment 21 Enrica Casucci 2012-05-04 16:20:31 PDT
Created attachment 140356 [details]
Patch5
Comment 22 Ryosuke Niwa 2012-05-04 16:50:29 PDT
please fix the test before you land it.
Comment 23 WebKit Review Bot 2012-05-04 16:53:45 PDT
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
Comment 24 WebKit Review Bot 2012-05-04 16:53:51 PDT
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
Comment 25 Ryosuke Niwa 2012-05-10 01:18:45 PDT
Closing the bug since the patch appears to have been landed in http://trac.webkit.org/changeset/116192.