Bug 60661 - WK2: VoiceOver cannot move focus into a web area programmatically
Summary: WK2: VoiceOver cannot move focus into a web area programmatically
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-05-11 14:22 PDT by chris fleizach
Modified: 2011-05-18 17:11 PDT (History)
5 users (show)

See Also:


Attachments
patch (11.01 KB, patch)
2011-05-12 13:19 PDT, chris fleizach
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch (20.00 KB, patch)
2011-05-12 14:18 PDT, chris fleizach
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
patch (20.51 KB, patch)
2011-05-12 14:41 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (21.02 KB, patch)
2011-05-12 19:21 PDT, chris fleizach
mjs: review-
Details | Formatted Diff | Diff
patch (8.28 KB, patch)
2011-05-17 18:36 PDT, chris fleizach
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
patch (8.54 KB, patch)
2011-05-18 00:08 PDT, chris fleizach
mjs: 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 2011-05-11 14:22:19 PDT
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
Comment 1 chris fleizach 2011-05-11 18:23:30 PDT
rdar://9384929
Comment 2 chris fleizach 2011-05-11 18:23:58 PDT
I have a fix by adding a new WebProxyMethod that will do what we want explicitly
Comment 3 chris fleizach 2011-05-12 13:19:49 PDT
Created attachment 93327 [details]
patch
Comment 4 WebKit Review Bot 2011-05-12 13:28:20 PDT
Comment on attachment 93327 [details]
patch

Attachment 93327 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8687776
Comment 5 Early Warning System Bot 2011-05-12 13:30:20 PDT
Comment on attachment 93327 [details]
patch

Attachment 93327 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8685848
Comment 6 WebKit Review Bot 2011-05-12 13:52:11 PDT
Comment on attachment 93327 [details]
patch

Attachment 93327 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8688818
Comment 7 chris fleizach 2011-05-12 14:18:32 PDT
Created attachment 93341 [details]
patch
Comment 8 chris fleizach 2011-05-12 14:29:05 PDT
The patch adds a new ChromeClient method that is named explicitly (setFocusOnContentView()).

this can be called be accessibility independently of other mechanisms
Comment 9 Early Warning System Bot 2011-05-12 14:38:05 PDT
Comment on attachment 93341 [details]
patch

Attachment 93341 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8688840
Comment 10 chris fleizach 2011-05-12 14:41:42 PDT
Created attachment 93347 [details]
patch
Comment 11 chris fleizach 2011-05-12 19:21:57 PDT
Created attachment 93390 [details]
patch
Comment 12 Maciej Stachowiak 2011-05-12 22:06:37 PDT
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.
Comment 13 chris fleizach 2011-05-12 22:13:30 PDT
> (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
Comment 14 chris fleizach 2011-05-17 18:36:53 PDT
Created attachment 93857 [details]
patch
Comment 15 Early Warning System Bot 2011-05-17 18:46:49 PDT
Comment on attachment 93857 [details]
patch

Attachment 93857 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8704488
Comment 16 chris fleizach 2011-05-18 00:08:49 PDT
Created attachment 93875 [details]
patch
Comment 17 Maciej Stachowiak 2011-05-18 16:08:30 PDT
Comment on attachment 93875 [details]
patch

r=me
Comment 18 chris fleizach 2011-05-18 17:11:29 PDT
http://trac.webkit.org/changeset/86806