| Summary: | AX: WKContentView needs to implement UITextInput methods to make speak selection highlighting work | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Nan Wang <n_wang> | ||||||||||||||||||||
| Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | aboxhall, apinheiro, buildbot, cdumez, cfleizach, commit-queue, dbates, dmazzoni, enrica, esprehn+autocc, jcraig, jdiggs, kangil.han, mario, n_wang, rniwa, samuel_white, thorton, webkit-bug-importer | ||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Nan Wang
2017-01-11 17:46:48 PST
Created attachment 298648 [details]
Initial patch
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.
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. 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
Created attachment 298733 [details]
patch
Build fix
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.
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. Created attachment 299261 [details]
patch
Got rid of the synchronous messages.
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? Created attachment 299684 [details]
patch
- simplified text search logic
- updated from review
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. 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. 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. > 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?
(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. 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. (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. 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... (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. Created attachment 299871 [details]
patch with test
Added test.
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. 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. (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. Created attachment 299952 [details]
patch
Added a new variant of findPlainText.
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. Created attachment 300061 [details]
patch
addressed review comments.
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? 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. Created attachment 300064 [details]
patch
make things more straightforward
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. Comment on attachment 300064 [details]
patch
Thanks for the review Ryosuke! I'll address those things before committing.
Committed r211356: <http://trac.webkit.org/changeset/211356> |