Bug 128026 – AX: Find and select text with respect to insertion point using accessibility API.

• Component: Accessibility
• Status: RESOLVED FIXED

[Samuel White]

We need the ability to find and select a piece of text via the accessibility API. Our initial requirements are:

The ability to specify before, after, or closest to the insertion point.
The ability to pass several strings and have the 'best' match be used.

I think we can support this need using only a single additional parameterized attribute and the following keys:

New parameterized attribute:
@“AXSelectTextWithCriteria"

Initial activity keys and values:
@"AXSelectTextActivity"
@"AXSelectTextActivityFindAndSelect"

Ambiguity resolution keys and values:
@"AXSelectTextAmbiguityResolution"
@"AXSelectTextAmbiguityResolutionClosestAfterSelection"
@"AXSelectTextAmbiguityResolutionClosestBeforeSelection"
@"AXSelectTextAmbiguityResolutionClosestToSelection"

Strings key:
@"AXSelectTextSearchStrings"

[Samuel White]

<rdar://problem/15969891>

[Samuel White]

Created attachment 223044 [details]
Patch for EWS.

Not for review. Doesn't include ChangeLog updates and some refactoring still needs to be done. Just looking for DRT and TestRunner failures on other platforms. Thanks.

[WebKit Commit Bot]

Attachment 223044 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Position.cpp:845:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObject.cpp:523:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObject.cpp:559:  Extra space after ( in if  [whitespace/parens] [5]
ERROR: Source/WebCore/accessibility/AccessibilityObject.cpp:559:  Extra space before ) in if  [whitespace/parens] [5]
ERROR: Source/WebCore/accessibility/AccessibilityObject.cpp:579:  Extra space after ( in if  [whitespace/parens] [5]
ERROR: Source/WebCore/accessibility/AccessibilityObject.cpp:579:  Extra space before ) in if  [whitespace/parens] [5]
ERROR: Source/WebCore/accessibility/AccessibilityObject.h:406:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObject.h:407:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 8 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Samuel White]

Created attachment 223046 [details]
Patch for EWS.

Style fixes. Still not for review.

[Build Bot]

Comment on attachment 223046 [details]
Patch for EWS.

Attachment 223046 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4775164168372224

New failing tests:
platform/mac/accessibility/bounds-for-range.html

[Build Bot]

Created attachment 223069 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 223046 [details]
Patch for EWS.

Attachment 223046 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5063360366247936

New failing tests:
platform/mac/accessibility/bounds-for-range.html

[Build Bot]

Created attachment 223070 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 223046 [details]
Patch for EWS.

Attachment 223046 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6275208340045824

New failing tests:
platform/mac/accessibility/bounds-for-range.html

[Build Bot]

Created attachment 223073 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 223046 [details]
Patch for EWS.

Attachment 223046 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4972425171173376

New failing tests:
platform/mac/accessibility/bounds-for-range.html

[Build Bot]

Created attachment 223091 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Samuel White]

Created attachment 223164 [details]
Patch for EWS.

Fingers crossed this fixes all the build issues (still no ChangeLogs as this is just for EWS).

[WebKit Commit Bot]

Attachment 223164 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1208:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1209:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Tools/DumpRenderTree/win/AccessibilityUIElementWin.cpp:613:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 3 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Samuel White]

Created attachment 223167 [details]
Patch for EWS.

Doh. Thanks a lot TextMate... Fixed spacing issues.

[Samuel White]

Created attachment 223178 [details]
Patch.

Since Win passed ok I'm uploading the final patch with ChangeLogs for review (Hopefully no GTK or EFL issues remain).

[chris fleizach]

Comment on attachment 223178 [details]
Patch.

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

> LayoutTests/platform/mac/accessibility/select-text.html:18
> +		return window.getSelection().toString();

there's all kinds of tabs in this file

> LayoutTests/platform/mac/accessibility/select-text.html:42
> +		// Select text after selection (multiple search strings).

add a newline between each of these "sub-tests" for readability

> Source/WebCore/accessibility/AccessibilityObject.cpp:521
> +    ASSERT(referenceRange);

maybe you should ASSERT that the afterRange > referenceRange and beforeRange < reference, which won't hurt Release performance

> Source/WebCore/accessibility/AccessibilityObject.cpp:608
> +    if ((ambiguityResolution == ClosestAfterSelectionAmbiguityResolution) || (ambiguityResolution == ClosestToSelectionAmbiguityResolution))

extra parens for each term are not necessary here

