Bug 166955 – AX: WKContentView needs to implement UITextInput methods to make speak selection highlighting work

• Component: Accessibility
• Status: RESOLVED FIXED

[Nan Wang]

- (UITextPosition *)positionFromPosition:(UITextPosition *)position offset:(NSInteger)offset
- (UITextRange *)textRangeFromPosition:(UITextPosition *)fromPosition toPosition:(UITextPosition *)toPosition
- (UITextRange *)rangeEnclosingPosition:(UITextPosition *)position withGranularity:(UITextGranularity)granularity inDirection:(UITextDirection)direction

Implement the above functions to allow Speak Selection to make sub ranges, get the rects and draw an appropriate highlight

<rdar://problem/19071617>

[Nan Wang]

Created attachment 298648 [details]
Initial patch

[WebKit Commit Bot]

Attachment 298648 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:2910:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:2912:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:2913:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3642:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3644:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3645:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 298648 [details]
Initial patch

Attachment 298648 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2875985

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298667 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Nan Wang]

Created attachment 298733 [details]
patch

Build fix

[WebKit Commit Bot]

Attachment 298733 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:2910:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:2912:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:2913:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3642:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3644:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3645:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 298733 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298733&action=review

> Source/WebKit2/WebProcess/WebPage/WebPage.messages.in:101
> +    RectForPointWithOffset(WebCore::IntPoint point, int32_t offset) -> (WebCore::FloatRect rect);
> +    SelectionRectsForPositions(WebCore::IntPoint from, WebCore::IntPoint to) -> (Vector<WebCore::SelectionRect> selectionRects, uint64_t length);
> +    SelectionRectsForGranularityAtPosition(WebCore::IntPoint point, uint32_t granularity) -> (Vector<WebCore::SelectionRect> selectionRects, WebCore::FloatRect startRect, WebCore::FloatRect endRect, uint64_t length);

