Bug 183352 - AX: Implement accessible dismiss action on iOS
Summary: AX: Implement accessible dismiss action on iOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-05 17:21 PST by Nan Wang
Modified: 2018-03-14 10:51 PDT (History)
11 users (show)

See Also:


Attachments
patch (4.31 KB, patch)
2018-03-06 11:58 PST, Nan Wang
no flags Details | Formatted Diff | Diff
patch (4.48 KB, patch)
2018-03-13 02:17 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (7.75 KB, patch)
2018-03-13 02:17 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (9.16 KB, patch)
2018-03-14 00:18 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (9.21 KB, patch)
2018-03-14 08:58 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (9.58 KB, patch)
2018-03-14 09:09 PDT, chris fleizach
zalan: review+
Details | Formatted Diff | Diff
patch for landing (9.87 KB, patch)
2018-03-14 09:24 PDT, chris fleizach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nan Wang 2018-03-05 17:21:42 PST
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.
Comment 1 Radar WebKit Bug Importer 2018-03-05 17:22:23 PST
<rdar://problem/38161500>
Comment 2 Nan Wang 2018-03-06 11:58:50 PST
Created attachment 335123 [details]
patch
Comment 3 Nan Wang 2018-03-06 12:01:03 PST
(In reply to Nan Wang from comment #2)
> Created attachment 335123 [details]
> patch

Wrong bug! :(
Comment 4 chris fleizach 2018-03-13 02:17:03 PDT
Created attachment 335685 [details]
patch
Comment 5 chris fleizach 2018-03-13 02:17:53 PDT
Created attachment 335686 [details]
patch
Comment 6 Nan Wang 2018-03-13 10:49:47 PDT
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
Comment 7 chris fleizach 2018-03-13 12:08:28 PDT
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?
Comment 8 Nan Wang 2018-03-13 12:14:11 PDT
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?
Comment 9 Nan Wang 2018-03-13 12:24:33 PDT
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.
Comment 10 chris fleizach 2018-03-14 00:18:54 PDT
Created attachment 335765 [details]
patch
Comment 11 chris fleizach 2018-03-14 08:51:01 PDT
> 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?
Comment 12 Nan Wang 2018-03-14 08:54:27 PDT
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
Comment 13 Nan Wang 2018-03-14 08:56:09 PDT
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
Comment 14 Nan Wang 2018-03-14 08:56:48 PDT
(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.
Comment 15 chris fleizach 2018-03-14 08:58:39 PDT
Created attachment 335771 [details]
patch
Comment 16 chris fleizach 2018-03-14 08:59:00 PDT
(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...
Comment 17 Nan Wang 2018-03-14 09:03:24 PDT
(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?
Comment 18 chris fleizach 2018-03-14 09:09:51 PDT
Created attachment 335773 [details]
patch
Comment 19 chris fleizach 2018-03-14 09:10:07 PDT
> Yes I was about to comment on this. Can we also add a test case when there's
> no event listener?

Added
Comment 20 zalan 2018-03-14 09:15:14 PDT
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.
Comment 21 chris fleizach 2018-03-14 09:24:59 PDT
Created attachment 335775 [details]
patch for landing
Comment 22 WebKit Commit Bot 2018-03-14 10:51:15 PDT
Comment on attachment 335775 [details]
patch for landing

Clearing flags on attachment: 335775

Committed r229603: <https://trac.webkit.org/changeset/229603>
Comment 23 WebKit Commit Bot 2018-03-14 10:51:16 PDT
All reviewed patches have been landed.  Closing bug.