⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 254557 in webkit


Ignore:
Timestamp:
Jan 14, 2020, 9:57:39 PM (7 years ago)
Author:
rniwa@webkit.org
Message:

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

Reviewed by Darin Adler.

Source/WebCore:

This patch fixes the erroneously disabled debug assertion in HTMLTextFormControlElement::indexForPosition.

It also fixes the bug that it was asserting even when VisiblePosition was null, and computed a wrong offset
when the entire input element is not visible (e.g. becaue height is 0px).

TextIterator::rangeLength and TextIterator::rangeFromLocationAndLength now takes an OptionSet of
newly added enum class TextIteratorLengthOption instead of a boolean indicating whether a space should be
generated for a replaced element. Most code changes are due to this refactoring.

No new tests since existing tests exercise this code.

  • accessibility/AXObjectCache.cpp:

(WebCore::AXObjectCache::rangeMatchesTextNearRange):

  • accessibility/AccessibilityRenderObject.cpp:

(WebCore::AccessibilityRenderObject::indexForVisiblePosition const):

  • accessibility/atk/WebKitAccessibleInterfaceText.cpp:

(getSelectionOffsetsForObject):

  • accessibility/atk/WebKitAccessibleUtil.cpp:

(objectFocusedAndCaretOffsetUnignored):

  • editing/ApplyStyleCommand.cpp:

(WebCore::ApplyStyleCommand::applyBlockStyle):

  • editing/CompositeEditCommand.cpp:

(WebCore::CompositeEditCommand::moveParagraphs):

  • editing/Editing.cpp:

(WebCore::indexForVisiblePosition):
(WebCore::visiblePositionForIndex):

  • editing/Editing.h:

(WebCore::indexForVisiblePosition):

  • editing/TextIterator.cpp:

(WebCore::behaviorFromLegnthOptions): Added.
(WebCore::TextIterator::rangeLength):
(WebCore::TextIterator::rangeFromLocationAndLength):

  • editing/TextIterator.h:

(WebCore::TextIterator::rangeLength):
(WebCore::TextIterator::rangeFromLocationAndLength):

  • editing/TextIteratorBehavior.h:
  • editing/ios/DictationCommandIOS.cpp:

(WebCore::DictationCommandIOS::doApply):

  • html/HTMLTextFormControlElement.cpp:

(WebCore::HTMLTextFormControlElement::indexForPosition const): Enabled the assertion when VisiblePosition
is not null, and fixed the bug that the offset computed from VisiblePosition were always 0 when the input
element is not visible (e.g. has 0px size or has visibility: hidden).

  • page/EventHandler.cpp:

(WebCore::textDistance):

Source/WebKit:

  • WebProcess/WebPage/ios/WebPageIOS.mm:

(WebKit::rangeNearPositionMatchesText):

