Merge r256563 - Unreviewed, rolling out r254557.

https://bugs.webkit.org/show_bug.cgi?id=207725

The assert is correct, but unfortunately it will alwasy fail
since there is an existing bug in
HTMLTextFormControlElement::indexForPosition(). See bug

Reverted changeset:

"Enable the offset assertion in
HTMLTextFormControlElement::indexForPosition"
https://bugs.webkit.org/show_bug.cgi?id=205706
https://trac.webkit.org/changeset/254557

Source/WebCore/ChangeLog

@@ -1,3 +1,20 @@
+2020-02-13  Commit Queue  <commit-queue@webkit.org>
+
+        Unreviewed, rolling out r254557.
+        https://bugs.webkit.org/show_bug.cgi?id=207725
+
+        The assert is correct, but unfortunately it will alwasy fail
+        since there is an existing bug in
+        HTMLTextFormControlElement::indexForPosition(). See bug
+        #207724 for more details. (Requested by dydz on #webkit).
+
+        Reverted changeset:
+
+        "Enable the offset assertion in
+        HTMLTextFormControlElement::indexForPosition"
+        https://bugs.webkit.org/show_bug.cgi?id=205706
+        https://trac.webkit.org/changeset/254557
+
 2020-02-13  Adrian Perez de Castro  <aperez@igalia.com>
 
         Non-unified build fixes mid February 2020 edition

Source/WebCore/accessibility/AXObjectCache.cpp