Please do not add any additional synchronous messages (an please don't call any existing ones).  Please re-work this to not require synchronous messaging to the WebProcess.

[Nan Wang]

Created attachment 299261 [details]
patch

Got rid of the synchronous messages.

[Ryosuke Niwa]

Comment on attachment 299261 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299261&action=review

> Source/WebCore/dom/Range.cpp:1269
> -void Range::collectSelectionRects(Vector<SelectionRect>& rects)
> +void Range::collectSelectionRectsWithoutUnionInteriorLines(Vector<SelectionRect>& rects, int& maxLineNumber)

Why isn't this function simply returning the maxLineNumber?

> Source/WebCore/dom/Range.cpp:1432
> +    int maxLineNumber;
> +    collectSelectionRectsWithoutUnionInteriorLines(rects, maxLineNumber);
> +    const size_t numberOfRects = rects.size();
> +    

A pattern like this where we rely on a function being called to set a value to maxLineNumber is dangerous.
If someone adds an early return in collectSelectionRectsWithoutUnionInteriorLines, for example,
we may end up never initializing maxLineNumber.

It's better if collectSelectionRectsWithoutUnionInteriorLines returned this value
so that maxLineNumber is always assigned some value, and the code leaves no ambiguity in that regard.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1819
> +    // Hit test the position make sure it's visible.

*to* make sure.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1822
> +    FloatRect rect = result.absoluteCaretBounds();
> +    IntPoint center = IntPoint(rect.center());
> +    result = frame.visiblePositionForPoint(center);

Are you sure we really need to do this? This is a very funky operation.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1839
> +    WebCore::TextGranularity webGranularity = static_cast<WebCore::TextGranularity>(granularity);

"web" prefix is usually used for WK/WebView types, not WebCore types.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1925
> +        // Try to search for a word range within the selection range that matches the passed in text.
> +        if (RefPtr<Range> wordRange = wordRangeForPositionMatchesText(startPosition, range, text, selectionRange))
> +            range = wordRange;

Why can't we just use findPlainText instead? That'd just return a range that includes the text inside a given range.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1940
> +    if (!resultRange || resultRange->collapsed())
> +        resultRange = range;

Why do we want to do this?

[Nan Wang]

Created attachment 299684 [details]
patch

- simplified text search logic
- updated from review

[Ryosuke Niwa]

Comment on attachment 299684 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299684&action=review

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1870
> +    while (!previous.isNull() || !next.isNull()) {
> +        previous = !previous.isNull() ? previous.previous() : previous;
> +        next = !next.isNull() ? next.next() : next;

This is quite possibly the most inefficient way of expanding positions all the way to the beginning or the end of a document.
Why can't we just use findPlainText? Please explain in the change or in the comment.

[Nan Wang]

Comment on attachment 299684 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299684&action=review

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1870
>> +        next = !next.isNull() ? next.next() : next;
> 
> This is quite possibly the most inefficient way of expanding positions all the way to the beginning or the end of a document.
> Why can't we just use findPlainText? Please explain in the change or in the comment.

I've got a comment already: Try to search for a range which is the closest to the position within the selection range that matches the passed in text.

I think the problem with findPlainText is that the result range might not be the closest or includes the position we passed in. 

e.g. we want to search for the second "the" in "the car is the fastest in the world" at offset 12 (at "h" of the second "the"), findPlainText might return the range of the first or third "the" depending on the range and direction we passed in.

[Nan Wang]

Comment on attachment 299684 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299684&action=review

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1870
>> +        next = !next.isNull() ? next.next() : next;
> 
> This is quite possibly the most inefficient way of expanding positions all the way to the beginning or the end of a document.
> Why can't we just use findPlainText? Please explain in the change or in the comment.

I've got a comment already: Try to search for a range which is the closest to the position within the selection range that matches the passed in text.

I think the problem with findPlainText is that the result range might not be the closest or includes the position we passed in. 

e.g. we want to search for the second "the" in "the car is the fastest in the world" at offset 12 (at "h" of the second "the"), findPlainText might return the range of the first or third "the" depending on the range and direction we passed in.

[Ryosuke Niwa]

> e.g. we want to search for the second "the" in "the car is the fastest in the world" at offset 12 (at "h" of the second "the"), findPlainText might return the range of the first or third "the" depending on the range and direction we passed in.

What's the desired behavior then?

[Nan Wang]

(In reply to comment #14)
> > e.g. we want to search for the second "the" in "the car is the fastest in the world" at offset 12 (at "h" of the second "the"), findPlainText might return the range of the first or third "the" depending on the range and direction we passed in.
> 
> What's the desired behavior then?

We want it to return the range of the second "the", from offset 11 to 14. This is the closest match to the offset we passed in.

[Ryosuke Niwa]

Comment on attachment 299684 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299684&action=review

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1850
> +    return rangeText.length() > 0 && (rangeText.contains(matchText) || matchText.contains(rangeText));

I don't think this does the right case folding.

[Ryosuke Niwa]

(In reply to comment #15)
> (In reply to comment #14)
> > > e.g. we want to search for the second "the" in "the car is the fastest in the world" at offset 12 (at "h" of the second "the"), findPlainText might return the range of the first or third "the" depending on the range and direction we passed in.
> > 
> > What's the desired behavior then?
> 
> We want it to return the range of the second "the", from offset 11 to 14.
> This is the closest match to the offset we passed in.

Ah, in that case, we should probably add a new variant of findPlainText that does this instead of implementing a brand new text search algorithm in WK2 layer. The code you're adding doesn't take care of case differences, code point equivalency, etc... that we've  fixed over many years of the development of WebKit.

[Ryosuke Niwa]

Also, please add tests. We should be able to test this in API tests. If we add a new variant of findPlainText, we should even be able to add internals method to test that function separately.

We need lots of test cases involving Japanese half-width katakana, German SS, CSS transformed text, etc...

[Nan Wang]

(In reply to comment #17)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > > e.g. we want to search for the second "the" in "the car is the fastest in the world" at offset 12 (at "h" of the second "the"), findPlainText might return the range of the first or third "the" depending on the range and direction we passed in.
> > > 
> > > What's the desired behavior then?
> > 
> > We want it to return the range of the second "the", from offset 11 to 14.
> > This is the closest match to the offset we passed in.
> 
> Ah, in that case, we should probably add a new variant of findPlainText that
> does this instead of implementing a brand new text search algorithm in WK2
> layer. The code you're adding doesn't take care of case differences, code
> point equivalency, etc... that we've  fixed over many years of the
> development of WebKit.

I think in our case we want case sensitive matched result. But I'll take a look at the findPlainText function.

[Nan Wang]

Created attachment 299871 [details]
patch with test

Added test.

[Ryosuke Niwa]

Comment on attachment 299871 [details]
patch with test

View in context: https://bugs.webkit.org/attachment.cgi?id=299871&action=review

> LayoutTests/accessibility/ios-simulator/speak-selection-word-query-expected.txt:11
> +Content: the
> +Content: tHe
> +Content: The

This doesn't tell us anything about where the selected content is.
We could have just used findPlainText, and we would have gotten the same result.
Also, we need tests with text-transform: uppercase, German S transformation, as well as CJK, and bidirectional text.

[Ryosuke Niwa]

Comment on attachment 299871 [details]
patch with test

View in context: https://bugs.webkit.org/attachment.cgi?id=299871&action=review

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1863
> +    // Sometimes we did not advance enough to create a text range, e.g. when there's a soft line break.

How does soft line break prevent Position from moving forward/backward?
This definitely needs a test case.

[Nan Wang]

(In reply to comment #22)
> Comment on attachment 299871 [details]
> patch with test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299871&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1863
> > +    // Sometimes we did not advance enough to create a text range, e.g. when there's a soft line break.
> 
> How does soft line break prevent Position from moving forward/backward?
> This definitely needs a test case.

It doesn't prevent position from moving forward/backward. The problem is that the offset we passed in comes from NSRange from a string and VisiblePosition sometimes need to advance more to get the same string.

[Nan Wang]

Created attachment 299952 [details]
patch

Added a new variant of findPlainText.

[Ryosuke Niwa]

Comment on attachment 299952 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299952&action=review

Wow, the code looks much nicer! & great test cases!
Please address the comments below and we're good to go!

> Source/WebCore/editing/TextIterator.cpp:2659
> -static std::optional<std::pair<size_t, size_t>> findPlainTextOffset(SearchBuffer& buffer, CharacterIterator& findIterator, bool searchForward)
> +static std::optional<std::pair<size_t, size_t>> findPlainTextOffset(SearchBuffer& buffer, CharacterIterator& findIterator, const std::function<bool(size_t&, size_t&, size_t, size_t)>& found)

It's cleaner if this function just returned void and instead each lambda kept matchStart, matchEnd themselves.
We should probably rename this function to findPlainTextMatches() to signify the fact we just calls the callback.

Once we did that, this function's lambda can just be std::function<bool(size_t, size_t)>.

> Source/WebCore/editing/TextIterator.cpp:2677
> -            if (searchForward) // Look for the last match when searching backwards instead.
> +            if (found(matchStart, matchLength, lastCharacterInBufferOffset - matchStartOffset, newMatchLength))

Once we made the change above, we should just call this function callback.
Basically, for backwards searching and your function, this function will always return false.
For forward searching, it always returns true (since the first match should be returned).

> Source/WebCore/editing/TextIterator.cpp:2688
> -Ref<Range> findPlainText(const Range& range, const String& target, FindOptions options)
> +static Ref<Range> findPlainTextWithFunc(const Range& range, const String& target, FindOptions options, const std::function<bool(size_t&, size_t&, size_t, size_t)>& func)

Please don't use abbreviations like "func".
I'd call this function findPlainTextMatchesInRange or something instead of suffixing it with the last argument's type.

> Source/WebCore/editing/TextIterator.cpp:2720
> +        size_t newDistance = std::min(abs((signed)(start - targetOffset)), abs((signed)(start + length - targetOffset)));

Please don't use C-style casing like (signed).

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1829
> +static VisiblePosition visiblePositionForPositionWithOffset(const VisiblePosition& position, int32_t offset)
> +{
> +    SelectionDirection direction = offset < 0 ? DirectionBackward : DirectionForward;
> +    VisiblePosition result = position;
> +    result.setAffinity(DOWNSTREAM);
> +    for (int i = 0; i < abs(offset); ++i) {
> +        result = direction == DirectionForward ? result.next() : result.previous();
> +        if (result.isNull())
> +            break;
> +    }
> +    return result;
> +}

Please use visiblePositionForIndex(root, indexForVisiblePosition(position, root) + offset) instead
where root is commonAncestorContainer() of the range or its rootEditableElement().

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1830
> +    

There's a whitespace here.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1835
> +    

All these functions have whitespace in blank lines. Please remove them all.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1841
> +    VisiblePosition position = visiblePositionForPositionWithOffset(selectionStart, offset);

Use auto.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1844
> +    RefPtr<Range> range = enclosingTextUnitOfGranularity(position, static_cast<WebCore::TextGranularity>(granularity), direction);

Use auto.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1869
> +    RefPtr<Range> selectionRange = frame.selection().selection().toNormalizedRange();

Normalizing Range is expensive. Why don't we call it in inside the if (rangeText != text) below?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1878
> +    VisiblePosition startPosition = visiblePositionForPositionWithOffset(selectionStart, offset);
> +    VisiblePosition endPosition = visiblePositionForPositionWithOffset(startPosition, length);
> +    RefPtr<Range> range = Range::create(*frame.document(), startPosition, endPosition);

Use auto.

> LayoutTests/editing/text-iterator/range-of-string-closest-to-position.html:27
> +    

Please remove these whitespace in blank lines.

[Nan Wang]

Created attachment 300061 [details]
patch

addressed review comments.

[Darin Adler]

Comment on attachment 300061 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300061&action=review

> Source/WebCore/dom/Range.h:128
> +    WEBCORE_EXPORT int collectSelectionRectsWithoutUnionInteriorLines(Vector<SelectionRect>&);

Seems like line number should be unsigned rather than int.

> Source/WebCore/editing/TextIterator.cpp:2681
> +static Ref<Range> findPlainTextMatchesInRange(const Range& range, const String& target, FindOptions options, size_t& matchStart, size_t& matchLength, const std::function<bool(size_t, size_t)>& matches)

I find the design of this function quite peculiar and not straightforward as it should be. The idea that the function’s job is to fill in the two values, matchStart and matchLength, as a side effect, is super-strange. Can we find a more straightforward design pattern?

> Source/WebCore/editing/TextIterator.cpp:2704
> +    auto result = std::pair<size_t, size_t> { matchStart, matchLength };

What’s the point of putting this into a pair, since we just break the pair apart below?

[Nan Wang]

Comment on attachment 300061 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300061&action=review

>> Source/WebCore/dom/Range.h:128
>> +    WEBCORE_EXPORT int collectSelectionRectsWithoutUnionInteriorLines(Vector<SelectionRect>&);
> 
> Seems like line number should be unsigned rather than int.

The SelectionRect::lineNumber() is int and all the comparisons are int within the function. So I'd rather keep it rather than doing the casts.

>> Source/WebCore/editing/TextIterator.cpp:2681
>> +static Ref<Range> findPlainTextMatchesInRange(const Range& range, const String& target, FindOptions options, size_t& matchStart, size_t& matchLength, const std::function<bool(size_t, size_t)>& matches)
> 
> I find the design of this function quite peculiar and not straightforward as it should be. The idea that the function’s job is to fill in the two values, matchStart and matchLength, as a side effect, is super-strange. Can we find a more straightforward design pattern?

Yes, I agree this looks kinda strange. The problem is that based on Ryosuke's previous comment, we want to hold matchStart and matchLength inside the lambda but we have to use the two values once the searching is done, which can only be in the first function that passes in this lambda. I'll try to break this function and make things more straightforward. If it's still not clear, I think I'd revert it to my original design.

[Nan Wang]

Created attachment 300064 [details]
patch

make things more straightforward

[Ryosuke Niwa]

Comment on attachment 300064 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300064&action=review

r=me assuming enrica & darin are happy with this patch as well.

> Source/WebCore/editing/TextIterator.cpp:2675
> +            if (matches(lastCharacterInBufferOffset - matchStartOffset, newMatchLength))

We should probably call this "match" since we usually use a verb for function names.

> Source/WebCore/editing/TextIterator.cpp:2681
> -Ref<Range> findPlainText(const Range& range, const String& target, FindOptions options)
> +static void updateSearchBuffer(SearchBuffer& buffer, const Range& range)

I think it's cleaner if updateSearchBuffer and findIteratorOptions were just merged into findPlainTextMatches
since findClosestPlainText and findPlainText don't need to have access to SearchBuffer at all.
So make findPlainTextMatches take a Range, and options before the adjustment.

> Source/WebCore/editing/TextIterator.cpp:2702
> +static Ref<Range> rangeMatches(const Range& range, TextIteratorBehavior iteratorOptions, size_t matchStart, size_t matchLength, bool searchForward)

We don't have "matches". We have a single match here.
So it's probably more appropriate to call this function rangeForMatch or matchedRange.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1837
> +        send(Messages::WebPageProxy::SelectionRectsCallback(Vector<WebCore::SelectionRect>(), callbackID));

You couldn't just do ({ }, callbackID))?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1846
> +        send(Messages::WebPageProxy::SelectionRectsCallback(Vector<WebCore::SelectionRect>(), callbackID));

Ditto.

[Nan Wang]

Comment on attachment 300064 [details]
patch

Thanks for the review Ryosuke! I'll address those things before committing.

[Nan Wang]

Committed r211356: <http://trac.webkit.org/changeset/211356>