Bug 206093 – AX: Unable to use AccessibilityObject::replaceTextInRange to insert text at first time when the text fields are empty

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Canhai Chen]

Created attachment 387374 [details]
sample html with text fields

Overview:

Unable to use AccessibilityObject::replaceTextInRange to insert text at first time when the text fields are empty. After the first try, text are able to be inserted. This can be reproduced on text <input>, <textarea> and editable <div>. If there are pretyped text in the text fields, the API works well.

Steps to Reproduce:

1) View any web page with text <input>, <textarea> and editable <div> (attached a text html with these elements).

2) Focus on an empty text field

3) Type (via AccessibilityObject::replaceTextInRange; in our case, we are trying to adopt this API to Braille typing).

Actual Results:

Nothing gets inserted at the first insertion. After that, we are able to insert the text.

Expected Results:

Text get inserted properly even when the text fields are empty.

[Radar WebKit Bug Importer]

<rdar://problem/58491448>

[Canhai Chen]

Created attachment 387722 [details]
Patch

[chris fleizach]

Comment on attachment 387722 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1192
> +    if (!range.start && !range.length && !textLength)

I think I agree that this is an invalid range, but should be also be checking for an invalid range passed into replaceTextInRange?

also is the client that's calling into here doing any value checking before they send the range in?

[chris fleizach]

Comment on attachment 387722 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1193
> +        return nullptr;

I think this can be written as if (range.isNull() && !textLength)

[Canhai Chen]

Comment on attachment 387722 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1192
>> +    if (!range.start && !range.length && !textLength)
> 
> I think I agree that this is an invalid range, but should be also be checking for an invalid range passed into replaceTextInRange?
> 
> also is the client that's calling into here doing any value checking before they send the range in?

Yes, I think another possible fix here is to check the range first in `accessibilityReplaceRange:withText:`, if the range is not (0, 0), we continue to call `AccessibilityObject::replaceTextInRange`, otherwise we call `AccessibilityObject::insertText`. But I think `AccessibilityObject::replaceTextInRange` itself should be able to handle (0, 0) case.

The client is calling here with `accessibilityReplaceRange:withText:` interface, so it does not have knowledge about whether the element is from WebKit or AppKit (although we have ways to try to determine it). And in AppKit, the `accessibilityReplaceRange:withText:` works fine even when the range is (0, 0).

I think the range (0, 0) is "valid" for `accessibilityReplaceRange:withText:` but "invalid" for a `VisibleSelection`. The reason it caused the text replacement failed is because the `WebCore::Range` it creates here will make the `setSelectedRange` create a new `VisibleSelection` whose container node will be used to determine whether the element is editable or not. Most of the cases in text replacement, the container node is an editable `WebCore::Text` element. But the new `VisibleSelection` it creates for (0, 0) here will return the parent node as the container node (could be a `HTMLDivElement` or `HTMLBodyElement`), which makes it uneditable.

Return nullptr if the range is (0, 0) and the text length is 0 here can avoid this, so that when the frame selection is trying to `setSelectedRange` before replacing text, instead of creating an uneditable `VisibleSelection`, it will just return and later in `Editor::replaceSelectionWithText`, it will use the default VisibleSelection with range (0, 0), of which the container node is an editable `TextControlInnerTextElement`.

One workaround for the client right now is to send a "invalid" range like (X, 0) when the range is (0, 0), so that it will fall into the invalid case here and use the correct `VisibleSelection` with `TextControlInnerTextElement` to do the text replacement.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1193
>> +        return nullptr;
> 
> I think this can be written as if (range.isNull() && !textLength)

Thanks for the note, will update in a new patch!

[Canhai Chen]

Created attachment 387877 [details]
Patch

[chris fleizach]

@Ryosuke, can you also take a look at this?

Thanks

[Ryosuke Niwa]

Comment on attachment 387877 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        When we are trying to insert text in an empty text field with (0, 0) range, the frame selection will create a new VisibleSelection in FrameSelection::setSelectedRange, and the container node that this new VisibleSelection returns is the parent node of the text field element, which could be a HTMLDivElement or HTMLBodyElement. Because the container node is not editable, it failed to insert text in Editor::replaceSelectionWithText later.

Can we hard-wrap this line into multiple lines?
This is a really long single line comment.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1193
> +    if (range.isNull() && !textLength)
> +        return nullptr;

Can we check that the backing element for this object is HTMLTextFormControlElement
to minimize the risk of this code change causing other kinds of issues?

[Ryosuke Niwa]

Comment on attachment 387877 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1193
>> +        return nullptr;
> 
> Can we check that the backing element for this object is HTMLTextFormControlElement
> to minimize the risk of this code change causing other kinds of issues?

Also, we should probably add a comment about why we have to do this.
Maybe something like: // Avoid setting selection to outside input element in setSelectionRange. See webkit.org/b/206093.

[Canhai Chen]

Comment on attachment 387877 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        When we are trying to insert text in an empty text field with (0, 0) range, the frame selection will create a new VisibleSelection in FrameSelection::setSelectedRange, and the container node that this new VisibleSelection returns is the parent node of the text field element, which could be a HTMLDivElement or HTMLBodyElement. Because the container node is not editable, it failed to insert text in Editor::replaceSelectionWithText later.
> 
> Can we hard-wrap this line into multiple lines?
> This is a really long single line comment.

Will do. Thanks!

[Canhai Chen]

Comment on attachment 387877 [details]
Patch

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

>>> Source/WebCore/accessibility/AccessibilityObject.cpp:1193
>>> +        return nullptr;
>> 
>> Can we check that the backing element for this object is HTMLTextFormControlElement
>> to minimize the risk of this code change causing other kinds of issues?
> 
> Also, we should probably add a comment about why we have to do this.
> Maybe something like: // Avoid setting selection to outside input element in setSelectionRange. See webkit.org/b/206093.

Will add the comment!

About the element check, this bug is not only reproducible on HTMLTextFormControlElement(<input> and <textarea>) but also on and content-editable <div> (See the added test for an example).

[Canhai Chen]

Created attachment 387978 [details]
Patch

[Canhai Chen]

Updates:

- Wrapped long notes into multiple lines in Changelog.
- Added a comment for the fix.

[WebKit Commit Bot]

Comment on attachment 387978 [details]
Patch

Clearing flags on attachment: 387978

Committed r254782: <https://trac.webkit.org/changeset/254782>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.