Summary: | AX: WKView does not become first responder when the voiceover cursor lands on it | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, commit-queue, dmazzoni, jdiggs, lforschler, mario, thorton, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
chris fleizach
2013-08-14 16:07:30 PDT
Created attachment 208770 [details]
patch
Created attachment 208771 [details]
patch
Comment on attachment 208771 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=208771&action=review > Source/WebCore/accessibility/AccessibilityScrollView.cpp:106 > + return webArea->setFocused(focused); why return? Comment on attachment 208771 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=208771&action=review >> Source/WebCore/accessibility/AccessibilityScrollView.cpp:106 >> + return webArea->setFocused(focused); > > why return? ah good point. no need. will remove Also, can we test this? (In reply to comment #7) > Also, can we test this? I don't think there's an easy way to test this because focus has to be outside the web content and then highlight scroll view content and then we need to confirm that WKView is the first responder. Comment on attachment 208771 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=208771&action=review > Source/WebCore/accessibility/AccessibilityScrollView.cpp:93 > + if (AccessibilityObject* webArea = webAreaObject()) > + return webArea->canSetFocusAttribute(); > + return false; I think small functions like this read better as: AccessibilityObject* webArea = webAreaObject(); return webArea && webArea->canSetFocusAttribute(); > Source/WebCore/accessibility/AccessibilityScrollView.h:69 > + virtual void setFocused(bool); > + virtual bool canSetFocusAttribute() const; > + virtual bool isFocused() const; Should have OVERRIDE on all these. (In reply to comment #10) > (From update of attachment 208771 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208771&action=review > > > Source/WebCore/accessibility/AccessibilityScrollView.cpp:93 > > + if (AccessibilityObject* webArea = webAreaObject()) > > + return webArea->canSetFocusAttribute(); > > + return false; > > I think small functions like this read better as: > > AccessibilityObject* webArea = webAreaObject(); > return webArea && webArea->canSetFocusAttribute(); > > > Source/WebCore/accessibility/AccessibilityScrollView.h:69 > > + virtual void setFocused(bool); > > + virtual bool canSetFocusAttribute() const; > > + virtual bool isFocused() const; > > Should have OVERRIDE on all these. Thanks. Will fix these right away (In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 208771 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=208771&action=review > > > > > Source/WebCore/accessibility/AccessibilityScrollView.cpp:93 > > > + if (AccessibilityObject* webArea = webAreaObject()) > > > + return webArea->canSetFocusAttribute(); > > > + return false; > > > > I think small functions like this read better as: > > > > AccessibilityObject* webArea = webAreaObject(); > > return webArea && webArea->canSetFocusAttribute(); > > > > > Source/WebCore/accessibility/AccessibilityScrollView.h:69 > > > + virtual void setFocused(bool); > > > + virtual bool canSetFocusAttribute() const; > > > + virtual bool isFocused() const; > > > > Should have OVERRIDE on all these. > > Thanks. Will fix these right away http://trac.webkit.org/changeset/154123 |