- Revision
- 90072
- Author
- [email protected]
- Date
- 2011-06-29 17:10:53 -0700 (Wed, 29 Jun 2011)
Log Message
2011-06-29 Justin Garcia <[email protected]>
Reviewed by Enrica Casucci.
https://bugs.webkit.org/show_bug.cgi?id=62922
indexForVisiblePosition(const VisiblePosition& visiblePosition) does not consider shadow content
VisiblePositions can be inside web form text regions, which use shadow trees. Made indexForVisiblePosition
aware of this, and added a new parameter to obtain the scope for a VisiblePosition, in addition to its index.
Added visiblePositionForIndex to go in the opposite direction, taking into account the scope
used to compute the index.
These two functions use TextIterators to convert between VisiblePositions and indices. But
TextIterator iteration using TextIteratorEmitsCharactersBetweenAllVisiblePositions does not
exactly match VisiblePosition iteration, so using them to preserve a selection during an
editing operation is unreliable. This can be seen in the expected results for:
editing/execCommand/indent-pre-list.html
editing/execCommand/crash-indenting-list-item.html
TextIterator's TextIteratorEmitsCharactersBetweenAllVisiblePositions mode needs to be fixed, or
these functions need to be changed to iterate using actual VisiblePositions. See:
https://bugs.webkit.org/show_bug.cgi?id=63590
TextIterators in TextIteratorEmitsCharactersBetweenAllVisiblePositions do not exactly match VisiblePositions
Also:
https://bugs.webkit.org/show_bug.cgi?id=63592
Use visiblePositionForIndex and indexForVisiblePosition everywhere that TextIterators are used to convert between VisiblePositions and indices
No new tests added because indexForVisiblePosition is currently only used for editing operations
that cannot be performed inside web form fields.
* editing/ApplyBlockElementCommand.cpp:
(WebCore::ApplyBlockElementCommand::doApply):
* editing/InsertListCommand.cpp:
(WebCore::InsertListCommand::doApply):
* editing/htmlediting.cpp:
(WebCore::indexForVisiblePosition):
(WebCore::visiblePositionForIndex):
* editing/htmlediting.h:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (90071 => 90072)
--- trunk/Source/WebCore/ChangeLog 2011-06-30 00:03:36 UTC (rev 90071)
+++ trunk/Source/WebCore/ChangeLog 2011-06-30 00:10:53 UTC (rev 90072)
@@ -1,3 +1,47 @@
+2011-06-29 Justin Garcia <[email protected]>
+
+ Reviewed by Enrica Casucci.
+
+ https://bugs.webkit.org/show_bug.cgi?id=62922
+ indexForVisiblePosition(const VisiblePosition& visiblePosition) does not consider shadow content
+
+ VisiblePositions can be inside web form text regions, which use shadow trees. Made indexForVisiblePosition
+ aware of this, and added a new parameter to obtain the scope for a VisiblePosition, in addition to its index.
+
+ Added visiblePositionForIndex to go in the opposite direction, taking into account the scope
+ used to compute the index.
+
+ These two functions use TextIterators to convert between VisiblePositions and indices. But
+ TextIterator iteration using TextIteratorEmitsCharactersBetweenAllVisiblePositions does not
+ exactly match VisiblePosition iteration, so using them to preserve a selection during an
+ editing operation is unreliable. This can be seen in the expected results for:
+
+ editing/execCommand/indent-pre-list.html
+ editing/execCommand/crash-indenting-list-item.html
+
+ TextIterator's TextIteratorEmitsCharactersBetweenAllVisiblePositions mode needs to be fixed, or
+ these functions need to be changed to iterate using actual VisiblePositions. See:
+
+ https://bugs.webkit.org/show_bug.cgi?id=63590
+ TextIterators in TextIteratorEmitsCharactersBetweenAllVisiblePositions do not exactly match VisiblePositions
+
+ Also:
+
+ https://bugs.webkit.org/show_bug.cgi?id=63592
+ Use visiblePositionForIndex and indexForVisiblePosition everywhere that TextIterators are used to convert between VisiblePositions and indices
+
+ No new tests added because indexForVisiblePosition is currently only used for editing operations
+ that cannot be performed inside web form fields.
+
+ * editing/ApplyBlockElementCommand.cpp:
+ (WebCore::ApplyBlockElementCommand::doApply):
+ * editing/InsertListCommand.cpp:
+ (WebCore::InsertListCommand::doApply):
+ * editing/htmlediting.cpp:
+ (WebCore::indexForVisiblePosition):
+ (WebCore::visiblePositionForIndex):
+ * editing/htmlediting.h:
+
2011-06-29 Dimitri Glazkov <[email protected]>
Reviewed by Kent Tamura.
Modified: trunk/Source/WebCore/editing/ApplyBlockElementCommand.cpp (90071 => 90072)
--- trunk/Source/WebCore/editing/ApplyBlockElementCommand.cpp 2011-06-30 00:03:36 UTC (rev 90071)
+++ trunk/Source/WebCore/editing/ApplyBlockElementCommand.cpp 2011-06-30 00:10:53 UTC (rev 90072)
@@ -81,17 +81,24 @@
VisiblePosition endOfSelection = selection.visibleEnd();
ASSERT(!startOfSelection.isNull());
ASSERT(!endOfSelection.isNull());
- int startIndex = indexForVisiblePosition(startOfSelection);
- int endIndex = indexForVisiblePosition(endOfSelection);
+ Element* startScope = 0;
+ int startIndex = indexForVisiblePosition(startOfSelection, &startScope);
+ Element* endScope = 0;
+ int endIndex = indexForVisiblePosition(endOfSelection, &endScope);
formatSelection(startOfSelection, endOfSelection);
updateLayout();
-
- RefPtr<Range> startRange = TextIterator::rangeFromLocationAndLength(document()->documentElement(), startIndex, 0, true);
- RefPtr<Range> endRange = TextIterator::rangeFromLocationAndLength(document()->documentElement(), endIndex, 0, true);
- if (startRange && endRange)
- setEndingSelection(VisibleSelection(startRange->startPosition(), endRange->startPosition(), DOWNSTREAM));
+
+ ASSERT(startScope == endScope);
+ ASSERT(startIndex >= 0);
+ ASSERT(startIndex <= endIndex);
+ if (startScope == endScope && startIndex >= 0 && startIndex <= endIndex) {
+ VisiblePosition start(visiblePositionForIndex(startIndex, startScope));
+ VisiblePosition end(visiblePositionForIndex(endIndex, endScope));
+ if (start.isNotNull() && end.isNotNull())
+ setEndingSelection(VisibleSelection(start, end));
+ }
}
void ApplyBlockElementCommand::formatSelection(const VisiblePosition& startOfSelection, const VisiblePosition& endOfSelection)
Modified: trunk/Source/WebCore/editing/InsertListCommand.cpp (90071 => 90072)
--- trunk/Source/WebCore/editing/InsertListCommand.cpp 2011-06-30 00:03:36 UTC (rev 90071)
+++ trunk/Source/WebCore/editing/InsertListCommand.cpp 2011-06-30 00:10:53 UTC (rev 90072)
@@ -152,16 +152,16 @@
// 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.
- int indexForEndOfSelection = indexForVisiblePosition(endOfSelection);
+ Element* scope = 0;
+ int indexForEndOfSelection = indexForVisiblePosition(endOfSelection, &scope);
doApplyForSingleParagraph(forceCreateList, listTag, currentSelection.get());
if (endOfSelection.isNull() || endOfSelection.isOrphan() || startOfLastParagraph.isNull() || startOfLastParagraph.isOrphan()) {
- RefPtr<Range> lastSelectionRange = TextIterator::rangeFromLocationAndLength(document()->documentElement(), indexForEndOfSelection, 0, true);
- // If lastSelectionRange is null, then some contents have been deleted from the document.
+ endOfSelection = visiblePositionForIndex(indexForEndOfSelection, scope);
+ // 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(lastSelectionRange);
- if (!lastSelectionRange)
+ ASSERT(endOfSelection.isNotNull());
+ if (endOfSelection.isNull())
return;
- endOfSelection = lastSelectionRange->startPosition();
startOfLastParagraph = startOfParagraph(endOfSelection, CanSkipOverEditingBoundary);
}
Modified: trunk/Source/WebCore/editing/VisiblePosition.h (90071 => 90072)
--- trunk/Source/WebCore/editing/VisiblePosition.h 2011-06-30 00:03:36 UTC (rev 90071)
+++ trunk/Source/WebCore/editing/VisiblePosition.h 2011-06-30 00:10:53 UTC (rev 90072)
@@ -79,6 +79,7 @@
UChar32 characterAfter() const;
UChar32 characterBefore() const { return previous().characterAfter(); }
+ // FIXME: This does not handle [table, 0] correctly.
Element* rootEditableElement() const { return m_deepPosition.isNotNull() ? m_deepPosition.deprecatedNode()->rootEditableElement() : 0; }
void getInlineBoxAndOffset(InlineBox*& inlineBox, int& caretOffset) const
Modified: trunk/Source/WebCore/editing/htmlediting.cpp (90071 => 90072)
--- trunk/Source/WebCore/editing/htmlediting.cpp 2011-06-30 00:03:36 UTC (rev 90071)
+++ trunk/Source/WebCore/editing/htmlediting.cpp 2011-06-30 00:10:53 UTC (rev 90072)
@@ -1010,17 +1010,51 @@
return newSelection;
}
-
-int indexForVisiblePosition(const VisiblePosition& visiblePosition)
+// FIXME: indexForVisiblePosition and visiblePositionForIndex use TextIterators to convert between
+// VisiblePositions and indices. But TextIterator iteration using TextIteratorEmitsCharactersBetweenAllVisiblePositions
+// does not exactly match VisiblePosition iteration, so using them to preserve a selection during an editing
+// opertion is unreliable. TextIterator's TextIteratorEmitsCharactersBetweenAllVisiblePositions mode needs to be fixed,
+// or these functions need to be changed to iterate using actual VisiblePositions.
+// FIXME: Deploy these functions everywhere that TextIterators are used to convert between VisiblePositions and indices.
+int indexForVisiblePosition(const VisiblePosition& visiblePosition, Element **scope)
{
if (visiblePosition.isNull())
return 0;
+
Position p(visiblePosition.deepEquivalent());
- RefPtr<Range> range = Range::create(p.anchorNode()->document(), firstPositionInNode(p.anchorNode()->document()->documentElement()),
+ Document* document = p.anchorNode()->document();
+
+ Element* root;
+ Node* shadowRoot = p.anchorNode()->shadowTreeRootNode();
+
+ if (shadowRoot) {
+ // Use the shadow root for form elements, since TextIterators will not enter shadow content.
+ ASSERT(shadowRoot->isElementNode());
+ root = static_cast<Element *>(shadowRoot);
+ } else
+ root = document->documentElement();
+
+ if (scope) {
+ ASSERT(!*scope);
+ *scope = root;
+ }
+
+ RefPtr<Range> range = Range::create(document,
+ firstPositionInNode(root),
p.parentAnchoredEquivalent());
+
return TextIterator::rangeLength(range.get(), true);
}
+VisiblePosition visiblePositionForIndex(int index, Element *scope)
+{
+ RefPtr<Range> range = TextIterator::rangeFromLocationAndLength(scope, index, 0, true);
+ // indexForVisiblePosition can create a
+ if (!range)
+ return VisiblePosition();
+ return VisiblePosition(range->startPosition());
+}
+
// Determines whether two positions are visibly next to each other (first then second)
// while ignoring whitespaces and unrendered nodes
bool isVisiblyAdjacent(const Position& first, const Position& second)
Modified: trunk/Source/WebCore/editing/htmlediting.h (90071 => 90072)
--- trunk/Source/WebCore/editing/htmlediting.h 2011-06-30 00:03:36 UTC (rev 90071)
+++ trunk/Source/WebCore/editing/htmlediting.h 2011-06-30 00:10:53 UTC (rev 90072)
@@ -173,8 +173,10 @@
bool lineBreakExistsAtVisiblePosition(const VisiblePosition&);
int comparePositions(const VisiblePosition&, const VisiblePosition&);
-int indexForVisiblePosition(const VisiblePosition&);
+int indexForVisiblePosition(const VisiblePosition&, Element **scope);
+VisiblePosition visiblePositionForIndex(int index, Element *scope);
+
// -------------------------------------------------------------------------
// Range
// -------------------------------------------------------------------------