@@ -1933,7 +1933,7 @@ RefPtr<Range> AXObjectCache::rangeMatchesTextNearRange(RefPtr<Range> originalRan
         return nullptr;
 
     auto range = Range::create(m_document, startPosition, originalRange->startPosition());
-    unsigned targetOffset = TextIterator::rangeLength(range.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+    unsigned targetOffset = TextIterator::rangeLength(range.ptr(), true);
     return findClosestPlainText(searchRange.get(), matchText, { }, targetOffset);
 }
 

Source/WebCore/accessibility/AccessibilityRenderObject.cpp

@@ -2032,10 +2032,12 @@ int AccessibilityRenderObject::indexForVisiblePosition(const VisiblePosition& po
 #if USE(ATK)
     // We need to consider replaced elements for GTK, as they will be
     // presented with the 'object replacement character' (0xFFFC).
-    return WebCore::indexForVisiblePosition(*node, position, { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+    bool forSelectionPreservation = true;
 #else
-    return WebCore::indexForVisiblePosition(*node, position);
+    bool forSelectionPreservation = false;
 #endif
+
+    return WebCore::indexForVisiblePosition(*node, position, forSelectionPreservation);
 }
 
 Element* AccessibilityRenderObject::rootEditableElementForPosition(const Position& position) const

Source/WebCore/accessibility/atk/WebKitAccessibleHyperlink.cpp

@@ -154,7 +154,7 @@ static AtkObject* webkitAccessibleHyperlinkGetObject(AtkHyperlink* link, gint in
 static gint rangeLengthForObject(AccessibilityObject& obj, Range* range)
 {
     // This is going to be the actual length in most of the cases
-    int baseLength = TextIterator::rangeLength(range, { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+    int baseLength = TextIterator::rangeLength(range, true);
 
     // Check whether the current hyperlink belongs to a list item.
     // If so, we need to consider the length of the item's marker

Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp

@@ -410,9 +410,9 @@ static void getSelectionOffsetsForObject(AccessibilityObject* coreObject, Visibl
 
     // Set values for start offsets and calculate initial range length.
     // These values might be adjusted later to cover special cases.
-    startOffset = webCoreOffsetToAtkOffset(coreObject, TextIterator::rangeLength(rangeInParent.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements }));
+    startOffset = webCoreOffsetToAtkOffset(coreObject, TextIterator::rangeLength(rangeInParent.ptr(), true));
     auto nodeRange = Range::create(node->document(), nodeRangeStart, nodeRangeEnd);
-    int rangeLength = TextIterator::rangeLength(nodeRange.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+    int rangeLength = TextIterator::rangeLength(nodeRange.ptr(), true);
 
     // Special cases that are only relevant when working with *_END boundaries.
     if (selection.affinity() == UPSTREAM) {

Source/WebCore/accessibility/atk/WebKitAccessibleUtil.cpp

@@ -247,10 +247,10 @@ AXCoreObject* objectFocusedAndCaretOffsetUnignored(AXCoreObject* referenceObject
         offset = 0;
     else if (!isStartOfLine(endPosition)) {
         RefPtr<Range> range = makeRange(startPosition, endPosition.previous());
-        offset = TextIterator::rangeLength(range.get(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements }) + 1;
+        offset = TextIterator::rangeLength(range.get(), true) + 1;
     } else {
         RefPtr<Range> range = makeRange(startPosition, endPosition);
-        offset = TextIterator::rangeLength(range.get(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+        offset = TextIterator::rangeLength(range.get(), true);
     }
 
     return firstUnignoredParent;

Source/WebCore/editing/ApplyStyleCommand.cpp

@@ -252,8 +252,8 @@ void ApplyStyleCommand::applyBlockStyle(EditingStyle& style)
 
     auto startRange = Range::create(document(), firstPositionInNode(scope), visibleStart.deepEquivalent().parentAnchoredEquivalent());
     auto endRange = Range::create(document(), firstPositionInNode(scope), visibleEnd.deepEquivalent().parentAnchoredEquivalent());
-    int startIndex = TextIterator::rangeLength(startRange.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
-    int endIndex = TextIterator::rangeLength(endRange.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+    int startIndex = TextIterator::rangeLength(startRange.ptr(), true);
+    int endIndex = TextIterator::rangeLength(endRange.ptr(), true);
 
     VisiblePosition paragraphStart(startOfParagraph(visibleStart));
     VisiblePosition nextParagraphStart(endOfParagraph(paragraphStart).next());
@@ -285,8 +285,8 @@ void ApplyStyleCommand::applyBlockStyle(EditingStyle& style)
     }
 
     {
-        auto startRange = TextIterator::rangeFromLocationAndLength(scope, startIndex, 0, { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
-        auto endRange = TextIterator::rangeFromLocationAndLength(scope, endIndex, 0, { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+        auto startRange = TextIterator::rangeFromLocationAndLength(scope, startIndex, 0, true);
+        auto endRange = TextIterator::rangeFromLocationAndLength(scope, endIndex, 0, true);
         if (startRange && endRange)
             updateStartEnd(startRange->startPosition(), endRange->startPosition());
     }

Source/WebCore/editing/CompositeEditCommand.cpp

@@ -1433,13 +1433,13 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
             startIndex = 0;
             if (startInParagraph) {
                 auto startRange = Range::create(document(), startOfParagraphToMove.deepEquivalent().parentAnchoredEquivalent(), visibleStart.deepEquivalent().parentAnchoredEquivalent());
-                startIndex = TextIterator::rangeLength(startRange.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+                startIndex = TextIterator::rangeLength(startRange.ptr(), true);
             }
 
             endIndex = 0;
             if (endInParagraph) {
                 auto endRange = Range::create(document(), startOfParagraphToMove.deepEquivalent().parentAnchoredEquivalent(), visibleEnd.deepEquivalent().parentAnchoredEquivalent());
-                endIndex = TextIterator::rangeLength(endRange.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+                endIndex = TextIterator::rangeLength(endRange.ptr(), true);
             }
         }
     }
@@ -1510,7 +1510,7 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
         editableRoot = &document();
 
     auto startToDestinationRange = Range::create(document(), firstPositionInNode(editableRoot.get()), destination.deepEquivalent().parentAnchoredEquivalent());
-    destinationIndex = TextIterator::rangeLength(startToDestinationRange.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+    destinationIndex = TextIterator::rangeLength(startToDestinationRange.ptr(), true);
 
     setEndingSelection(VisibleSelection(destination, originalIsDirectional));
     ASSERT(endingSelection().isCaretOrRange());
@@ -1532,10 +1532,8 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
         // causes spaces to be collapsed during the move operation.  This results
         // in a call to rangeFromLocationAndLength with a location past the end
         // of the document (which will return null).
-        RefPtr<Range> start = TextIterator::rangeFromLocationAndLength(editableRoot.get(), destinationIndex + startIndex, 0,
-            { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
-        RefPtr<Range> end = TextIterator::rangeFromLocationAndLength(editableRoot.get(), destinationIndex + endIndex, 0,
-            { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+        RefPtr<Range> start = TextIterator::rangeFromLocationAndLength(editableRoot.get(), destinationIndex + startIndex, 0, true);
+        RefPtr<Range> end = TextIterator::rangeFromLocationAndLength(editableRoot.get(), destinationIndex + endIndex, 0, true);
         if (start && end)
             setEndingSelection(VisibleSelection(start->startPosition(), end->startPosition(), DOWNSTREAM, originalIsDirectional));
     }

Source/WebCore/editing/Editing.cpp

@@ -1084,14 +1084,14 @@ int indexForVisiblePosition(const VisiblePosition& visiblePosition, RefPtr<Conta
     }
 
     auto range = Range::create(document, firstPositionInNode(scope.get()), position.parentAnchoredEquivalent());
-    return TextIterator::rangeLength(range.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+    return TextIterator::rangeLength(range.ptr(), true);
 }
 
 // FIXME: Merge this function with the one above.
-int indexForVisiblePosition(Node& node, const VisiblePosition& visiblePosition, OptionSet<TextIteratorLengthOption> options)
+int indexForVisiblePosition(Node& node, const VisiblePosition& visiblePosition, bool forSelectionPreservation)
 {
     auto range = Range::create(node.document(), firstPositionInNode(&node), visiblePosition.deepEquivalent().parentAnchoredEquivalent());
-    return TextIterator::rangeLength(range.ptr(), options);
+    return TextIterator::rangeLength(range.ptr(), forSelectionPreservation);
 }
 
 VisiblePosition visiblePositionForPositionWithOffset(const VisiblePosition& position, int offset)
@@ -1106,7 +1106,7 @@ VisiblePosition visiblePositionForPositionWithOffset(const VisiblePosition& posi
 
 VisiblePosition visiblePositionForIndex(int index, ContainerNode* scope)
 {
-    auto range = TextIterator::rangeFromLocationAndLength(scope, index, 0, { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+    auto range = TextIterator::rangeFromLocationAndLength(scope, index, 0, true);
     // Check for an invalid index. Certain editing operations invalidate indices because 
     // of problems with TextIteratorEmitsCharactersBetweenAllVisiblePositions.
     if (!range)

Source/WebCore/editing/Editing.h

@@ -26,10 +26,8 @@
 #pragma once
 
 #include "Position.h"
-#include "TextIteratorBehavior.h"
 #include <wtf/Forward.h>
 #include <wtf/HashSet.h>
-#include <wtf/OptionSet.h>
 #include <wtf/unicode/CharacterNames.h>
 
 namespace WebCore {
@@ -151,7 +149,7 @@ bool lineBreakExistsAtVisiblePosition(const VisiblePosition&);
 WEBCORE_EXPORT int comparePositions(const VisiblePosition&, const VisiblePosition&);
 
 WEBCORE_EXPORT int indexForVisiblePosition(const VisiblePosition&, RefPtr<ContainerNode>& scope);
-int indexForVisiblePosition(Node&, const VisiblePosition&, OptionSet<TextIteratorLengthOption> = { });
+int indexForVisiblePosition(Node&, const VisiblePosition&, bool forSelectionPreservation);
 WEBCORE_EXPORT VisiblePosition visiblePositionForPositionWithOffset(const VisiblePosition&, int offset);
 WEBCORE_EXPORT VisiblePosition visiblePositionForIndex(int index, ContainerNode* scope);
 VisiblePosition visiblePositionForIndexUsingCharacterIterator(Node&, int index); // FIXME: Why do we need this version?

Source/WebCore/editing/TextIterator.cpp

@@ -2396,20 +2396,10 @@ size_t SearchBuffer::length() const
 
 // --------
 
-static TextIteratorBehavior behaviorFromLegnthOptions(OptionSet<TextIteratorLengthOption> options)
-{
-    TextIteratorBehavior behavior = TextIteratorDefaultBehavior;
-    if (options.contains(TextIteratorLengthOption::GenerateSpacesForReplacedElements))
-        behavior |= TextIteratorEmitsCharactersBetweenAllVisiblePositions;
-    if (options.contains(TextIteratorLengthOption::IgnoreVisibility))
-        behavior |= TextIteratorIgnoresStyleVisibility;
-    return behavior;
-}
-
-int TextIterator::rangeLength(const Range* range, OptionSet<TextIteratorLengthOption> options)
+int TextIterator::rangeLength(const Range* range, bool forSelectionPreservation)
 {
     unsigned length = 0;
-    for (TextIterator it(range, behaviorFromLegnthOptions(options)); !it.atEnd(); it.advance())
+    for (TextIterator it(range, forSelectionPreservation ? TextIteratorEmitsCharactersBetweenAllVisiblePositions : TextIteratorDefaultBehavior); !it.atEnd(); it.advance())
         length += it.text().length();
     return length;
 }
@@ -2428,7 +2418,7 @@ static inline bool isInsideReplacedElement(TextIterator& iterator)
     return node && isRendererReplacedElement(node->renderer());
 }
 
-RefPtr<Range> TextIterator::rangeFromLocationAndLength(ContainerNode* scope, int rangeLocation, int rangeLength, OptionSet<TextIteratorLengthOption> options)
+RefPtr<Range> TextIterator::rangeFromLocationAndLength(ContainerNode* scope, int rangeLocation, int rangeLength, bool forSelectionPreservation)
 {
     Ref<Range> resultRange = scope->document().createRange();
 
@@ -2438,7 +2428,7 @@ RefPtr<Range> TextIterator::rangeFromLocationAndLength(ContainerNode* scope, int
 
     Ref<Range> textRunRange = rangeOfContents(*scope);
 
-    TextIterator it(textRunRange.ptr(), behaviorFromLegnthOptions(options));
+    TextIterator it(textRunRange.ptr(), forSelectionPreservation ? TextIteratorEmitsCharactersBetweenAllVisiblePositions : TextIteratorDefaultBehavior);
 
     // FIXME: the atEnd() check shouldn't be necessary, workaround for <http://bugs.webkit.org/show_bug.cgi?id=6289>.
     if (!rangeLocation && !rangeLength && it.atEnd()) {

Source/WebCore/editing/TextIterator.h

@@ -31,7 +31,6 @@
 #include "LineLayoutTraversal.h"
 #include "Range.h"
 #include "TextIteratorBehavior.h"
-#include <wtf/OptionSet.h>
 #include <wtf/Vector.h>
 #include <wtf/text/StringView.h>
 
@@ -122,8 +121,8 @@ class TextIterator {
     const TextIteratorCopyableText& copyableText() const { ASSERT(!atEnd()); return m_copyableText; }
     void appendTextToStringBuilder(StringBuilder& builder) const { copyableText().appendToStringBuilder(builder); }
 
-    WEBCORE_EXPORT static int rangeLength(const Range*, OptionSet<TextIteratorLengthOption> = { });
-    WEBCORE_EXPORT static RefPtr<Range> rangeFromLocationAndLength(ContainerNode* scope, int rangeLocation, int rangeLength, OptionSet<TextIteratorLengthOption> = { });
+    WEBCORE_EXPORT static int rangeLength(const Range*, bool spacesForReplacedElements = false);
+    WEBCORE_EXPORT static RefPtr<Range> rangeFromLocationAndLength(ContainerNode* scope, int rangeLocation, int rangeLength, bool spacesForReplacedElements = false);
     WEBCORE_EXPORT static bool getLocationAndLengthFromRange(Node* scope, const Range*, size_t& location, size_t& length);
     WEBCORE_EXPORT static Ref<Range> subrange(Range& entireRange, int characterOffset, int characterCount);
 

Source/WebCore/editing/TextIteratorBehavior.h

@@ -63,9 +63,4 @@ enum TextIteratorBehaviorFlag {
 
 typedef unsigned short TextIteratorBehavior;
 
-enum class TextIteratorLengthOption : uint8_t {
-    GenerateSpacesForReplacedElements = 1 << 0,
-    IgnoreVisibility = 1 << 1,
-};
-
 } // namespace WebCore

Source/WebCore/editing/ios/DictationCommandIOS.cpp

@@ -68,12 +68,11 @@ void DictationCommandIOS::doApply()
 
     // FIXME: Add the result marker using a Position cached before results are inserted, instead of relying on TextIterators.
     auto rangeToEnd = Range::create(document(), createLegacyEditingPosition((Node *)root, 0), afterResults.deepEquivalent());
-    int endIndex = TextIterator::rangeLength(rangeToEnd.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+    int endIndex = TextIterator::rangeLength(rangeToEnd.ptr(), true);
     int startIndex = endIndex - resultLength;
 
     if (startIndex >= 0) {
-        RefPtr<Range> resultRange = TextIterator::rangeFromLocationAndLength(document().documentElement(), startIndex, endIndex,
-            { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+        RefPtr<Range> resultRange = TextIterator::rangeFromLocationAndLength(document().documentElement(), startIndex, endIndex, true);
         ASSERT(resultRange); // FIXME: What guarantees this?
         document().markers().addDictationResultMarker(*resultRange, m_metadata);
     }

Source/WebCore/html/HTMLTextFormControlElement.cpp

@@ -655,13 +655,16 @@ unsigned HTMLTextFormControlElement::indexForPosition(const Position& passedPosi
 
     unsigned length = innerTextValue().length();
     index = std::min(index, length); // FIXME: We shouldn't have to call innerTextValue() just to ignore the last LF. See finishText.
+#if 0
+    // FIXME: This assertion code was never built, has bit rotted, and needs to be fixed before it can be enabled:
+    // https://bugs.webkit.org/show_bug.cgi?id=205706.
 #if ASSERT_ENABLED
     VisiblePosition visiblePosition = passedPosition;
-    if (visiblePosition.isNotNull()) {
-        unsigned indexComputedByVisiblePosition = WebCore::indexForVisiblePosition(*innerText, visiblePosition,
-            { TextIteratorLengthOption::GenerateSpacesForReplacedElements, TextIteratorLengthOption::IgnoreVisibility });
-        ASSERT(index == indexComputedByVisiblePosition);
-    }
+    unsigned indexComputedByVisiblePosition = 0;
+    if (visiblePosition.isNotNull())
+        indexComputedByVisiblePosition = WebCore::indexForVisiblePosition(innerText, visiblePosition, false /* forSelectionPreservation */);
+    ASSERT(index == indexComputedByVisiblePosition);
+#endif
 #endif
     return index;
 }

Source/WebCore/page/EventHandler.cpp

@@ -656,7 +656,7 @@ bool EventHandler::handleMousePressEventTripleClick(const MouseEventWithHitTestR
 static int textDistance(const Position& start, const Position& end)
 {
     auto range = Range::create(start.anchorNode()->document(), start, end);
-    return TextIterator::rangeLength(range.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+    return TextIterator::rangeLength(range.ptr(), true);
 }
 
 bool EventHandler::handleMousePressEventSingleClick(const MouseEventWithHitTestResults& event)

Source/WebKit/ChangeLog

@@ -1,3 +1,20 @@
+2020-02-13  Commit Queue  <commit-queue@webkit.org>
+
+        Unreviewed, rolling out r254557.
+        https://bugs.webkit.org/show_bug.cgi?id=207725
+
+        The assert is correct, but unfortunately it will alwasy fail
+        since there is an existing bug in
+        HTMLTextFormControlElement::indexForPosition(). See bug
+        #207724 for more details. (Requested by dydz on #webkit).
+
+        Reverted changeset:
+
+        "Enable the offset assertion in
+        HTMLTextFormControlElement::indexForPosition"
+        https://bugs.webkit.org/show_bug.cgi?id=205706
+        https://trac.webkit.org/changeset/254557
+
 2020-02-13  Said Abou-Hallawa  <said@apple.com>
 
         Unreviewed, rolling out r255158, 255405 and r255486

Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm

@@ -1958,7 +1958,7 @@ static IntPoint constrainPoint(const IntPoint& point, const Frame& frame, const
 static RefPtr<Range> rangeNearPositionMatchesText(const VisiblePosition& position, RefPtr<Range> originalRange, const String& matchText, RefPtr<Range> selectionRange)
 {
     auto range = Range::create(selectionRange->ownerDocument(), selectionRange->startPosition(), position.deepEquivalent().parentAnchoredEquivalent());
-    unsigned targetOffset = TextIterator::rangeLength(range.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
+    unsigned targetOffset = TextIterator::rangeLength(range.ptr(), true);
     return findClosestPlainText(*selectionRange.get(), matchText, { }, targetOffset);
 }