Bug 213744 – AX: Implement relevant simulated key presses for custom ARIA widgets for increment/decrement

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[chris fleizach]

Implement relevant simulated key presses for custom ARIA widgets, according to Authoring Practices Guide… 

For example: 
- increment is swipe up on iOS, and interact with then VO+Up on macOS. In both cases, WebKit shoud send the relevant arrow key.

Temp PR URL:
https://github.com/WICG/aom/blob/keyevents/explainer.md#user-action-events-from-assistive-technology

Will be merged here.
https://github.com/WICG/aom/blob/gh-pages/explainer.md#user-action-events-from-assistive-technology


<rdar://problem/64820964>

[chris fleizach]

Created attachment 403082 [details]
patch`

[chris fleizach]

Created attachment 403083 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #2)
> Created attachment 403083 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
@@ -1094,18 +1095,41 @@ void AccessibilityNodeObject::decrement()
     alterSliderValue(false);
 }

+// Fire a keyboard event if we were not able to set this value natively.

I'd put this comment in the declaration in the header instead.

+void AccessibilityNodeObject::postKeyboardKeysForValueChange(bool increase)
+{
+    auto keyInit = KeyboardEvent::Init();
+    bool vertical = orientation() == AccessibilityOrientation::Vertical;
+    bool isLTR = page()->userInterfaceLayoutDirection() == UserInterfaceLayoutDirection::LTR;
+
+    keyInit.key = increase ? vertical ? "up" : isLTR ? "right" : "left" : vertical ? "down" : isLTR ? "left" : "right";
+    keyInit.keyCode = increase ? vertical ? 38 : isLTR ? 39 : 37 : vertical ? 40 : isLTR ? 37: 39;

Missing space after 37:
37: 39; -> 37 : 39;
Aren't named consts for these values?

+
+    auto downEvent = KeyboardEvent::create(eventNames().keydownEvent, keyInit);
+    node()->dispatchEvent(downEvent);
+
+    auto upEvent = KeyboardEvent::create(eventNames().keyupEvent, keyInit);
+    node()->dispatchEvent(upEvent);
+}
+
+void AccessibilityNodeObject::setNodeValue(bool increase, float value)
+{
+    bool didSet = setValue(String::number(value));
+
+    if (didSet) {
+        if (auto* cache = axObjectCache())
+            cache->postNotification(node(), AXObjectCache::AXValueChanged);

Why not cache->postNotification(this, AXObjectCache::AXValueChanged) directly?

+    } else
+        postKeyboardKeysForValueChange(increase);

Don't we need to postNotification here too?

[chris fleizach]

> +    } else
> +        postKeyboardKeysForValueChange(increase);
> 
> Don't we need to postNotification here too?

This will allow the web app to update itself if necessary and that change would result in a AXValueChange (if a change occurred). At this point (just posting keys) we don't know if the value changes

[chris fleizach]

Created attachment 403269 [details]
patch

[chris fleizach]

Created attachment 403270 [details]
patch

[Darin Adler]

Comment on attachment 403270 [details]
patch

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

looks generally OK, but super concerned about object lifetime and possibly introducing a security bug

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1106
> +    keyInit.key = increase ? vertical ? "ArrowUp" : isLTR ? "ArrowRight" : "ArrowLeft" : vertical ? "ArrowDown" : isLTR ? "ArrowLeft" : "ArrowRight";
> +    keyInit.keyIdentifier = increase ? vertical ? "up" : isLTR ? "right" : "left" : vertical ? "down" : isLTR ? "left" : "right";

Using _s on all these constants should make things more efficient.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1109
> +    auto downEvent = KeyboardEvent::create(eventNames().keydownEvent, keyInit);
> +    node()->dispatchEvent(downEvent);

One-liner instead?

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1112
> +    auto upEvent = KeyboardEvent::create(eventNames().keyupEvent, keyInit);
> +    node()->dispatchEvent(upEvent);

One-liner instead?

After dispatching the first event, lots may have changed. I don’t think we have a guarantee about node() after that. Maybe it’s been deallocated or is not in the document any more? Are we guaranteed that "this" is still alive and node() is still alive, non-null, and attached to the document? May need to intentionally construct a test case where the down event handler makes huge disruptive changes.

[chris fleizach]

(In reply to Darin Adler from comment #7)
> Comment on attachment 403270 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403270&action=review
> 
> looks generally OK, but super concerned about object lifetime and possibly
> introducing a security bug
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1106
> > +    keyInit.key = increase ? vertical ? "ArrowUp" : isLTR ? "ArrowRight" : "ArrowLeft" : vertical ? "ArrowDown" : isLTR ? "ArrowLeft" : "ArrowRight";
> > +    keyInit.keyIdentifier = increase ? vertical ? "up" : isLTR ? "right" : "left" : vertical ? "down" : isLTR ? "left" : "right";
> 
> Using _s on all these constants should make things more efficient.
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1109
> > +    auto downEvent = KeyboardEvent::create(eventNames().keydownEvent, keyInit);
> > +    node()->dispatchEvent(downEvent);
> 
> One-liner instead?
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1112
> > +    auto upEvent = KeyboardEvent::create(eventNames().keyupEvent, keyInit);
> > +    node()->dispatchEvent(upEvent);
> 
> One-liner instead?

> 
> After dispatching the first event, lots may have changed. I don’t think we
> have a guarantee about node() after that. Maybe it’s been deallocated or is
> not in the document any more? Are we guaranteed that "this" is still alive
> and node() is still alive, non-null, and attached to the document? May need
> to intentionally construct a test case where the down event handler makes
> huge disruptive changes.

Will add a test case for that and add more checks

[Darin Adler]

Comment on attachment 403270 [details]
patch

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

>>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1106
>>> +    keyInit.keyIdentifier = increase ? vertical ? "up" : isLTR ? "right" : "left" : vertical ? "down" : isLTR ? "left" : "right";
>> 
>> Using _s on all these constants should make things more efficient.
> 
> 

Another idea would be to use the constants from EventNames.h instead of literals.

[chris fleizach]

Created attachment 403307 [details]
landing patch

[Darin Adler]

Comment on attachment 403307 [details]
landing patch

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

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1111
> +    // Ensure node is still valid and wasn't removed after the keydown.

What about the AccessibilityNodeObject? Is that guaranteed to still be alive? I don’t see this function doing a ref/deref on it, so the guarantee must come from outside the function.

[chris fleizach]

Comment on attachment 403307 [details]
landing patch

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

>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1111
>> +    // Ensure node is still valid and wasn't removed after the keydown.
> 
> What about the AccessibilityNodeObject? Is that guaranteed to still be alive? I don’t see this function doing a ref/deref on it, so the guarantee must come from outside the function.

I don't see a guarantee for that. we'll need to add that

[chris fleizach]

Created attachment 403315 [details]
patch

[chris fleizach]

Created attachment 403321 [details]
Patch

Skip tests on windows

[EWS]

Committed r263823: <https://trac.webkit.org/changeset/263823>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403321 [details].