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>
Created attachment 403082 [details] patch`
Created attachment 403083 [details] Patch
(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?
> + } 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
Created attachment 403269 [details] patch
Created attachment 403270 [details] patch
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.
(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
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.
Created attachment 403307 [details] landing patch
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.
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
Created attachment 403315 [details] patch
Created attachment 403321 [details] Patch Skip tests on windows
Committed r263823: <https://trac.webkit.org/changeset/263823> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403321 [details].