Bug 42436

Summary:

listifyParagraph should use moveParagraphWithClones instead of moveParagraph

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

NEW

Severity:

Normal

CC:

enrica, justin.garcia, rolandsteiner, tkent, tony

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

42913, 33668, 42608

Bug Blocks:

27522, 32422, 49315

Attachments:

Description	Flags
work in progress, request for comments	none
work in progress 2	none
work in progress 3	none
work in progress 3	none

Ryosuke Niwa

Reported 2010-07-15 18:15:20 PDT

To fix the bug 27522 and the bug 32422, listifyParagraph should be modified to use moveParagraphWithClones instead of moveParagraph.

Attachments
work in progress, request for comments (7.16 KB, patch) 2010-07-16 14:01 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
work in progress 2 (17.73 KB, patch) 2010-07-18 17:54 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
work in progress 3 (13.35 KB, patch) 2010-07-19 15:23 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
work in progress 3 (18.57 KB, patch) 2010-07-19 15:51 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2010-07-16 14:01:13 PDT

Created attachment 61844 [details] work in progress, request for comments Work in progress. I made some good progress but moveParagraphWithClones grabs extra div's in many cases. It seems like I'm not giving the correct outerBlock.

Ryosuke Niwa

Comment 2 2010-07-18 17:54:07 PDT

Created attachment 61921 [details] work in progress 2 More progress. 5 rebaselines are added to the patch but there are 3 additional tests in /editing/execCommand/ that require rebaseline: create-list-from-range-selection.html, create-list-with-hr.html, and insert-list-empty-div.html. These tests should be dumpAsText or dumpAsMarkup tests instead and should be converted in a separate patch before this patch is to be landed. It seems like I should be making a separate cleanup patch at this point. I'll appreciate if any of you can comment on my approach / modification here.

Ryosuke Niwa

Comment 3 2010-07-19 15:23:15 PDT

Created attachment 62000 [details] work in progress 3 I added PersistentEditingPosition to isolate the code saving and restoring selection. It stores a VisiblePosition and returns the VisiblePosition when available. When the deepEquivalent node gets detached from the document and the original VisiblePosition becomes invalid, it restores the position using the range length.

Ryosuke Niwa

Comment 4 2010-07-19 15:51:22 PDT

Created attachment 62007 [details] work in progress 3 Sorry, I uploaded a wrong patch.

Ryosuke Niwa

Comment 5 2010-07-19 22:35:13 PDT

+Roland I'll appreciate if you can comment on PersistentEditingPosition and if there's any similarity to what you were proposing few weeks ago.

Ryosuke Niwa

Comment 6 2010-07-24 00:16:05 PDT

I just realized that my patch might cause a significant performance regression since TextIterator will iterate through all the nodes starting at the editable root to find the correct location. I need to figure out an alternative approach here.

Note You need to log in before you can comment on or make changes to this bug.