Bug 60661 – WK2: VoiceOver cannot move focus into a web area programmatically

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[chris fleizach]

Due to 
https://bugs.webkit.org/show_bug.cgi?id=60315
the code to bring focus automatically to a view was removed. this still needs to be allowed when VoiceOver requests focus move to an element in a view

[chris fleizach]

rdar://9384929

[chris fleizach]

I have a fix by adding a new WebProxyMethod that will do what we want explicitly

[chris fleizach]

Created attachment 93327 [details]
patch

[WebKit Review Bot]

Comment on attachment 93327 [details]
patch

Attachment 93327 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8687776

[Early Warning System Bot]

Comment on attachment 93327 [details]
patch

Attachment 93327 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8685848

[WebKit Review Bot]

Comment on attachment 93327 [details]
patch

Attachment 93327 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8688818

[chris fleizach]

Created attachment 93341 [details]
patch

[chris fleizach]

The patch adds a new ChromeClient method that is named explicitly (setFocusOnContentView()).

this can be called be accessibility independently of other mechanisms

[Early Warning System Bot]

Comment on attachment 93341 [details]
patch

Attachment 93341 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8688840

[chris fleizach]

Created attachment 93347 [details]
patch

[chris fleizach]

Created attachment 93390 [details]
patch

[Maciej Stachowiak]

Comment on attachment 93390 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93390&action=review

Marking r- because I am not totally sure this is the right design and I feel this needs another attempt.

> Source/WebKit/chromium/ChangeLog:9
> +        * src/ChromeClientImpl.h:
> +        (WebKit::ChromeClientImpl::setFocusOnContentView):

I'm not entirely happy with the naming of this method.

(1) The use of ContentView here seems inconsistent with the use in Cocoa, where it is a Window's main view. The WKView may or may not be the content view.
(2) It sounds like this is totally generic and should be used any time a view is to gain focus, but it's actually a special-purpose method for accessibility.
(3) The name makes it sound like it is needed for every port, but it's really just a special thing for WebKit2.

If this is even the right design, can we give it a name that's more appropriate?

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:205
> +void WebChromeClient::setFocusOnContentView()
> +{
> +    // On WK1, this is the same behavior as focus
> +    focus();

Even though you said in a comment that this is only needed for WebKit2, here's an actual implementation for WebKit1. Is the comment wrong, or is this code redundant?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2212
> +    // The containing view needs to be informed that accessibility wants to 
> +    // move focus into this view. This is only needed on WK2, because it
> +    // happens automatically in WK1.
> +    
> +    if (on && documentFrameView() && !documentFrameView()->platformWidget())
> +        m_renderer->document()->page()->chrome()->setFocusOnContentView();

So, why can't we just make WebKit2 do the right thing in the same way that WebKit1 handled this? I get that there was a previous bug due to trying to do that, but maybe that fix was too aggressive. Perhaps it is possible to focus the view without causing side effects like wrongly having windows bring themselves to the front. I feel like that is a better approach than adding this new ChromeClient method that is unnecessary in all other ports but does a special hack for WebKit2.

[chris fleizach]

> (1) The use of ContentView here seems inconsistent with the use in Cocoa, where it is a Window's main view. The WKView may or may not be the content view.
> (2) It sounds like this is totally generic and should be used any time a view is to gain focus, but it's actually a special-purpose method for accessibility.

It shouldn't necessarily be an accessibility thing. I imagine if a javascript method called focus() while focus was not in the WKView, it might also not work.

> (3) The name makes it sound like it is needed for every port, but it's really just a special thing for WebKit2.

It's not a special thing. it's a general purpose mechanism, that happens to right now only be needed for WK2, because WK1 does the right thing already

> 
> If this is even the right design, can we give it a name that's more appropriate?
> 

However, I agree this is probably not the best approach

> > Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:205
> > +void WebChromeClient::setFocusOnContentView()
> > +{
> > +    // On WK1, this is the same behavior as focus
> > +    focus();
> 
> Even though you said in a comment that this is only needed for WebKit2, here's an actual implementation for WebKit1. Is the comment wrong, or is this code redundant?
> 

It is only there to fill in the ChromeClient API. If it's called, it will function correctly.


> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2212
> > +    // The containing view needs to be informed that accessibility wants to 
> > +    // move focus into this view. This is only needed on WK2, because it
> > +    // happens automatically in WK1.
> > +    
> > +    if (on && documentFrameView() && !documentFrameView()->platformWidget())
> > +        m_renderer->document()->page()->chrome()->setFocusOnContentView();
> 
> So, why can't we just make WebKit2 do the right thing in the same way that WebKit1 handled this? I 
get that there was a previous bug due to trying to do that, but maybe that fix was too aggressive. Perhaps it is possible to focus the view without causing side effects like wrongly having windows bring themselves to the front. I feel like that is a better approach than adding this new ChromeClient method that is unnecessary in all other ports but does a special hack for WebKit2.

I can try that again. As you can imagine, I don't want this to revert again at the last minute, so that's why I was adding a new method that was very explicit in what it did and would not likely be removed or altered so easily

[chris fleizach]

Created attachment 93857 [details]
patch

[Early Warning System Bot]

Comment on attachment 93857 [details]
patch

Attachment 93857 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8704488

[chris fleizach]

Created attachment 93875 [details]
patch

[Maciej Stachowiak]

Comment on attachment 93875 [details]
patch

r=me

[chris fleizach]

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