Fix issue with selection being lost when InsertListCommand changes the list type.

Source/core/editing/InsertListCommand.cpp

 /*
  * Copyright (C) 2006, 2010 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 111 matching lines @@
     // margin/padding, but not others.  We should make the gap painting more consistent and
     // then use a left margin/padding rule here.
     if (visibleEnd != visibleStart && isStartOfParagraph(visibleEnd, CanSkipOverEditingBoundary)) {
         setEndingSelection(VisibleSelection(visibleStart, visibleEnd.previous(CannotCrossEditingBoundary), endingSelection().isDirectional()));
         if (!endingSelection().rootEditableElement())
             return;
     }
 
     const QualifiedName& listTag = (m_type == OrderedList) ? olTag : ulTag;
     if (endingSelection().isRange()) {
+        bool forceListCreation = false;
         VisibleSelection selection = selectionForParagraphIteration(endingSelection());
         ASSERT(selection.isRange());
         VisiblePosition startOfSelection = selection.visibleStart();
         VisiblePosition endOfSelection = selection.visibleEnd();
         VisiblePosition startOfLastParagraph = startOfParagraph(endOfSelection, CanSkipOverEditingBoundary);
 
+        RefPtrWillBeRawPtr<Range> currentSelection = endingSelection().firstRange();
+        RefPtrWillBeRawPtr<ContainerNode> scopeForStartOfSelection = nullptr;
+        RefPtrWillBeRawPtr<ContainerNode> scopeForEndOfSelection = nullptr;
+        // FIXME: This is an inefficient way to keep selection alive because indexForVisiblePosition walks from

[tkent, 2014/06/27 05:04:57]

nit: recommend to wrap comments in 80-columns.

[arpitabahuguna, 2014/06/27 06:15:30]

Done.

+        // the beginning of the document to the endOfSelection everytime this code is executed.
+        // But not using index is hard because there are so many ways we can los eselection inside doApplyForSingleParagraph.
+        int indexForStartOfSelection = indexForVisiblePosition(startOfSelection, scopeForStartOfSelection);
+        int indexForEndOfSelection = indexForVisiblePosition(endOfSelection, scopeForEndOfSelection);
+
         if (startOfParagraph(startOfSelection, CanSkipOverEditingBoundary) != startOfLastParagraph) {
-            RefPtrWillBeRawPtr<ContainerNode> scope = nullptr;
-            int indexForEndOfSelection = indexForVisiblePosition(endOfSelection, scope);
-            bool forceCreateList = !selectionHasListOfType(selection, listTag);
+            forceListCreation = !selectionHasListOfType(selection, listTag);
 
-            RefPtrWillBeRawPtr<Range> currentSelection = endingSelection().firstRange();
             VisiblePosition startOfCurrentParagraph = startOfSelection;
             while (startOfCurrentParagraph.isNotNull() && !inSameParagraph(startOfCurrentParagraph, startOfLastParagraph, CanCrossEditingBoundary)) {
                 // doApply() may operate on and remove the last paragraph of the selection from the document
                 // if it's in the same list item as startOfCurrentParagraph.  Return early to avoid an
                 // infinite loop and because there is no more work to be done.
                 // FIXME(<rdar://problem/5983974>): The endingSelection() may be incorrect here.  Compute
                 // the new location of endOfSelection and use it as the end of the new selection.
                 if (!startOfLastParagraph.deepEquivalent().inDocument())
                     return;
                 setEndingSelection(startOfCurrentParagraph);
 
                 // Save and restore endOfSelection and startOfLastParagraph when necessary
                 // since moveParagraph and movePragraphWithClones can remove nodes.
-                // FIXME: This is an inefficient way to keep selection alive because indexForVisiblePosition walks from
-                // the beginning of the document to the endOfSelection everytime this code is executed.
-                // But not using index is hard because there are so many ways we can lose selection inside doApplyForSingleParagraph.
-                doApplyForSingleParagraph(forceCreateList, listTag, *currentSelection);
+                doApplyForSingleParagraph(forceListCreation, listTag, *currentSelection);
                 if (endOfSelection.isNull() || endOfSelection.isOrphan() || startOfLastParagraph.isNull() || startOfLastParagraph.isOrphan()) {
-                    endOfSelection = visiblePositionForIndex(indexForEndOfSelection, scope.get());
+                    endOfSelection = visiblePositionForIndex(indexForEndOfSelection, scopeForEndOfSelection.get());
                     // If endOfSelection is null, then some contents have been deleted from the document.
                     // This should never happen and if it did, exit early immediately because we've lost the loop invariant.
                     ASSERT(endOfSelection.isNotNull());
                     if (endOfSelection.isNull())
                         return;
                     startOfLastParagraph = startOfParagraph(endOfSelection, CanSkipOverEditingBoundary);
                 }
 
-                // Fetch the start of the selection after moving the first paragraph,
-                // because moving the paragraph will invalidate the original start.
-                // We'll use the new start to restore the original selection after
-                // we modified all selected paragraphs.
-                if (startOfCurrentParagraph == startOfSelection)
-                    startOfSelection = endingSelection().visibleStart();
-
                 startOfCurrentParagraph = startOfNextParagraph(endingSelection().visibleStart());
             }
             setEndingSelection(endOfSelection);
-            doApplyForSingleParagraph(forceCreateList, listTag, *currentSelection);
-            // Fetch the end of the selection, for the reason mentioned above.
-            if (endOfSelection.isNull() || endOfSelection.isOrphan()) {
-                endOfSelection = visiblePositionForIndex(indexForEndOfSelection, scope.get());
-                if (endOfSelection.isNull())
-                    return;
-            }
-            setEndingSelection(VisibleSelection(startOfSelection, endOfSelection, endingSelection().isDirectional()));
-            return;
         }
+        doApplyForSingleParagraph(forceListCreation, listTag, *currentSelection);
+        // Fetch the end of the selection, for the reason mentioned above.
+        if (endOfSelection.isNull() || endOfSelection.isOrphan()) {
+            endOfSelection = visiblePositionForIndex(indexForEndOfSelection, scopeForEndOfSelection.get());
+            if (endOfSelection.isNull())
+                return;
+        }
+        if (startOfSelection.isNull() || startOfSelection.isOrphan()) {
+            startOfSelection = visiblePositionForIndex(indexForStartOfSelection, scopeForStartOfSelection.get());
+            if (startOfSelection.isNull())
+                return;
+        }
+        setEndingSelection(VisibleSelection(startOfSelection, endOfSelection, endingSelection().isDirectional()));
+        return;
     }
 
     ASSERT(endingSelection().firstRange());
     doApplyForSingleParagraph(false, listTag, *endingSelection().firstRange());
 }
 
 void InsertListCommand::doApplyForSingleParagraph(bool forceCreateList, const QualifiedName& listTag, Range& currentSelection)
 {
     // FIXME: This will produce unexpected results for a selection that starts just before a
     // table and ends inside the first cell, selectionForParagraphIteration should probably
@@ skipping 204 matching lines @@
     return listElement;
 }
 
 void InsertListCommand::trace(Visitor* visitor)
 {
     visitor->trace(m_listElement);
     CompositeEditCommand::trace(visitor);
 }
 
 }