Bug 42436 - listifyParagraph should use moveParagraphWithClones instead of moveParagraph
Summary: listifyParagraph should use moveParagraphWithClones instead of moveParagraph
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 42913 33668 42608
Blocks: 49315 27522 32422
  Show dependency treegraph
 
Reported: 2010-07-15 18:15 PDT by Ryosuke Niwa
Modified: 2017-07-18 08:27 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 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.
Comment 1 Ryosuke Niwa 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.
Comment 2 Ryosuke Niwa 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.
Comment 3 Ryosuke Niwa 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.
Comment 4 Ryosuke Niwa 2010-07-19 15:51:22 PDT
Created attachment 62007 [details]
work in progress 3

Sorry, I uploaded a wrong patch.
Comment 5 Ryosuke Niwa 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.
Comment 6 Ryosuke Niwa 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.