Bug 261056

<rdar://problem/114851958>

Created attachment 467522 [details] Patch

Comment on attachment 467522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=467522&action=review > Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:218 > + unsigned lastPresentedCompleteWordLength = [lastPresentedCompleteWord.first length]; Since lastPresentedCompleteWord and everything in it are C++ types, I think it may be better style to avoid bracket calling syntax. > Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:225 > + if (!lastPresentedCompleteWord.first.isEmpty() && lastPresentedCompleteWordPosition + lastPresentedCompleteWordLength <= [attrString length]) { > + NSRange completeWordRange = NSMakeRange(lastPresentedCompleteWordPosition, lastPresentedCompleteWordLength); > + if ([[attrString.string substringWithRange:completeWordRange] isEqualToString:lastPresentedCompleteWord.first]) > + [attrString addAttribute:UIAccessibilityAcceptedInlineTextCompletion value:lastPresentedCompleteWord.first range:completeWordRange]; > + } Will having a nil attrString cause any logic issues in this block? If so, maybe we can add a bail condition above. > Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:244 > +#endif #endifs that are reasonably large (like this one) should have a comment indicating the branch they're closing. So should be this: #endif // HAVE(INLINE_PREDICTIONS) > Source/WebCore/editing/Editor.cpp:2344 > + // Assistive technologies need to know the position & text of the last accepted completion to expose it properly. I think WebKit comment style prefers spelling out "and" vs. using &. Also, this comment could be re-worded a bit, since it implies assistive technologies are the ones exposing this information, but really it's WebKit. > Source/WebCore/editing/Editor.cpp:2345 > + if (AXObjectCache::accessibilityEnabled() && state == AXObjectCache::CompositionState::Ended && !m_lastPresentedTextPrediction.first.isEmpty()) { I don't think we need to check AXObjectCache::accessibilityEnabled() here. If we've got this far, that means Document::existingAXObjectCache() returned non-null, which must mean that accessibility is enabled. > Source/WebCore/editing/Editor.cpp:2347 > + size_t wordStart = previousCompositionNodeText.contains(" "_s) ? previousCompositionNodeText.reverseFind(" "_s) + 1 : 0; Do we need to check for other types of whitespace like line breaks or tabs? Or is it OK to only check for spaces? > Source/WebCore/editing/Editor.cpp:2351 > + Vector<String> strings = { previousCompositionNodeText, m_lastPresentedTextPrediction.first }; > + String completePredictedWord = makeStringByJoining(strings, ""_s); Is the purpose of this makeStringByJoining and Vector just to combine these two strings? If so, I think you can do it much more simply like this: String completePredictedWord = makeString(previousCompositionNodeText, m_lastPresentedTextPrediction.first); > Source/WebCore/editing/Editor.cpp:2354 > + // Reset last presented prrediction since a candidate was accepted. Typo on prrediction here. > Source/WebCore/editing/Editor.cpp:2361 > + m_lastPresentedTextPredictionComplete = std::make_pair(""_s, 0); If you make m_lastPresentedTextPredictionComplete a struct, you could have a reset() method that documents the intention better here and above. > Source/WebCore/editing/Editor.h:603 > + std::pair<String, unsigned> lastPresentedTextPrediction() const { return m_lastPresentedTextPrediction; } > + std::pair<String, unsigned> lastPresentedTextPredictionComplete() const { return m_lastPresentedTextPredictionComplete; } It might be better to use a struct rather than a pair since you can name the members of a struct. Based on the name of the variable, it's reasonable to guess that the String part is the text prediction. But it's not as clear what the unsigned refers to. You can declare the struct right above these two function declarations.

Comment on attachment 467522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=467522&action=review > Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:217 > + std::pair<String, unsigned> lastPresentedCompleteWord = editor.lastPresentedTextPredictionComplete(); auto lastPresentedCompleteWord > Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:227 > + if (&node != editor.compositionNode()) we should check this at the top so we don't do this other work first > Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:230 > + std::pair<String, unsigned> lastPresentedTextPrediction = editor.lastPresentedTextPrediction(); auto

