AX: setting focus via accessibility object needs to set isSynchronizing in resulting selection intent

[Doug Russell]

Setting focus on an AccessibilityRenderObject via accessibility API can result in a selection change, which doesn't currently set the isSynchronizing flag on the associated AXTextStateChangeIntent so that the assistive technology that set focus can reliably know that the selection change is the result of the focus call.

[Radar WebKit Bug Importer]

<rdar://problem/20776565>

[Doug Russell]

Created attachment 252138 [details]
patch

[chris fleizach]

Comment on attachment 252138 [details]
patch

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

> Source/WebCore/ChangeLog:10
> +        Call new AXTextStateChangeIntent helper from AccessibilityRenderObject::setFocus().

i think what's missing from this description is why do we need to make these changes

[Doug Russell]

Created attachment 252192 [details]
patch

update changelog

[Darin Adler]

Comment on attachment 252192 [details]
patch

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

> Source/WebCore/accessibility/AXObjectCache.h:202
> -    void setTextSelectionIntent(AXTextStateChangeIntent);
> +    void setTextSelectionIntent(AXTextStateChangeIntent, bool isSynchronizing = false);

When callers are all passing constants to a boolean in a case like this, the WebKit project tradition is to either use two functions (which seems like the right solution in this case) so the names of the functions can make it clear what the difference is, or to use an enum so the names of the constants can make that clear.

Since every single caller uses a boolean, I suggest two separate named functions rather than a function with an optional boolean argument.

I also suggest changing this function to take const AXStateChangeIntent& since that would be slightly more efficient; not sure why this passes by value instead.

[Doug Russell]

Created attachment 252198 [details]
patch

split isSynchronizing into it's own method

[Darin Adler]

Comment on attachment 252198 [details]
patch

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

> Source/WebCore/accessibility/AXObjectCache.h:203
> +    void setIsSynchronizingSelection(bool);

Oh, I think you misunderstood. This second function both set the intent and set “is synchronizing” to true. It just needs a name that makes that clear. Something like this would do:

    void setTextSelectionIntentWhileSynchronizing(const AXTextStateChangeIntent&);

I must admit I don’t understand the concepts here well enough to understand what a good name for this would be.

Your original idea that when setting intent we also want to set m_isSynchronizingSelection each time still probably makes sense and makes the class a bit more foolproof.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1491
> +static void setTextSelectionIntent(const AccessibilityRenderObject* renderObject, bool isRange)

Since the caller is always passing “this” it can never be null, so this should take a reference rather than a pointer and the callers should pass *this.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1669
> +    setTextSelectionIntent(this, false);

The "false" here is a bit mysterious.

[Doug Russell]

Created attachment 252229 [details]
patch

Updates from review.
setTextSelectionIntent was wrong in setSelectedVisiblePositionRange.

[Doug Russell]

Created attachment 252235 [details]
patch

Added test support for setSelectedVisibleTextRange()

[Doug Russell]

Created attachment 252236 [details]
patch

fix gtk builds

[Build Bot]

Comment on attachment 252236 [details]
patch

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

New failing tests:
platform/mac/accessibility/selection-sync.html

[Build Bot]

Created attachment 252238 [details]
Archive of layout-test-results from ews102 for mac-mavericks

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

[Doug Russell]

Created attachment 252248 [details]
patch

fix dump render tree

[Darin Adler]

Comment on attachment 252248 [details]
patch

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

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1915
> +bool AccessibilityUIElement::setSelectedVisibleTextRange(AccessibilityTextMarkerRange*);

The extra semicolon on the end of this line is why GTK and EFL are failing to compile.

[Doug Russell]

Created attachment 252254 [details]
patch

fix selection sync take 2

[Doug Russell]

correction: fix gtk take 2

[chris fleizach]

Comment on attachment 252254 [details]
patch

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

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:912
> +        uiElement->setSelectedVisibleTextRange(textMarkerRange);

we should use the return value of this when making the JSValueMakeBoolean result

[chris fleizach]

(In reply to comment #17)
> Comment on attachment 252254 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252254&action=review
> 
> > Tools/DumpRenderTree/AccessibilityUIElement.cpp:912
> > +        uiElement->setSelectedVisibleTextRange(textMarkerRange);
> 
> we should use the return value of this when making the JSValueMakeBoolean
> result

please upload patch to address my last comment. thanks

[Doug Russell]

Created attachment 252349 [details]
patch

update from review

[WebKit Commit Bot]

Comment on attachment 252349 [details]
patch

Clearing flags on attachment: 252349

Committed r183783: <http://trac.webkit.org/changeset/183783>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.