Bug 84597 - InsertUnorderedList and InsertOrderedList (execCommand) do not remove bullets
Summary: InsertUnorderedList and InsertOrderedList (execCommand) do not remove bullets
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-23 07:37 PDT by bnusfmhpq
Modified: 2012-06-07 03:36 PDT (History)
6 users (show)

See Also:


Attachments
Simple HTML file with basic test case. (537 bytes, text/html)
2012-04-23 07:37 PDT, bnusfmhpq
no flags Details
Patch (1.81 KB, patch)
2012-06-05 07:13 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (1.66 KB, patch)
2012-06-05 07:28 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (458.79 KB, application/zip)
2012-06-05 22:30 PDT, WebKit Review Bot
no flags Details
Patch (4.71 KB, patch)
2012-06-06 06:41 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (5.10 KB, patch)
2012-06-06 22:50 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description bnusfmhpq 2012-04-23 07:37:33 PDT
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)
Comment 1 Arpita Bahuguna 2012-06-05 07:13:53 PDT
Created attachment 145784 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-05 07:15:43 PDT
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.
Comment 3 Arpita Bahuguna 2012-06-05 07:28:50 PDT
Created attachment 145788 [details]
Patch
Comment 4 Arpita Bahuguna 2012-06-05 07:33:17 PDT
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.
Comment 5 WebKit Review Bot 2012-06-05 22:30:02 PDT
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
Comment 6 WebKit Review Bot 2012-06-05 22:30:06 PDT
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
Comment 7 Ryosuke Niwa 2012-06-05 22:30:38 PDT
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.
Comment 8 Arpita Bahuguna 2012-06-06 06:41:10 PDT
Created attachment 146011 [details]
Patch
Comment 9 Ryosuke Niwa 2012-06-06 12:12:59 PDT
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.
Comment 10 Arpita Bahuguna 2012-06-06 22:50:52 PDT
Created attachment 146200 [details]
Patch
Comment 11 WebKit Review Bot 2012-06-07 03:36:53 PDT
Comment on attachment 146200 [details]
Patch

Clearing flags on attachment: 146200

Committed r119701: <http://trac.webkit.org/changeset/119701>
Comment 12 WebKit Review Bot 2012-06-07 03:36:58 PDT
All reviewed patches have been landed.  Closing bug.