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.
Created attachment 252138 [details]
Comment on attachment 252138 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=252138&action=review
> + Call new AXTextStateChangeIntent helper from AccessibilityRenderObject::setFocus().
i think what's missing from this description is why do we need to make these changes
Created attachment 252192 [details]
Comment on attachment 252192 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=252192&action=review
> - 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.
Created attachment 252198 [details]
split isSynchronizing into it's own method
Comment on attachment 252198 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=252198&action=review
> + 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.
> +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.
> + setTextSelectionIntent(this, false);
The "false" here is a bit mysterious.
Created attachment 252229 [details]
Updates from review.
setTextSelectionIntent was wrong in setSelectedVisiblePositionRange.
Created attachment 252235 [details]
Added test support for setSelectedVisibleTextRange()
Created attachment 252236 [details]
fix gtk builds
Comment on attachment 252236 [details]
Attachment 252236 [details] did not pass mac-ews (mac):
New failing tests:
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
Created attachment 252248 [details]
fix dump render tree
Comment on attachment 252248 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=252248&action=review
> +bool AccessibilityUIElement::setSelectedVisibleTextRange(AccessibilityTextMarkerRange*);
The extra semicolon on the end of this line is why GTK and EFL are failing to compile.
Created attachment 252254 [details]
fix selection sync take 2
correction: fix gtk take 2
Comment on attachment 252254 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=252254&action=review
> + uiElement->setSelectedVisibleTextRange(textMarkerRange);
we should use the return value of this when making the JSValueMakeBoolean result
(In reply to comment #17)
> Comment on attachment 252254 [details]
> View in context:
> > Tools/DumpRenderTree/AccessibilityUIElement.cpp:912
> > + uiElement->setSelectedVisibleTextRange(textMarkerRange);
> we should use the return value of this when making the JSValueMakeBoolean
please upload patch to address my last comment. thanks
Created attachment 252349 [details]
update from review
Comment on attachment 252349 [details]
Clearing flags on attachment: 252349
Committed r183783: <http://trac.webkit.org/changeset/183783>
All reviewed patches have been landed. Closing bug.