Bug 119824 – AX: WKView does not become first responder when the voiceover cursor lands on it

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

* SUMMARY
WKView does not become first responder when the voiceover cursor lands on it

* STEPS TO REPRODUCE
1. Open Safari or Mail and enable voiceover
2. Navigate voiceover cursor to the web view

* RESULTS
Expected:
The WKView should become the first responder

Actual:
The WKView does not become the first responder

[chris fleizach]

rdar://14719187

[Lucas Forschler]

<rdar://problem/14719187>

[chris fleizach]

Created attachment 208770 [details]
patch

[chris fleizach]

Created attachment 208771 [details]
patch

[Tim Horton]

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?

[chris fleizach]

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

[Tim Horton]

Also, can we test this?

[chris fleizach]

(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.

[chris fleizach]

http://trac.webkit.org/changeset/154083

[Darin Adler]

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.

[chris fleizach]

(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

[chris fleizach]

(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