Summary: | AX: Implement accessible dismiss action on iOS | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nan Wang <n_wang> | ||||||||||||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, webkit-bug-importer, zalan | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Nan Wang
2018-03-05 17:21:42 PST
Created attachment 335123 [details]
patch
(In reply to Nan Wang from comment #2) > Created attachment 335123 [details] > patch Wrong bug! :( Created attachment 335685 [details]
patch
Created attachment 335686 [details]
patch
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 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 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 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. Created attachment 335765 [details]
patch
> 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 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 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 (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. Created attachment 335771 [details]
patch
(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... (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? Created attachment 335773 [details]
patch
> Yes I was about to comment on this. Can we also add a test case when there's
> no event listener?
Added
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. Created attachment 335775 [details]
patch for landing
Comment on attachment 335775 [details] patch for landing Clearing flags on attachment: 335775 Committed r229603: <https://trac.webkit.org/changeset/229603> All reviewed patches have been landed. Closing bug. |