[Webkit-unassigned] [Bug 261056] AX: Expose accessibility attributes for inline text predictions

[bugzilla-daemon at webkit.org]

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

Tyler Wilcock <tyler_w at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tyler_w at apple.com

--- Comment #4 from Tyler Wilcock <tyler_w at apple.com> ---
Comment on attachment 467522
  --> https://bugs.webkit.org/attachment.cgi?id=467522
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20230902/69be75fc/attachment.htm>