Comment on attachment 467522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=467522&action=review >> Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:227 >> + if (&node != editor.compositionNode()) > > we should check this at the top so we don't do this other work first We need to do this check after getting the complete word because the composition node that stores the last word will not be equal to the current node (since the prediction is complete). >> Source/WebCore/editing/Editor.h:603 >> + std::pair<String, unsigned> lastPresentedTextPredictionComplete() const { return m_lastPresentedTextPredictionComplete; } > > It might be better to use a struct rather than a pair since you can name the members of a struct. Based on the name of the variable, it's reasonable to guess that the String part is the text prediction. But it's not as clear what the unsigned refers to. You can declare the struct right above these two function declarations. I like the idea of a struct—will switch to that!

Created attachment 467565 [details] Patch

Comment on attachment 467565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=467565&action=review Thanks for getting a test working! > Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:212 > +static void attributedStringSetCompositionAttributes(NSMutableAttributedString *attrString, Node& node) I know we use the attrString abbreviation everywhere, but since this is new code let's follow the style guide and spell it out as attributedString. > Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:231 > + if (!lastPresentedCompleteWord.text.isEmpty() && lastPresentedCompleteWordPosition + lastPresentedCompleteWordLength <= [attrString length]) { > + NSRange completeWordRange = NSMakeRange(lastPresentedCompleteWordPosition, lastPresentedCompleteWordLength); > + if ([[attrString.string substringWithRange:completeWordRange] isEqualToString:lastPresentedCompleteWord.text]) > + [attrString addAttribute:UIAccessibilityAcceptedInlineTextCompletion value:lastPresentedCompleteWord.text range:completeWordRange]; > + } > + > + if (&node != editor.compositionNode()) > + return; Would it make sense to move this early-return check before doing the work on this block (maybe with the attributedString nil check)? Seems like the result isn't used if &node != editor.compositionNode(). > Source/WebCore/editing/Editor.cpp:2348 > + for (size_t pos = previousCompositionNodeText.length() - 1; pos > 0; pos--) { This should probably be position instead of pos. Also: size_t pos = previousCompositionNodeText.length() - 1 if previousCompositionNodeText.length() is zero, subtracting one from it (an unsigned value) will wrap to the max unsigned value, causing pos to be very big, in turn causing an out-of-bounds access on previousCompositionNodeText[pos]. Is it guaranteed that this length will not be zero? Or can we make this safer somehow? > Source/WebCore/editing/Editor.h:602 > + struct InlineTextPrediction { Is it OK to just call this TextPrediction? Inline feels unnecessarily specific, but you know this code better, so feel free to leave it if you feel it makes sense. > Source/WebCore/editing/Editor.h:604 > + String text; > + size_t location; Let's default initialize this size_t to zero just to be safe. We don't need to default initialize WTFStrings. size_t location { 0 }; > Source/WebCore/editing/Editor.h:605 > + void reset() { text = ""_s; location = 0; } WebKit coding style (https://webkit.org/code-style-guidelines/) recommends one statement per-line. So maybe something like this: void reset() { text = ""_s; location = 0; } > LayoutTests/accessibility/ios-simulator/inline-prediction-attributed-string.html:2 > + Extra newline here I think. > LayoutTests/accessibility/ios-simulator/inline-prediction-attributed-string.html:9 > +<body id="body"> Is this id="body" necessary? > LayoutTests/accessibility/ios-simulator/inline-prediction-attributed-string.html:11 > + > + One extra newline here I think. > LayoutTests/accessibility/ios-simulator/inline-prediction-attributed-string.html:20 > + var obj = accessibilityController.focusedElement; > + obj.insertText("Good mo") Calling this `input` (or something else) instead of `obj` might read better.

Comment on attachment 467565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=467565&action=review >> Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:231 >> + return; > > Would it make sense to move this early-return check before doing the work on this block (maybe with the attributedString nil check)? Seems like the result isn't used if &node != editor.compositionNode(). When we are getting a completed word, the previous composition will have ended, so the node we are looking at will not be equal to the compositionNode of the editor. We need them to be equal only when parsing presented (and incomplete) completions. >> Source/WebCore/editing/Editor.cpp:2348 >> + for (size_t pos = previousCompositionNodeText.length() - 1; pos > 0; pos--) { > > This should probably be position instead of pos. > > Also: > size_t pos = previousCompositionNodeText.length() - 1 > > if previousCompositionNodeText.length() is zero, subtracting one from it (an unsigned value) will wrap to the max unsigned value, causing pos to be very big, in turn causing an out-of-bounds access on previousCompositionNodeText[pos]. > > Is it guaranteed that this length will not be zero? Or can we make this safer somehow? I can guard against the length of that text earlier.

Created attachment 467570 [details] Patch

(In reply to Joshua Hoffman from comment #10) > Created attachment 467570 [details] > Patch --- a/Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm +++ b/Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm +static void attributedStringSetCompositionAttributes(NSMutableAttributedString *attributedString, Node& node) Does this need to be different from the same function in AccessibilityObjectMac.mm? Or can we have a common COCOA function? + if (&node != editor.compositionNode()) + return; Seems like this check should be earlier in the function, same as in the MAC version. --- a/Source/WebCore/editing/Editor.cpp +++ b/Source/WebCore/editing/Editor.cpp cache->onTextCompositionChange(*previousCompositionNode, state, true); + + // WebKit needs to store inline-predicted words to expose their position and text values to assistive technologies. + if (state == AXObjectCache::CompositionState::Ended && !m_lastPresentedTextPrediction.text.isEmpty()) { + String previousCompositionNodeText = previousCompositionNode->wholeText(); + size_t wordStart = 0; + if (previousCompositionNodeText.length()) { + for (size_t position = previousCompositionNodeText.length() - 1; position > 0; position--) { + if (isASCIIWhitespace(previousCompositionNodeText[position])) { + wordStart = position + 1; + break; + } + } + } + previousCompositionNodeText = previousCompositionNodeText.substring(wordStart); + + String completePredictedWord = makeString(previousCompositionNodeText, m_lastPresentedTextPrediction.text); + m_lastPresentedTextPredictionComplete = { completePredictedWord, wordStart }; + + // Reset last presented prediction since a candidate was accepted. + m_lastPresentedTextPrediction.reset(); + } } if (m_compositionNode) { auto state = previousCompositionNode ? AXObjectCache::CompositionState::InProgress : AXObjectCache::CompositionState::Started; cache->onTextCompositionChange(*m_compositionNode, state, true); + m_lastPresentedTextPredictionComplete.reset(); Instead of keeping these variables in the Editor, I wondering if it would make more sense to pass them as context to AXObjectCache::onTextCompositionChange.

(In reply to Andres Gonzalez from comment #11) > (In reply to Joshua Hoffman from comment #10) > > Created attachment 467570 [details] > > Patch > > --- a/Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm > +++ b/Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm > > +static void > attributedStringSetCompositionAttributes(NSMutableAttributedString > *attributedString, Node& node) > > Does this need to be different from the same function in > AccessibilityObjectMac.mm? Or can we have a common COCOA function? We need to have these stay separate functions, since we expose different attributes for each platform. > + if (&node != editor.compositionNode()) > + return; > > Seems like this check should be earlier in the function, same as in the MAC > version. When accessing completed words, the previous composition will have ended, so the node we are looking at will not be equal to the compositionNode of the editor. We need them to be equal only when parsing presented (and incomplete) completions. > --- a/Source/WebCore/editing/Editor.cpp > +++ b/Source/WebCore/editing/Editor.cpp > > cache->onTextCompositionChange(*previousCompositionNode, state, > true); > + > + // WebKit needs to store inline-predicted words to expose their > position and text values to assistive technologies. > + if (state == AXObjectCache::CompositionState::Ended && > !m_lastPresentedTextPrediction.text.isEmpty()) { > + String previousCompositionNodeText = > previousCompositionNode->wholeText(); > + size_t wordStart = 0; > + if (previousCompositionNodeText.length()) { > + for (size_t position = > previousCompositionNodeText.length() - 1; position > 0; position--) { > + if > (isASCIIWhitespace(previousCompositionNodeText[position])) { > + wordStart = position + 1; > + break; > + } > + } > + } > + previousCompositionNodeText = > previousCompositionNodeText.substring(wordStart); > + > + String completePredictedWord = > makeString(previousCompositionNodeText, m_lastPresentedTextPrediction.text); > + m_lastPresentedTextPredictionComplete = { > completePredictedWord, wordStart }; > + > + // Reset last presented prediction since a candidate was > accepted. > + m_lastPresentedTextPrediction.reset(); > + } > } > if (m_compositionNode) { > auto state = previousCompositionNode ? > AXObjectCache::CompositionState::InProgress : > AXObjectCache::CompositionState::Started; > cache->onTextCompositionChange(*m_compositionNode, state, true); > + m_lastPresentedTextPredictionComplete.reset(); > > Instead of keeping these variables in the Editor, I wondering if it would > make more sense to pass them as context to > AXObjectCache::onTextCompositionChange. I have debated this as well. Ultimately, I believe maintaining this state here will make more sense so that the state remains up-to-date, and we don't have to worry about updating the cached properties. Happy to discuss further, however.

(In reply to Joshua Hoffman from comment #10) > Created attachment 467570 [details] > Patch --- a/Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm +++ b/Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm +static void attributedStringSetCompositionAttributes(NSMutableAttributedString *attributedString, Node& node) +{ +#if HAVE(INLINE_PREDICTIONS) + auto& editor = node.document().editor(); + + auto lastPresentedCompleteWord = editor.lastPresentedTextPredictionComplete(); + unsigned lastPresentedCompleteWordLength = lastPresentedCompleteWord.text.length(); + unsigned lastPresentedCompleteWordPosition = lastPresentedCompleteWord.location; + + if (!attributedString) + return; Should we move this param validation / early return to the beginning of the function?

(In reply to Joshua Hoffman from comment #12) > (In reply to Andres Gonzalez from comment #11) > > (In reply to Joshua Hoffman from comment #10) > > > Created attachment 467570 [details] > > > Patch > > > > --- a/Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm > > +++ b/Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm > > > > +static void > > attributedStringSetCompositionAttributes(NSMutableAttributedString > > *attributedString, Node& node) > > > > Does this need to be different from the same function in > > AccessibilityObjectMac.mm? Or can we have a common COCOA function? > We need to have these stay separate functions, since we expose different > attributes for each platform. > > > + if (&node != editor.compositionNode()) > > + return; > > > > Seems like this check should be earlier in the function, same as in the MAC > > version. > When accessing completed words, the previous composition will have ended, so > the node we are looking at will not be equal to the compositionNode of the > editor. We need them to be equal only when parsing presented (and > incomplete) completions. > > > --- a/Source/WebCore/editing/Editor.cpp > > +++ b/Source/WebCore/editing/Editor.cpp > > > > cache->onTextCompositionChange(*previousCompositionNode, state, > > true); > > + > > + // WebKit needs to store inline-predicted words to expose their > > position and text values to assistive technologies. > > + if (state == AXObjectCache::CompositionState::Ended && > > !m_lastPresentedTextPrediction.text.isEmpty()) { > > + String previousCompositionNodeText = > > previousCompositionNode->wholeText(); > > + size_t wordStart = 0; > > + if (previousCompositionNodeText.length()) { > > + for (size_t position = > > previousCompositionNodeText.length() - 1; position > 0; position--) { > > + if > > (isASCIIWhitespace(previousCompositionNodeText[position])) { > > + wordStart = position + 1; > > + break; > > + } > > + } > > + } > > + previousCompositionNodeText = > > previousCompositionNodeText.substring(wordStart); > > + > > + String completePredictedWord = > > makeString(previousCompositionNodeText, m_lastPresentedTextPrediction.text); > > + m_lastPresentedTextPredictionComplete = { > > completePredictedWord, wordStart }; > > + > > + // Reset last presented prediction since a candidate was > > accepted. > > + m_lastPresentedTextPrediction.reset(); > > + } > > } > > if (m_compositionNode) { > > auto state = previousCompositionNode ? > > AXObjectCache::CompositionState::InProgress : > > AXObjectCache::CompositionState::Started; > > cache->onTextCompositionChange(*m_compositionNode, state, true); > > + m_lastPresentedTextPredictionComplete.reset(); > > > > Instead of keeping these variables in the Editor, I wondering if it would > > make more sense to pass them as context to > > AXObjectCache::onTextCompositionChange. > > I have debated this as well. Ultimately, I believe maintaining this state > here will make more sense so that the state remains up-to-date, and we don't > have to worry about updating the cached properties. Happy to discuss > further, however. If this is used only by accessibility, I think we should pass the relevant state to onTextCompositionChange and there we do the processing of the notification. The necessary state can be kept in the AXObjectCache or maybe better, in the corresponding AX object.

Created attachment 467592 [details] Patch

Created attachment 467595 [details] Patch

Created attachment 467596 [details] Patch

Comment on attachment 467596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=467596&action=review > Source/WebCore/accessibility/AccessibilityObject.h:782 > +#endif This block is probably long enough to warrant a // PLATFORM(IOS) after the #endif.

(In reply to Joshua Hoffman from comment #17) > Created attachment 467596 [details] > Patch --- a/Source/WebCore/accessibility/AccessibilityObject.cpp +++ b/Source/WebCore/accessibility/AccessibilityObject.cpp +#if PLATFORM(IOS) +void AccessibilityObject::setLastPresentedTextPrediction(Node& previousCompositionNode, CompositionState state, String text, size_t location, bool handlingAcceptedCandidate) This should be in AccessibilityObjectIOS.mm instead. --- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h +++ b/Source/WebCore/accessibility/AccessibilityObjectInterface.h +enum class CompositionState : uint8_t { Started, InProgress, Ended }; Could we have Accepted as another CompositionState? As opposed to a separate state/variable/param?

Created attachment 467641 [details] Patch

Created attachment 467646 [details] Patch

(In reply to Joshua Hoffman from comment #21) > Created attachment 467646 [details] > Patch --- a/Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm +++ b/Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm + // Find the location of the the complete word being predicted by iterating backwards through the text to find whitespace. Typo: the the + previousCompositionNodeText = previousCompositionNodeText.substring(wordStart); If wordStart is 0, we are unnecessarily copying previousCompositionNodeText. Can we avoid this copy? +static void attributedStringSetCompositionAttributes(NSMutableAttributedString *attributedString, RenderObject* renderer) +{ +#if HAVE(INLINE_PREDICTIONS) + if (!renderer) + return; + + RefPtr object = renderer->document().axObjectCache()->getOrCreate(renderer); + + auto lastPresentedCompleteWord = object->lastPresentedTextPredictionComplete(); Check object for null before using it? Also, we are copying the strings unnecessarily. You may want to make the inline method lastPresentedTextPredictionComplete() to return a reference. + auto lastPresentedTextPrediction = object->lastPresentedTextPrediction(); Same comment applies here to lastPresentedTextPrediction(). --- /dev/null +++ b/LayoutTests/accessibility/ios-simulator/inline-prediction-attributed-string.html + editable.focus(); editable.xxx --> document.getElementById("editable").xxx instead. + input.insertText("Good mo") Missing ;

Created attachment 467666 [details] Patch

(In reply to Joshua Hoffman from comment #23) > Created attachment 467666 [details] > Patch --- a/Source/WebCore/accessibility/AXObjectCache.h +++ b/Source/WebCore/accessibility/AXObjectCache.h - void onTextCompositionChange(Node&, CompositionState, bool); + void onTextCompositionChange(Node&, CompositionState, bool, String, size_t, bool); String parameter should be const String&. --- a/Source/WebCore/accessibility/AccessibilityObject.h +++ b/Source/WebCore/accessibility/AccessibilityObject.h + void setLastPresentedTextPrediction(Node&, CompositionState, String, size_t, bool); Same here. --- a/Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm +++ b/Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm + String completePredictedWord = makeString(previousCompositionNodeText, m_lastPresentedTextPrediction.text); + m_lastPresentedTextPredictionComplete = { completePredictedWord, wordStart }; You can use the + operator to concatenate WTF::Strings instead of makeString. Also, I don't think you need the completePredictedWord temp, which may prevent the compiler from moving the String into the struct.

(In reply to Andres Gonzalez from comment #24) > You can use the + operator to concatenate WTF::Strings instead of > makeString. Also, I don't think you need the completePredictedWord temp, > which may prevent the compiler from moving the String into the struct. That's a good call out—I'll remove that.

Created attachment 467675 [details] Patch

Committed 267990@main (4d7674e3b99d): <https://commits.webkit.org/267990@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 467675 [details].