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

Wed Sep 6 00:26:01 PDT 2023

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

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

-- 
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/20230906/9faf442b/attachment.htm>