Changeset 163825 in webkit

Timestamp:

Feb 10, 2014 3:05:19 PM (10 years ago)

Author:

rniwa@webkit.org

Message:

HTMLTextFormControlElement::setSelectionRange shouldn't use VisiblePosition
​https://bugs.webkit.org/show_bug.cgi?id=128478

Reviewed by Darin Adler.

Added positionForIndex to compute Position given a selection index. This function doesn't
synchronously trigger like visiblePositionForIndex.

Also added assertions in visiblePositionForIndex and visiblePositionForIndex to make sure
they are inverse of one another.

• html/HTMLTextFormControlElement.cpp:

(WebCore::HTMLTextFormControlElement::setSelectionRange): Use positionForIndex. Also removed
the now tautological assertions since we would never create a position outside the inner text
element.

(WebCore::HTMLTextFormControlElement::indexForVisiblePosition): Fixed the bug where this
function could return a selection index beyond innerTextElement in some types of input
elements such as search fields. fast/forms/search-disabled-readonly.html hits the newly
added assertion without this change. Note we can't use visiblePositionForIndex here as that
would result in a mutual recursion with the assertion in visiblePositionForIndex.

(WebCore::HTMLTextFormControlElement::visiblePositionForIndex): Added an assertion.

(WebCore::positionForIndex): Added. It's here with prototype at the beginning of the file
so that it's right next to HTMLTextFormControlElement::innerText() where we do a similar
DOM traversal to obtain the inner text value.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• html/HTMLTextFormControlElement.cpp (modified) (4 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2014-02-10  Ryosuke Niwa  <rniwa@webkit.org>
+
+        HTMLTextFormControlElement::setSelectionRange shouldn't use VisiblePosition
+        https://bugs.webkit.org/show_bug.cgi?id=128478
+
+        Reviewed by Darin Adler.
+
+        Added positionForIndex to compute Position given a selection index. This function doesn't
+        synchronously trigger like visiblePositionForIndex.
+
+        Also added assertions in visiblePositionForIndex and visiblePositionForIndex to make sure
+        they are inverse of one another.
+
+        * html/HTMLTextFormControlElement.cpp:
+        (WebCore::HTMLTextFormControlElement::setSelectionRange): Use positionForIndex. Also removed
+        the now tautological assertions since we would never create a position outside the inner text
+        element.
+
+        (WebCore::HTMLTextFormControlElement::indexForVisiblePosition): Fixed the bug where this
+        function could return a selection index beyond innerTextElement in some types of input
+        elements such as search fields. fast/forms/search-disabled-readonly.html hits the newly
+        added assertion without this change. Note we can't use visiblePositionForIndex here as that
+        would result in a mutual recursion with the assertion in visiblePositionForIndex.
+
+        (WebCore::HTMLTextFormControlElement::visiblePositionForIndex): Added an assertion.
+
+        (WebCore::positionForIndex): Added. It's here with prototype at the beginning of the file
+        so that it's right next to HTMLTextFormControlElement::innerText() where we do a similar
+        DOM traversal to obtain the inner text value.
+
 2014-02-07  Jeffrey Pfau  <jpfau@apple.com>
 

trunk/Source/WebCore/html/HTMLTextFormControlElement.cpp

@@ -53 +53 @@
 using namespace HTMLNames;
 
+static Position positionForIndex(TextControlInnerTextElement*, unsigned);
+
 HTMLTextFormControlElement::HTMLTextFormControlElement(const QualifiedName& tagName, Document& document, HTMLFormElement* form)
     : HTMLFormControlElementWithState(tagName, document, form)
@@ -297 +299 @@
     start = std::min(std::max(start, 0), end);
 
-    if (!hasVisibleTextArea(*renderer(), innerTextElement())) {
+    TextControlInnerTextElement* innerText = innerTextElement();
+    if (!hasVisibleTextArea(*renderer(), innerText)) {
         cacheSelection(start, end, direction);
         return;
     }
-    VisiblePosition startPosition = visiblePositionForIndex(start);
-    VisiblePosition endPosition;
+    Position startPosition = positionForIndex(innerText, start);
+    Position endPosition;
     if (start == end)
         endPosition = startPosition;
     else
-        endPosition = visiblePositionForIndex(end);
-
-#if !PLATFORM(IOS)
-    // startPosition and endPosition can be null position for example when
-    // "-webkit-user-select: none" style attribute is specified.
-    if (startPosition.isNotNull() && endPosition.isNotNull()) {
-        ASSERT(startPosition.deepEquivalent().deprecatedNode()->shadowHost() == this
-            && endPosition.deepEquivalent().deprecatedNode()->shadowHost() == this);
-    }
-#endif
+        endPosition = positionForIndex(innerText, end);
+
     VisibleSelection newSelection;
     if (direction == SelectionHasBackwardDirection)
@@ -329 +324 @@
 int HTMLTextFormControlElement::indexForVisiblePosition(const VisiblePosition& pos) const
 {
-    if (enclosingTextFormControl(pos.deepEquivalent()) != this)
+    TextControlInnerTextElement* innerText = innerTextElement();
+    if (!innerText || !innerText->contains(pos.deepEquivalent().anchorNode()))
         return 0;
     bool forSelectionPreservation = false;
-    return WebCore::indexForVisiblePosition(innerTextElement(), pos, forSelectionPreservation);
+    unsigned index = WebCore::indexForVisiblePosition(innerTextElement(), pos, forSelectionPreservation);
+    ASSERT(VisiblePosition(positionForIndex(innerTextElement(), index)) == pos);
+    return index;
 }
 
 VisiblePosition HTMLTextFormControlElement::visiblePositionForIndex(int index) const
 {
-    return visiblePositionForIndexUsingCharacterIterator(innerTextElement(), index);
+    VisiblePosition position = positionForIndex(innerTextElement(), index);
+    ASSERT(indexForVisiblePosition(position) == index);
+    return position;
 }
 
@@ -556 +556 @@
     }
     return finishText(result);
+}
+
+static Position positionForIndex(TextControlInnerTextElement* innerText, unsigned index)
+{
+    unsigned remainingCharactersToMoveForward = index;
+    for (Node* node = innerText; node; node = NodeTraversal::next(node, innerText)) {
+        if (node->hasTagName(brTag)) {
+            if (!remainingCharactersToMoveForward)
+                return positionBeforeNode(node);
+            remainingCharactersToMoveForward--;
+        } else if (node->isTextNode()) {
+            Text* text = toText(node);
+            if (remainingCharactersToMoveForward < text->length())
+                return Position(text, remainingCharactersToMoveForward);
+            remainingCharactersToMoveForward -= text->length();
+        }
+    }
+    return lastPositionInNode(innerText);
 }