Bug 183352 – AX: Implement accessible dismiss action on iOS

• Component: Accessibility
• Status: RESOLVED FIXED

[Nan Wang]

With AOM accessibility events
Spec: https://wicg.github.io/aom/spec/phase2.html

we have a onaccessibledismiss event listener.

Let's first support the escape action on iOS.

[Radar WebKit Bug Importer]

<rdar://problem/38161500>

[Nan Wang]

Created attachment 335123 [details]
patch

[Nan Wang]

(In reply to Nan Wang from comment #2)
> Created attachment 335123 [details]
> patch

Wrong bug! :(

[chris fleizach]

Created attachment 335685 [details]
patch

[chris fleizach]

Created attachment 335686 [details]
patch

[Nan Wang]

Comment on attachment 335686 [details]
patch

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

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1496
> +    return m_object->dispatchAccessibilityEventWithType(AccessibilityEventType::Dismiss);

I think we should somehow determine when to dispatch this event, and return YES. Otherwise the page in Safari would still back to the previous page. 
Maybe we dispatch the event only when there's a listener

[chris fleizach]

Comment on attachment 335686 [details]
patch

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

>> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1496
>> +    return m_object->dispatchAccessibilityEventWithType(AccessibilityEventType::Dismiss);
> 
> I think we should somehow determine when to dispatch this event, and return YES. Otherwise the page in Safari would still back to the previous page. 
> Maybe we dispatch the event only when there's a listener

can we just if the event Default was prevented? is that good enough?

[Nan Wang]

Comment on attachment 335686 [details]
patch

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

>>> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1496
>>> +    return m_object->dispatchAccessibilityEventWithType(AccessibilityEventType::Dismiss);
>> 
>> I think we should somehow determine when to dispatch this event, and return YES. Otherwise the page in Safari would still back to the previous page. 
>> Maybe we dispatch the event only when there's a listener
> 
> can we just if the event Default was prevented? is that good enough?

It works but this event listener will only work with preventDefault. I'm not sure if it's a good practice?

[Nan Wang]

Comment on attachment 335686 [details]
patch

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

>>>> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1496
>>>> +    return m_object->dispatchAccessibilityEventWithType(AccessibilityEventType::Dismiss);
>>> 
>>> I think we should somehow determine when to dispatch this event, and return YES. Otherwise the page in Safari would still back to the previous page. 
>>> Maybe we dispatch the event only when there's a listener
>> 
>> can we just if the event Default was prevented? is that good enough?
> 
> It works but this event listener will only work with preventDefault. I'm not sure if it's a good practice?

Another concern is that there could be some generic dismiss action we implement in future (something similar to enter a 'ESC' keycode), and preventDefault is to prevent calling into that generic function.
But I think currently I'm fine with either case.

[chris fleizach]

Created attachment 335765 [details]
patch

[chris fleizach]

> Another concern is that there could be some generic dismiss action we
> implement in future (something similar to enter a 'ESC' keycode), and
> preventDefault is to prevent calling into that generic function.
> But I think currently I'm fine with either case.

can you take a look again?

[Nan Wang]

Comment on attachment 335765 [details]
patch

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

> LayoutTests/accessibility/ios-simulator/AOM-dismiss-event.html:30
> +        axNode.dismiss();

Can we also test the case when there's no event listener? Should be easy since dismiss() returns a boolean

[Nan Wang]

Comment on attachment 335765 [details]
patch

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

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1496
> +    m_object->dispatchAccessibilityEventWithType(AccessibilityEventType::Dismiss);

I think we should also call shouldDispatchAccessibilityEvent() here

[Nan Wang]

(In reply to Nan Wang from comment #13)
> Comment on attachment 335765 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335765&action=review
> 
> > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1496
> > +    m_object->dispatchAccessibilityEventWithType(AccessibilityEventType::Dismiss);
> 
> I think we should also call shouldDispatchAccessibilityEvent() here

nevermind, it's checked in dispatchAccessibilityEventWithType already.

[chris fleizach]

Created attachment 335771 [details]
patch

[chris fleizach]

(In reply to Nan Wang from comment #14)
> (In reply to Nan Wang from comment #13)
> > Comment on attachment 335765 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=335765&action=review
> > 
> > > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1496
> > > +    m_object->dispatchAccessibilityEventWithType(AccessibilityEventType::Dismiss);
> > 
> > I think we should also call shouldDispatchAccessibilityEvent() here
> 
> nevermind, it's checked in dispatchAccessibilityEventWithType already.

we should probably check that when returning whether this event was processed. updated patch...

[Nan Wang]

(In reply to chris fleizach from comment #16)
> (In reply to Nan Wang from comment #14)
> > (In reply to Nan Wang from comment #13)
> > > Comment on attachment 335765 [details]
> > > patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=335765&action=review
> > > 
> > > > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1496
> > > > +    m_object->dispatchAccessibilityEventWithType(AccessibilityEventType::Dismiss);
> > > 
> > > I think we should also call shouldDispatchAccessibilityEvent() here
> > 
> > nevermind, it's checked in dispatchAccessibilityEventWithType already.
> 
> we should probably check that when returning whether this event was
> processed. updated patch...

Yes I was about to comment on this. Can we also add a test case when there's no event listener?

[chris fleizach]

Created attachment 335773 [details]
patch

[chris fleizach]

> Yes I was about to comment on this. Can we also add a test case when there's
> no event listener?

Added

[alan]

Comment on attachment 335773 [details]
patch

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

> Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:74
> +    for (Node* node = this->node(); node; node = node->parentNode()) {

auto* should do.

[chris fleizach]

Created attachment 335775 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 335775 [details]
patch for landing

Clearing flags on attachment: 335775

Committed r229603: <https://trac.webkit.org/changeset/229603>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.