Summary: | WK2: VoiceOver cannot move focus into a web area programmatically | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||||||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | andersca, bdakin, dglazkov, sam, webkit.review.bot | ||||||||||||||
Priority: | P1 | Keywords: | InRadar | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
chris fleizach
2011-05-11 14:22:19 PDT
I have a fix by adding a new WebProxyMethod that will do what we want explicitly Created attachment 93327 [details]
patch
Comment on attachment 93327 [details] patch Attachment 93327 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8687776 Comment on attachment 93327 [details] patch Attachment 93327 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8685848 Comment on attachment 93327 [details] patch Attachment 93327 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8688818 Created attachment 93341 [details]
patch
The patch adds a new ChromeClient method that is named explicitly (setFocusOnContentView()). this can be called be accessibility independently of other mechanisms Comment on attachment 93341 [details] patch Attachment 93341 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8688840 Created attachment 93347 [details]
patch
Created attachment 93390 [details]
patch
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. > (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 Created attachment 93857 [details]
patch
Comment on attachment 93857 [details] patch Attachment 93857 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8704488 Created attachment 93875 [details]
patch
Comment on attachment 93875 [details]
patch
r=me
|