Make AXCoreObject::setSelectedVisiblePositionRange work in native text controls on MacOS.
Created attachment 436441 [details] Patch
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>
Created attachment 436542 [details] Patch
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
(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!
(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.
(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.
<rdar://problem/82416769>
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].