Summary: | AX: Implement user action spec for Escape action | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||||||||||||||||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, darin, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, webkit-bug-importer | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||
Attachments: |
|
Description
chris fleizach
2020-07-02 00:43:27 PDT
Created attachment 403350 [details]
Patch
Created attachment 403352 [details]
patch
Created attachment 403373 [details]
patch
Created attachment 403393 [details]
patch
Created attachment 403402 [details]
patch
Created attachment 403442 [details]
patch
Comment on attachment 403442 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=403442&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1100 > + // In case the keyboard event causes this element to be removed. > + Ref<AccessibilityObject> protectedThis(*this); This should be moved int dispatchSimulatedKeyboardUpDownEvent; that’s where the protection is needed, not here. > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1110 > +bool AccessibilityNodeObject::dispatchSimulatedKeyboardUpDownEvent(KeyboardEvent::Init& keyInit) Should take a const& > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1132 > Ref<AccessibilityObject> protectedThis(*this); Can move this into dispatchSimulatedKeyboardUpDownEvent and remove it here. > Source/WebCore/accessibility/AccessibilityNodeObject.h:32 > +#include "KeyboardEvent.h" This dependency is really unfortunate, but I suppose there’s no way to forward declare something from inside a base class? > Source/WebCore/accessibility/AccessibilityNodeObject.h:195 > + bool performEscape() override; final instead of override > Source/WebCore/accessibility/AccessibilityNodeObject.h:196 > + bool dispatchSimulatedKeyboardUpDownEvent(KeyboardEvent::Init&); Argument should be const&, not non-const&. > Source/WebCore/accessibility/AccessibilityObject.h:481 > + bool performEscape() override { return false; } final instead of override > Source/WebCore/accessibility/AccessibilityObjectInterface.h:910 > + virtual bool performEscape() = 0; Seems like more of this should be named performDismissAction, and not use the word "escape". (In reply to Darin Adler from comment #8) > Comment on attachment 403442 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403442&action=review > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1100 > > + // In case the keyboard event causes this element to be removed. > > + Ref<AccessibilityObject> protectedThis(*this); > > This should be moved int dispatchSimulatedKeyboardUpDownEvent; that’s where > the protection is needed, not here. > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1110 > > +bool AccessibilityNodeObject::dispatchSimulatedKeyboardUpDownEvent(KeyboardEvent::Init& keyInit) > > Should take a const& > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1132 > > Ref<AccessibilityObject> protectedThis(*this); > > Can move this into dispatchSimulatedKeyboardUpDownEvent and remove it here. > > > Source/WebCore/accessibility/AccessibilityNodeObject.h:32 > > +#include "KeyboardEvent.h" I think I can make this a static function inside the .cpp file so it doesn't have to be in the header > > This dependency is really unfortunate, but I suppose there’s no way to > forward declare something from inside a base class? > > > Source/WebCore/accessibility/AccessibilityNodeObject.h:195 > > + bool performEscape() override; > > final instead of override > > > Source/WebCore/accessibility/AccessibilityNodeObject.h:196 > > + bool dispatchSimulatedKeyboardUpDownEvent(KeyboardEvent::Init&); > > Argument should be const&, not non-const&. > > > Source/WebCore/accessibility/AccessibilityObject.h:481 > > + bool performEscape() override { return false; } > > final instead of override > > > Source/WebCore/accessibility/AccessibilityObjectInterface.h:910 > > + virtual bool performEscape() = 0; > > Seems like more of this should be named performDismissAction, and not use > the word "escape". Created attachment 403609 [details]
patch
Created attachment 403610 [details]
patch
Comment on attachment 403610 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=403610&action=review > Source/WebCore/accessibility/AccessibilityObjectInterface.h:910 > + virtual bool performDismissAction() { return false; }; No need for the trailing semicolon here. Created attachment 403615 [details]
patch
thanks!
Created attachment 403623 [details]
Patch
Created attachment 403625 [details]
patch
Committed r264000: <https://trac.webkit.org/changeset/264000> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403625 [details]. |