Bug 119824 - AX: WKView does not become first responder when the voiceover cursor lands on it
Summary: AX: WKView does not become first responder when the voiceover cursor lands on it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-08-14 16:07 PDT by chris fleizach
Modified: 2013-08-15 12:59 PDT (History)
9 users (show)

See Also:


Attachments
patch (2.81 KB, patch)
2013-08-14 16:29 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (2.67 KB, patch)
2013-08-14 16:31 PDT, chris fleizach
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2013-08-14 16:07:30 PDT
* 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
Comment 1 chris fleizach 2013-08-14 16:07:37 PDT
rdar://14719187
Comment 2 Lucas Forschler 2013-08-14 16:12:03 PDT
<rdar://problem/14719187>
Comment 3 chris fleizach 2013-08-14 16:29:48 PDT
Created attachment 208770 [details]
patch
Comment 4 chris fleizach 2013-08-14 16:31:53 PDT
Created attachment 208771 [details]
patch
Comment 5 Tim Horton 2013-08-14 16:35:09 PDT
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 6 chris fleizach 2013-08-14 16:41:22 PDT
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
Comment 7 Tim Horton 2013-08-14 16:41:38 PDT
Also, can we test this?
Comment 8 chris fleizach 2013-08-14 16:45:32 PDT
(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 9 chris fleizach 2013-08-14 16:59:11 PDT
http://trac.webkit.org/changeset/154083
Comment 10 Darin Adler 2013-08-15 12:23:18 PDT
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.
Comment 11 chris fleizach 2013-08-15 12:26:53 PDT
(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
Comment 12 chris fleizach 2013-08-15 12:59:14 PDT
(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