[webkit-reviews] review denied: [Bug 60661] WK2: VoiceOver cannot move focus into a web area programmatically : [Attachment 93390] patch

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 60661: WK2: VoiceOver cannot move focus into a web area programmatically
https://bugs.webkit.org/show_bug.cgi?id=60661

Attachment 93390: patch
https://bugs.webkit.org/attachment.cgi?id=93390&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
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.