> Source/WebCore/accessibility/AccessibilityObject.cpp:611
> +    if ((ambiguityResolution == ClosestBeforeSelectionAmbiguityResolution) || (ambiguityResolution == ClosestToSelectionAmbiguityResolution))

ditto about parens

> Source/WebCore/accessibility/AccessibilityObject.h:619
> +    PassRefPtr<Range> rangeClosestToRange(Range*, PassRefPtr<Range>, PassRefPtr<Range>);

you should probably use names for the 2 and 3rd parameters. it's not clear which does what in the header

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:629
> +    // FIXME: Support additional select text activities.

this FIXME is probably not necessary, since it's unclear from the comment what else would need to be supported

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:645
> +        for (size_t i = 0; i < searchStringsCount; ++i) {

Seems like you can use
for (NSString *searchString in searchStringParameter)
instead

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3373
> +        return m_object->selectText(&criteria);

NSAccessibilitySelectTextWithCriteriaParameterizedAttribute behaves more like an action, that returns a value strangely.
Should this be an action instead of an attribute?
I feel like this action then should more naturally return a boolean, indicating whether it succeeded or not.
then if the user wanted to get the selected text, they would make a new query for SelectedTextAttribute

> Source/WebCore/dom/Position.cpp:830
> +unsigned Position::positionsBetweenPositions(const Position& a, const Position& b)

should this be named lengthBetweenPositions()

> Source/WebCore/dom/Position.cpp:847
> +    while (!pos.atEndOfTree() && (pos != endPos)) {

parens not necessary for second term

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:272
> +        if (JSValueIsString(context, searchStrings)) {

it might simplify life to only support arrays being passed in here. the single values would just be arrays of size = 1

[Samuel White]

Created attachment 223248 [details]
Updated patch.

[Samuel White]

(In reply to comment #17)
> (From update of attachment 223178 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223178&action=review
> 
> > LayoutTests/platform/mac/accessibility/select-text.html:18
> > +		return window.getSelection().toString();
> 
> there's all kinds of tabs in this file

Sorry about that. Fixed.

> 
> > LayoutTests/platform/mac/accessibility/select-text.html:42
> > +		// Select text after selection (multiple search strings).
> 
> add a newline between each of these "sub-tests" for readability

Done.

> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:521
> > +    ASSERT(referenceRange);
> 
> maybe you should ASSERT that the afterRange > referenceRange and beforeRange < reference, which won't hurt Release performance

Good thought. Done.

> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:608
> > +    if ((ambiguityResolution == ClosestAfterSelectionAmbiguityResolution) || (ambiguityResolution == ClosestToSelectionAmbiguityResolution))
> 
> extra parens for each term are not necessary here

I have a habit of including extras when I think it helps readability. Removed as requested.

> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:611
> > +    if ((ambiguityResolution == ClosestBeforeSelectionAmbiguityResolution) || (ambiguityResolution == ClosestToSelectionAmbiguityResolution))
> 
> ditto about parens

Done.

> 
> > Source/WebCore/accessibility/AccessibilityObject.h:619
> > +    PassRefPtr<Range> rangeClosestToRange(Range*, PassRefPtr<Range>, PassRefPtr<Range>);
> 
> you should probably use names for the 2 and 3rd parameters. it's not clear which does what in the header

I made this method a static function instead and removed it from the header. I can't see any use for it outside of this stack and it doesn't need member access. Seems a better fit. Thanks for pointing out the confusion.

> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:629
> > +    // FIXME: Support additional select text activities.
> 
> this FIXME is probably not necessary, since it's unclear from the comment what else would need to be supported

Removed.

> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:645
> > +        for (size_t i = 0; i < searchStringsCount; ++i) {
> 
> Seems like you can use
> for (NSString *searchString in searchStringParameter)
> instead

And it's one less line of code. Done.

> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3373
> > +        return m_object->selectText(&criteria);
> 
> NSAccessibilitySelectTextWithCriteriaParameterizedAttribute behaves more like an action, that returns a value strangely.
> Should this be an action instead of an attribute?
> I feel like this action then should more naturally return a boolean, indicating whether it succeeded or not.
> then if the user wanted to get the selected text, they would make a new query for SelectedTextAttribute
> 

Replacement will be added in the future. This path (parameterized attribute) will allow us to handle very tricky cases where we half succeed. For example, we may want to find and select one of an array of strings per the request, but replacement may not be possible because the text may cross sensitive boundaries. In this future, we would return a dictionary that specified the text that was found and selected and the failure to replace given boundary constraints.

> > Source/WebCore/dom/Position.cpp:830
> > +unsigned Position::positionsBetweenPositions(const Position& a, const Position& b)
> 
> should this be named lengthBetweenPositions()
> 
> > Source/WebCore/dom/Position.cpp:847
> > +    while (!pos.atEndOfTree() && (pos != endPos)) {
> 
> parens not necessary for second term

Fixed.

> 
> > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:272
> > +        if (JSValueIsString(context, searchStrings)) {
> 
> it might simplify life to only support arrays being passed in here. the single values would just be arrays of size = 1

Agreed. However, I think this makes our .js testing facilities more robust and we do something similar for our search testing mechanism. Also, it only adds four lines. As always, I can pull it if you feel strongly.

Thanks for the feedback!

[chris fleizach]

Comment on attachment 223248 [details]
Updated patch.

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

> Source/WebCore/dom/Position.cpp:830
> +unsigned Position::positionsBetweenPositions(const Position& a, const Position& b)

any comments about the name of this method?

[Samuel White]

(In reply to comment #20)
> (From update of attachment 223248 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223248&action=review
> 
> > Source/WebCore/dom/Position.cpp:830
> > +unsigned Position::positionsBetweenPositions(const Position& a, const Position& b)
> 
> any comments about the name of this method?

Oops, yes. I thought it important to be very specific here to avoid confusing others who discover this method down the road. We have many concepts for distance in this area of the codebase, offset, position, visible position, text iterator position, etc. IMO this name helps disambiguate the intention of this method in a way that this least likely to be misunderstood. Thoughts?

[chris fleizach]

Comment on attachment 223248 [details]
Updated patch.

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

>>> Source/WebCore/dom/Position.cpp:830
>>> +unsigned Position::positionsBetweenPositions(const Position& a, const Position& b)
>> 
>> any comments about the name of this method?
> 
> Oops, yes. I thought it important to be very specific here to avoid confusing others who discover this method down the road. We have many concepts for distance in this area of the codebase, offset, position, visible position, text iterator position, etc. IMO this name helps disambiguate the intention of this method in a way that this least likely to be misunderstood. Thoughts?

maybe it should be positionCountBetweenPositions

[Samuel White]

(In reply to comment #22)
> (From update of attachment 223248 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223248&action=review
> 
> >>> Source/WebCore/dom/Position.cpp:830
> >>> +unsigned Position::positionsBetweenPositions(const Position& a, const Position& b)
> >> 
> >> any comments about the name of this method?
> > 
> > Oops, yes. I thought it important to be very specific here to avoid confusing others who discover this method down the road. We have many concepts for distance in this area of the codebase, offset, position, visible position, text iterator position, etc. IMO this name helps disambiguate the intention of this method in a way that this least likely to be misunderstood. Thoughts?
> 
> maybe it should be positionCountBetweenPositions

Sounds good. Will upload a new patch once I've finished investigating the elf-wk2 build issue.

[chris fleizach]

(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 223248 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=223248&action=review
> > 
> > >>> Source/WebCore/dom/Position.cpp:830
> > >>> +unsigned Position::positionsBetweenPositions(const Position& a, const Position& b)
> > >> 
> > >> any comments about the name of this method?
> > > 
> > > Oops, yes. I thought it important to be very specific here to avoid confusing others who discover this method down the road. We have many concepts for distance in this area of the codebase, offset, position, visible position, text iterator position, etc. IMO this name helps disambiguate the intention of this method in a way that this least likely to be misunderstood. Thoughts?
> > 
> > maybe it should be positionCountBetweenPositions
> 
> Sounds good. Will upload a new patch once I've finished investigating the elf-wk2 build issue.

EFL seems unrelated

CMake Error at Source/WebKit2/CMakeLists.txt:709 (add_library):
  Cannot find source file:

    UIProcess/WebUIClient.cpp

[Samuel White]

Created attachment 223387 [details]
Patch for review.

Updated per review. Thanks.

[WebKit Commit Bot]

Comment on attachment 223387 [details]
Patch for review.

Clearing flags on attachment: 223387

Committed r163632: <http://trac.webkit.org/changeset/163632>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Fixed 32-bit build in <http://trac.webkit.org/projects/webkit/changeset/163634>.

[Samuel White]

(In reply to comment #28)
> Fixed 32-bit build in <http://trac.webkit.org/projects/webkit/changeset/163634>.

Thanks for handling that Alexey. I didn't realize it would be an issue.