Bug 130393 – Applying ordered list followed by unordered to a selected all content selection disappears.

• Component: HTML Editing
• Status: NEW

[Sudarshan C P]

Created attachment 227041 [details]
Test Case

1.Fetch attached test case.
2.type some text
3.select all (ctrl+A)
4.click ordered list and then unordered list

Actual behavior :
Selection disappears.

Expected behavior :
Selection should retains.

[Sudarshan C P]

Created attachment 227155 [details]
Patch

[Build Bot]

Comment on attachment 227155 [details]
Patch

Attachment 227155 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5332482580807680

New failing tests:
editing/execCommand/insert-list-nested-with-orphaned.html

[Build Bot]

Created attachment 227158 [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

[Build Bot]

Comment on attachment 227155 [details]
Patch

Attachment 227155 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5361014048555008

New failing tests:
editing/execCommand/insert-list-nested-with-orphaned.html

[Build Bot]

Created attachment 227172 [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

[Sudarshan C P]

In the current fix we are placing the cursor at the end of applied list item,so failed case is because of cursor poistion, but actual issue is that "This tests hang when listifying nested lists with an orphaned list child in between", actual test case works fine Please review this patch.

[Darin Adler]

Comment on attachment 227155 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227155&action=review

> Source/WebCore/editing/InsertListCommand.cpp:246
> -            setEndingSelection(VisiblePosition(firstPositionInNode(newList.get())));
> +            setEndingSelection(VisibleSelection(firstPositionInNode(newList.get()), lastPositionInNode(newList.get()), endingSelection().isDirectional()));

Why select the whole list when the list type is changed? That seems almost as bad as the old rule (put the selection at the start of the first list element). What is the desired selection change in a case like this? Before we implement it we need to clearly state what we want? Do we want to preserve the existing selection? Or something else?

I think that when making a new list we want to put the insertion point in the list, ready to type into that list.

> LayoutTests/editing/execCommand/selectall-ol-ul-expected.txt:12
> +After Applying Orderedlist:
> +| <ol>
> +|   <li>
> +|     "<#selection-anchor>orderlist text to display<#selection-focus>"
> +|     <br>
> +
> +After Applying UnOrderedList:
> +| <ul>
> +|   <li>
> +|     "<#selection-anchor>orderlist text to display<#selection-focus>"

This test makes it look like the command preserves the selection. But what it actually does is select the entire contents of the list. I don’t think that’s right.

The test needs to show what happens when the contents of the list isn’t all selected. For example, what if just the word "text" was selected? What if the insertion point was after the word "display"?