Location:
trunk/Source
Files:
18 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r254556 r254557  
     12020-01-14  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Enable the offset assertion in HTMLTextFormControlElement::indexForPosition
     4        https://bugs.webkit.org/show_bug.cgi?id=205706
     5
     6        Reviewed by Darin Adler.
     7
     8        This patch fixes the erroneously disabled debug assertion in HTMLTextFormControlElement::indexForPosition.
     9
     10        It also fixes the bug that it was asserting even when VisiblePosition was null, and computed a wrong offset
     11        when the entire input element is not visible (e.g. becaue height is 0px).
     12
     13        TextIterator::rangeLength and TextIterator::rangeFromLocationAndLength now takes an OptionSet of
     14        newly added enum class TextIteratorLengthOption instead of a boolean indicating whether a space should be
     15        generated for a replaced element. Most code changes are due to this refactoring.
     16
     17        No new tests since existing tests exercise this code.
     18
     19        * accessibility/AXObjectCache.cpp:
     20        (WebCore::AXObjectCache::rangeMatchesTextNearRange):
     21        * accessibility/AccessibilityRenderObject.cpp:
     22        (WebCore::AccessibilityRenderObject::indexForVisiblePosition const):
     23        * accessibility/atk/WebKitAccessibleInterfaceText.cpp:
     24        (getSelectionOffsetsForObject):
     25        * accessibility/atk/WebKitAccessibleUtil.cpp:
     26        (objectFocusedAndCaretOffsetUnignored):
     27        * editing/ApplyStyleCommand.cpp:
     28        (WebCore::ApplyStyleCommand::applyBlockStyle):
     29        * editing/CompositeEditCommand.cpp:
     30        (WebCore::CompositeEditCommand::moveParagraphs):
     31        * editing/Editing.cpp:
     32        (WebCore::indexForVisiblePosition):
     33        (WebCore::visiblePositionForIndex):
     34        * editing/Editing.h:
     35        (WebCore::indexForVisiblePosition):
     36        * editing/TextIterator.cpp:
     37        (WebCore::behaviorFromLegnthOptions): Added.
     38        (WebCore::TextIterator::rangeLength):
     39        (WebCore::TextIterator::rangeFromLocationAndLength):
     40        * editing/TextIterator.h:
     41        (WebCore::TextIterator::rangeLength):
     42        (WebCore::TextIterator::rangeFromLocationAndLength):
     43        * editing/TextIteratorBehavior.h:
     44        * editing/ios/DictationCommandIOS.cpp:
     45        (WebCore::DictationCommandIOS::doApply):
     46        * html/HTMLTextFormControlElement.cpp:
     47        (WebCore::HTMLTextFormControlElement::indexForPosition const): Enabled the assertion when VisiblePosition
     48        is not null, and fixed the bug that the offset computed from VisiblePosition were always 0 when the input
     49        element is not visible (e.g. has 0px size or has visibility: hidden).
     50        * page/EventHandler.cpp:
     51        (WebCore::textDistance):
     52
    1532020-01-14  Chris Dumez  <cdumez@apple.com>
    254

  • trunk/Source/WebCore/accessibility/AXObjectCache.cpp

    r254320 r254557  
    19341934   
    19351935    auto range = Range::create(m_document, startPosition, originalRange->startPosition());
    1936     unsigned targetOffset = TextIterator::rangeLength(range.ptr(), true);
     1936    unsigned targetOffset = TextIterator::rangeLength(range.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    19371937    return findClosestPlainText(searchRange.get(), matchText, { }, targetOffset);
    19381938}

  • trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp

    r253565 r254557  
    20332033    // We need to consider replaced elements for GTK, as they will be
    20342034    // presented with the 'object replacement character' (0xFFFC).
    2035     bool forSelectionPreservation = true;
     2035    return WebCore::indexForVisiblePosition(*node, position, { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    20362036#else
    2037     bool forSelectionPreservation = false;
     2037    return WebCore::indexForVisiblePosition(*node, position);
    20382038#endif
    2039 
    2040     return WebCore::indexForVisiblePosition(*node, position, forSelectionPreservation);
    20412039}
    20422040

  • trunk/Source/WebCore/accessibility/atk/WebKitAccessibleHyperlink.cpp

    r253261 r254557  
    155155{
    156156    // This is going to be the actual length in most of the cases
    157     int baseLength = TextIterator::rangeLength(range, true);
     157    int baseLength = TextIterator::rangeLength(range, { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    158158
    159159    // Check whether the current hyperlink belongs to a list item.

  • trunk/Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp

    r251798 r254557  
    411411    // Set values for start offsets and calculate initial range length.
    412412    // These values might be adjusted later to cover special cases.
    413     startOffset = webCoreOffsetToAtkOffset(coreObject, TextIterator::rangeLength(rangeInParent.ptr(), true));
     413    startOffset = webCoreOffsetToAtkOffset(coreObject, TextIterator::rangeLength(rangeInParent.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements }));
    414414    auto nodeRange = Range::create(node->document(), nodeRangeStart, nodeRangeEnd);
    415     int rangeLength = TextIterator::rangeLength(nodeRange.ptr(), true);
     415    int rangeLength = TextIterator::rangeLength(nodeRange.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    416416
    417417    // Special cases that are only relevant when working with *_END boundaries.

  • trunk/Source/WebCore/accessibility/atk/WebKitAccessibleUtil.cpp

    r251798 r254557  
    248248    else if (!isStartOfLine(endPosition)) {
    249249        RefPtr<Range> range = makeRange(startPosition, endPosition.previous());
    250         offset = TextIterator::rangeLength(range.get(), true) + 1;
     250        offset = TextIterator::rangeLength(range.get(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements }) + 1;
    251251    } else {
    252252        RefPtr<Range> range = makeRange(startPosition, endPosition);
    253         offset = TextIterator::rangeLength(range.get(), true);
     253        offset = TextIterator::rangeLength(range.get(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    254254    }
    255255

  • trunk/Source/WebCore/editing/ApplyStyleCommand.cpp

    r252392 r254557  
    253253    auto startRange = Range::create(document(), firstPositionInNode(scope), visibleStart.deepEquivalent().parentAnchoredEquivalent());
    254254    auto endRange = Range::create(document(), firstPositionInNode(scope), visibleEnd.deepEquivalent().parentAnchoredEquivalent());
    255     int startIndex = TextIterator::rangeLength(startRange.ptr(), true);
    256     int endIndex = TextIterator::rangeLength(endRange.ptr(), true);
     255    int startIndex = TextIterator::rangeLength(startRange.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
     256    int endIndex = TextIterator::rangeLength(endRange.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    257257
    258258    VisiblePosition paragraphStart(startOfParagraph(visibleStart));
    … …  
    286286   
    287287    {
    288         auto startRange = TextIterator::rangeFromLocationAndLength(scope, startIndex, 0, true);
    289         auto endRange = TextIterator::rangeFromLocationAndLength(scope, endIndex, 0, true);
     288        auto startRange = TextIterator::rangeFromLocationAndLength(scope, startIndex, 0, { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
     289        auto endRange = TextIterator::rangeFromLocationAndLength(scope, endIndex, 0, { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    290290        if (startRange && endRange)
    291291            updateStartEnd(startRange->startPosition(), endRange->startPosition());

  • trunk/Source/WebCore/editing/CompositeEditCommand.cpp

    r253750 r254557  
    14341434            if (startInParagraph) {
    14351435                auto startRange = Range::create(document(), startOfParagraphToMove.deepEquivalent().parentAnchoredEquivalent(), visibleStart.deepEquivalent().parentAnchoredEquivalent());
    1436                 startIndex = TextIterator::rangeLength(startRange.ptr(), true);
     1436                startIndex = TextIterator::rangeLength(startRange.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    14371437            }
    14381438
    … …  
    14401440            if (endInParagraph) {
    14411441                auto endRange = Range::create(document(), startOfParagraphToMove.deepEquivalent().parentAnchoredEquivalent(), visibleEnd.deepEquivalent().parentAnchoredEquivalent());
    1442                 endIndex = TextIterator::rangeLength(endRange.ptr(), true);
     1442                endIndex = TextIterator::rangeLength(endRange.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    14431443            }
    14441444        }
    … …  
    15111511
    15121512    auto startToDestinationRange = Range::create(document(), firstPositionInNode(editableRoot.get()), destination.deepEquivalent().parentAnchoredEquivalent());
    1513     destinationIndex = TextIterator::rangeLength(startToDestinationRange.ptr(), true);
     1513    destinationIndex = TextIterator::rangeLength(startToDestinationRange.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    15141514
    15151515    setEndingSelection(VisibleSelection(destination, originalIsDirectional));
    … …  
    15331533        // in a call to rangeFromLocationAndLength with a location past the end
    15341534        // of the document (which will return null).
    1535         RefPtr<Range> start = TextIterator::rangeFromLocationAndLength(editableRoot.get(), destinationIndex + startIndex, 0, true);
    1536         RefPtr<Range> end = TextIterator::rangeFromLocationAndLength(editableRoot.get(), destinationIndex + endIndex, 0, true);
     1535        RefPtr<Range> start = TextIterator::rangeFromLocationAndLength(editableRoot.get(), destinationIndex + startIndex, 0,
     1536            { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
     1537        RefPtr<Range> end = TextIterator::rangeFromLocationAndLength(editableRoot.get(), destinationIndex + endIndex, 0,
     1538            { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    15371539        if (start && end)
    15381540            setEndingSelection(VisibleSelection(start->startPosition(), end->startPosition(), DOWNSTREAM, originalIsDirectional));

  • trunk/Source/WebCore/editing/Editing.cpp

    r249605 r254557  
    10851085
    10861086    auto range = Range::create(document, firstPositionInNode(scope.get()), position.parentAnchoredEquivalent());
    1087     return TextIterator::rangeLength(range.ptr(), true);
     1087    return TextIterator::rangeLength(range.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    10881088}
    10891089
    10901090// FIXME: Merge this function with the one above.
    1091 int indexForVisiblePosition(Node& node, const VisiblePosition& visiblePosition, bool forSelectionPreservation)
     1091int indexForVisiblePosition(Node& node, const VisiblePosition& visiblePosition, OptionSet<TextIteratorLengthOption> options)
    10921092{
    10931093    auto range = Range::create(node.document(), firstPositionInNode(&node), visiblePosition.deepEquivalent().parentAnchoredEquivalent());
    1094     return TextIterator::rangeLength(range.ptr(), forSelectionPreservation);
     1094    return TextIterator::rangeLength(range.ptr(), options);
    10951095}
    10961096
    … …  
    11071107VisiblePosition visiblePositionForIndex(int index, ContainerNode* scope)
    11081108{
    1109     auto range = TextIterator::rangeFromLocationAndLength(scope, index, 0, true);
     1109    auto range = TextIterator::rangeFromLocationAndLength(scope, index, 0, { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    11101110    // Check for an invalid index. Certain editing operations invalidate indices because
    11111111    // of problems with TextIteratorEmitsCharactersBetweenAllVisiblePositions.

  • trunk/Source/WebCore/editing/Editing.h

    r249605 r254557  
    2727
    2828#include "Position.h"
     29#include "TextIteratorBehavior.h"
    2930#include <wtf/Forward.h>
    3031#include <wtf/HashSet.h>
     32#include <wtf/OptionSet.h>
    3133#include <wtf/unicode/CharacterNames.h>
    3234
    … …  
    150152
    151153WEBCORE_EXPORT int indexForVisiblePosition(const VisiblePosition&, RefPtr<ContainerNode>& scope);
    152 int indexForVisiblePosition(Node&, const VisiblePosition&, bool forSelectionPreservation);
     154int indexForVisiblePosition(Node&, const VisiblePosition&, OptionSet<TextIteratorLengthOption> = { });
    153155WEBCORE_EXPORT VisiblePosition visiblePositionForPositionWithOffset(const VisiblePosition&, int offset);
    154156WEBCORE_EXPORT VisiblePosition visiblePositionForIndex(int index, ContainerNode* scope);

  • trunk/Source/WebCore/editing/TextIterator.cpp

    r250343 r254557  
    23972397// --------
    23982398
    2399 int TextIterator::rangeLength(const Range* range, bool forSelectionPreservation)
     2399static TextIteratorBehavior behaviorFromLegnthOptions(OptionSet<TextIteratorLengthOption> options)
     2400{
     2401    TextIteratorBehavior behavior = TextIteratorDefaultBehavior;
     2402    if (options.contains(TextIteratorLengthOption::GenerateSpacesForReplacedElements))
     2403        behavior |= TextIteratorEmitsCharactersBetweenAllVisiblePositions;
     2404    if (options.contains(TextIteratorLengthOption::IgnoreVisibility))
     2405        behavior |= TextIteratorIgnoresStyleVisibility;
     2406    return behavior;
     2407}
     2408
     2409int TextIterator::rangeLength(const Range* range, OptionSet<TextIteratorLengthOption> options)
    24002410{
    24012411    unsigned length = 0;
    2402     for (TextIterator it(range, forSelectionPreservation ? TextIteratorEmitsCharactersBetweenAllVisiblePositions : TextIteratorDefaultBehavior); !it.atEnd(); it.advance())
     2412    for (TextIterator it(range, behaviorFromLegnthOptions(options)); !it.atEnd(); it.advance())
    24032413        length += it.text().length();
    24042414    return length;
    … …  
    24192429}
    24202430
    2421 RefPtr<Range> TextIterator::rangeFromLocationAndLength(ContainerNode* scope, int rangeLocation, int rangeLength, bool forSelectionPreservation)
     2431RefPtr<Range> TextIterator::rangeFromLocationAndLength(ContainerNode* scope, int rangeLocation, int rangeLength, OptionSet<TextIteratorLengthOption> options)
    24222432{
    24232433    Ref<Range> resultRange = scope->document().createRange();
    … …  
    24292439    Ref<Range> textRunRange = rangeOfContents(*scope);
    24302440
    2431     TextIterator it(textRunRange.ptr(), forSelectionPreservation ? TextIteratorEmitsCharactersBetweenAllVisiblePositions : TextIteratorDefaultBehavior);
     2441    TextIterator it(textRunRange.ptr(), behaviorFromLegnthOptions(options));
    24322442   
    24332443    // FIXME: the atEnd() check shouldn't be necessary, workaround for <http://bugs.webkit.org/show_bug.cgi?id=6289>.

  • trunk/Source/WebCore/editing/TextIterator.h

    r252528 r254557  
    3232#include "Range.h"
    3333#include "TextIteratorBehavior.h"
     34#include <wtf/OptionSet.h>
    3435#include <wtf/Vector.h>
    3536#include <wtf/text/StringView.h>
    … …  
    122123    void appendTextToStringBuilder(StringBuilder& builder) const { copyableText().appendToStringBuilder(builder); }
    123124
    124     WEBCORE_EXPORT static int rangeLength(const Range*, bool spacesForReplacedElements = false);
    125     WEBCORE_EXPORT static RefPtr<Range> rangeFromLocationAndLength(ContainerNode* scope, int rangeLocation, int rangeLength, bool spacesForReplacedElements = false);
     125    WEBCORE_EXPORT static int rangeLength(const Range*, OptionSet<TextIteratorLengthOption> = { });
     126    WEBCORE_EXPORT static RefPtr<Range> rangeFromLocationAndLength(ContainerNode* scope, int rangeLocation, int rangeLength, OptionSet<TextIteratorLengthOption> = { });
    126127    WEBCORE_EXPORT static bool getLocationAndLengthFromRange(Node* scope, const Range*, size_t& location, size_t& length);
    127128    WEBCORE_EXPORT static Ref<Range> subrange(Range& entireRange, int characterOffset, int characterCount);

  • trunk/Source/WebCore/editing/TextIteratorBehavior.h

    r210432 r254557  
    6464typedef unsigned short TextIteratorBehavior;
    6565
     66enum class TextIteratorLengthOption : uint8_t {
     67    GenerateSpacesForReplacedElements = 1 << 0,
     68    IgnoreVisibility = 1 << 1,
     69};
     70
    6671} // namespace WebCore

  • trunk/Source/WebCore/editing/ios/DictationCommandIOS.cpp

    r240237 r254557  
    6969    // FIXME: Add the result marker using a Position cached before results are inserted, instead of relying on TextIterators.
    7070    auto rangeToEnd = Range::create(document(), createLegacyEditingPosition((Node *)root, 0), afterResults.deepEquivalent());
    71     int endIndex = TextIterator::rangeLength(rangeToEnd.ptr(), true);
     71    int endIndex = TextIterator::rangeLength(rangeToEnd.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    7272    int startIndex = endIndex - resultLength;
    7373
    7474    if (startIndex >= 0) {
    75         RefPtr<Range> resultRange = TextIterator::rangeFromLocationAndLength(document().documentElement(), startIndex, endIndex, true);
     75        RefPtr<Range> resultRange = TextIterator::rangeFromLocationAndLength(document().documentElement(), startIndex, endIndex,
     76            { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    7677        ASSERT(resultRange); // FIXME: What guarantees this?
    7778        document().markers().addDictationResultMarker(*resultRange, m_metadata);

  • trunk/Source/WebCore/html/HTMLTextFormControlElement.cpp

    r254087 r254557  
    644644    unsigned length = innerTextValue().length();
    645645    index = std::min(index, length); // FIXME: We shouldn't have to call innerTextValue() just to ignore the last LF. See finishText.
    646 #if 0
    647     // FIXME: This assertion code was never built, has bit rotted, and needs to be fixed before it can be enabled:
    648     // https://bugs.webkit.org/show_bug.cgi?id=205706.
    649646#if ASSERT_ENABLED
    650647    VisiblePosition visiblePosition = passedPosition;
    651     unsigned indexComputedByVisiblePosition = 0;
    652     if (visiblePosition.isNotNull())
    653         indexComputedByVisiblePosition = WebCore::indexForVisiblePosition(innerText, visiblePosition, false /* forSelectionPreservation */);
    654     ASSERT(index == indexComputedByVisiblePosition);
    655 #endif
     648    if (visiblePosition.isNotNull()) {
     649        unsigned indexComputedByVisiblePosition = WebCore::indexForVisiblePosition(*innerText, visiblePosition,
     650            { TextIteratorLengthOption::GenerateSpacesForReplacedElements, TextIteratorLengthOption::IgnoreVisibility });
     651        ASSERT(index == indexComputedByVisiblePosition);
     652    }
    656653#endif
    657654    return index;

  • trunk/Source/WebCore/page/EventHandler.cpp

    r254029 r254557  
    658658{
    659659    auto range = Range::create(start.anchorNode()->document(), start, end);
    660     return TextIterator::rangeLength(range.ptr(), true);
     660    return TextIterator::rangeLength(range.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    661661}
    662662

  • trunk/Source/WebKit/ChangeLog

    r254556 r254557  
     12020-01-14  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Enable the offset assertion in HTMLTextFormControlElement::indexForPosition
     4        https://bugs.webkit.org/show_bug.cgi?id=205706
     5
     6        Reviewed by Darin Adler.
     7
     8        * WebProcess/WebPage/ios/WebPageIOS.mm:
     9        (WebKit::rangeNearPositionMatchesText):
     10
    1112020-01-14  Chris Dumez  <cdumez@apple.com>
    212

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

    r254387 r254557  
    19581958{
    19591959    auto range = Range::create(selectionRange->ownerDocument(), selectionRange->startPosition(), position.deepEquivalent().parentAnchoredEquivalent());
    1960     unsigned targetOffset = TextIterator::rangeLength(range.ptr(), true);
     1960    unsigned targetOffset = TextIterator::rangeLength(range.ptr(), { TextIteratorLengthOption::GenerateSpacesForReplacedElements });
    19611961    return findClosestPlainText(*selectionRange.get(), matchText, { }, targetOffset);
    19621962}
Note: See TracChangeset for help on using the changeset viewer.