Bug 229529 – Make AXCoreObject::setSelectedVisiblePositionRange work in native text controls on MacOS.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andres Gonzalez]

Make AXCoreObject::setSelectedVisiblePositionRange work in native text controls on MacOS.

[Andres Gonzalez]

Created attachment 436441 [details]
Patch

[chris fleizach]

Comment on attachment 436441 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2294
> +    if (isNativeTextControl()) {

is isNativeTextControl check identical to downcast<RenderTextControl>.

Should we use is<RenderTextControl>(m_renderr) instead?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2300
> +        // the above indexs will be 0, leading to an incorrect selected range

indexs > indexes

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2304
> +        // Thus, the following corrects the start and end indexes in such scenario.

such "a" scenario

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2307
> +            if (range.start < innerRange.start)

is there ever a case where we would expect the range coming into setSelectedVisiblePosition to WANT to select beyond the range of the text field?

In otherwords, should we make this change in the range we return for a text field element?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2598
> +        return (id)Accessibility::retrieveAutoreleasedValueFromMainThread<AXTextMarkerRangeRef>([protectedSelf = retainPtr(self)] () -> RetainPtr<AXTextMarkerRangeRef> {

this feels like a step backwards to remove the function and inline more code into this long method

> LayoutTests/accessibility/mac/textarea-set-selected-textmarker-range.html:9
> +<textarea id="textarea">This is a textarea.</textarea>

should we also check a <input type=text> and a <input type=password>

[Andres Gonzalez]

Created attachment 436542 [details]
Patch

[chris fleizach]

Comment on attachment 436542 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2307
> +        if (range.start.deepEquivalent().anchorNode() == range.end.deepEquivalent().anchorNode()

we should cache range.start.deepEquivalent().anchorNode() if possible since we use it twice

[Andres Gonzalez]

(In reply to chris fleizach from comment #2)
> Comment on attachment 436441 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436441&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2294
> > +    if (isNativeTextControl()) {
> 
> is isNativeTextControl check identical to downcast<RenderTextControl>.
> 
> Should we use is<RenderTextControl>(m_renderr) instead?

I changed it to downcast<HTmLTextFormControlElement> which is more clear it is safe if isNativeTextControl() returns true.

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2300
> > +        // the above indexs will be 0, leading to an incorrect selected range
> 
> indexs > indexes

Fixed.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2304
> > +        // Thus, the following corrects the start and end indexes in such scenario.
> 
> such "a" scenario

Fixed.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2307
> > +            if (range.start < innerRange.start)
> 
> is there ever a case where we would expect the range coming into
> setSelectedVisiblePosition to WANT to select beyond the range of the text
> field?
> 
> In otherwords, should we make this change in the range we return for a text
> field element?

Good point. that was my initial attempt, but that creates a new array of problems because the range we would return for the object is such that the element obtained from range.start doesn't match the original object, and furthermore it can be an object that is not exposed in the AX tree.

So the alternative in this revision is to ensure that the range is contained in this object before trying to do this. The check in question is:

        if (range.start.deepEquivalent().anchorNode() == range.end.deepEquivalent().anchorNode()
            && range.start.deepEquivalent().anchorNode() == textControl) {

> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2598
> > +        return (id)Accessibility::retrieveAutoreleasedValueFromMainThread<AXTextMarkerRangeRef>([protectedSelf = retainPtr(self)] () -> RetainPtr<AXTextMarkerRangeRef> {
> 
> this feels like a step backwards to remove the function and inline more code
> into this long method

OK, put it back but renamed it to selectedTextMarkerRange for naming consistency.

> 
> > LayoutTests/accessibility/mac/textarea-set-selected-textmarker-range.html:9
> > +<textarea id="textarea">This is a textarea.</textarea>
> 
> should we also check a <input type=text> and a <input type=password>

Added <input type="text"> test case. Password fields are left out at this time because it has a whole host of other problems with ranges. Will have to fix password fields in a separate patch.
Thanks!

[Andres Gonzalez]

(In reply to chris fleizach from comment #4)
> Comment on attachment 436542 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436542&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2307
> > +        if (range.start.deepEquivalent().anchorNode() == range.end.deepEquivalent().anchorNode()
> 
> we should cache range.start.deepEquivalent().anchorNode() if possible since
> we use it twice

Is that for clarity/style? I believe these are all inline accessors to member variables, so it should have little to no perf impact.

[Andres Gonzalez]

(In reply to chris fleizach from comment #4)
> Comment on attachment 436542 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436542&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2307
> > +        if (range.start.deepEquivalent().anchorNode() == range.end.deepEquivalent().anchorNode()
> 
> we should cache range.start.deepEquivalent().anchorNode() if possible since
> we use it twice

I think it is more expensive to cache in this case because it will mean a copy, while this way we are essentially just accessing the member variable directly since deepEquivalent and anchorNode are both inline accessors.

[Radar WebKit Bug Importer]

<rdar://problem/82416769>

[EWS]

Committed r281691 (241041@main): <https://commits.webkit.org/241041@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436542 [details].