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"
<rdar://problem/15969891>
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.
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.
Created attachment 223046 [details] Patch for EWS. Style fixes. Still not for review.
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
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
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
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
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
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
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
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
Created attachment 223164 [details] Patch for EWS. Fingers crossed this fixes all the build issues (still no ChangeLogs as this is just for EWS).
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.
Created attachment 223167 [details] Patch for EWS. Doh. Thanks a lot TextMate... Fixed spacing issues.
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).
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
Created attachment 223248 [details] Updated patch.
(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!
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?
(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?
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
(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.
(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
Created attachment 223387 [details] Patch for review. Updated per review. Thanks.
Comment on attachment 223387 [details] Patch for review. Clearing flags on attachment: 223387 Committed r163632: <http://trac.webkit.org/changeset/163632>
All reviewed patches have been landed. Closing bug.
Fixed 32-bit build in <http://trac.webkit.org/projects/webkit/changeset/163634>.
(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.