Bug 84597 – InsertUnorderedList and InsertOrderedList (execCommand) do not remove bullets

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Created attachment 138341 [details]
Simple HTML file with basic test case.

In some cases, using "InsertUnorderedList" or "InsertOrderedList" command on editable + designMode content allows to insert bullets to elements, but not to remove them.

Steps to reproduce
¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯
1) Click in a DIV having properties: contentEditable=='true' and designMode == 'on'
2) Paste the following text in the DIV:

This is my list of items:
Item 1
Item 2
Item 3
This is the end of my list.

3) Select the lines "Item 1" to "Item 3"
4) Execute the following command in the debugging console: "{myEditableDiv}.execCommand("InsertUnorderedList", false, null)"
- (List is now bulleted)
5) Execute the same command in the debugging console, again: "{myEditableDiv}.execCommand("InsertUnorderedList", false, null)"

Actual result
¯¯¯¯¯¯¯¯¯¯¯¯¯
- List is still bulleted

Expected result
¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯
- List is unbulleted

Additional notes
¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯
- Works fine with single line selected
- If multiple lines are selected, if words fine if and only if the last line is part of the selection

- Reproduced under Google Chrome 18.0.1025.162 release, Google Chrome 20.0.1113.0 canary and Safari 5.1.2

Attachment
¯¯¯¯¯¯¯¯¯¯
Simple HTML file with basic test case. Use the following command: 

document.getElementById("editableDiv").execCommand("InsertUnorderedList", false, null)

[Arpita Bahuguna]

Created attachment 145784 [details]
Patch

[WebKit Review Bot]

Attachment 145784 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Arpita Bahuguna]

Created attachment 145788 [details]
Patch

[Arpita Bahuguna]

The InsertListCommand::selectionHasListOfType() comparison of the start and the end VisiblePositions is erroneous to the extent that the selection's end visiblePosition consists of the offset (if any) as well.
On the other hand the visiblePosition being compared against (start) always points to the start of the paragraph i.e. with offset 0.
Thus the != comparison fails and hence returns false in case of multiple lines selected for reverting the ordered/unordered list.

This is a proposed patch only to verify whether the direction am considering is proper or not.

Or, we could probably have another function wherein the comparison is done between start and end Positions as a.deprecatedEditingOffset() <= b.deprecatedEditingOffset() as opposed to just an equality comparison.

[WebKit Review Bot]

Comment on attachment 145788 [details]
Patch

Attachment 145788 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12896783

New failing tests:
http/tests/media/media-source/video-media-source-event-attributes.html

[WebKit Review Bot]

Created attachment 145934 [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]

Comment on attachment 145788 [details]
Patch

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

r- because we need a test for this.

> Source/WebCore/ChangeLog:13
> +        Comparison between start and end VisiblePositions is not proper since the end
> +        VisiblePosition also contains the offset, whereas comparison is made with a
> +        VisiblePosition which is at the start of the paragraph being compared i.e. with
> +        offset always as zero.

This isn't really accurate description of what's happening. The problem is that we're sometimes comparing a position inside a paragraph with a position at the start of the paragraph.

[Arpita Bahuguna]

Created attachment 146011 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 146011 [details]
Patch

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

Please fix nits below:

> Source/WebCore/ChangeLog:17
> +        Comparison between start and end VisiblePositions will fail when a position inside
> +        a paragraph is compared with one at the start of the paragraph.
> +
> +        This fix thus converts the end VisiblePosition (which could be inside the paragraph)
> +        to the start of the paragraph thereby enabling a proper comparison of start and end
> +        positions.

We normally put these long description after "Reviewed by" before the list of tests wrapped in blank lines.

> LayoutTests/ChangeLog:12
> +        This test verifies removing of an (un)ordered list on a selection consisting of multiple
> +        list items (paragraphs) when the end point of the selection points to the end of the last
> +        list item; followed by another paragraph not part of the selection/list.

This should probably go after "Reviewed by" line but before the list of files.

> LayoutTests/editing/execCommand/remove-list-from-multi-list-items.html:3
> +<p>This test verifies removing of an (Un)OrderedList on a selection consisting of multiple list items (paragaraphs) when the end point of the selection points to the end of the last list item; followed by another paragraph not part of the selection/list.</p>

You need to pass this to Markup.description.

[Arpita Bahuguna]

Created attachment 146200 [details]
Patch

[WebKit Review Bot]

Comment on attachment 146200 [details]
Patch

Clearing flags on attachment: 146200

Committed r119701: <http://trac.webkit.org/changeset/119701>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.