[webkit-reviews] review granted: [Bug 213744] AX: Implement relevant simulated key presses for custom ARIA widgets for increment/decrement : [Attachment 403270] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 213744: AX: Implement relevant simulated key presses for custom ARIA
widgets for increment/decrement
https://bugs.webkit.org/show_bug.cgi?id=213744

Attachment 403270: patch

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




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 403270
  --> https://bugs.webkit.org/attachment.cgi?id=403270
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.