Bug 55959

Summary: REGRESSION (WebKit2): VoiceOver focus no longer follows keyboard focus
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bdakin, buildbot, sam, webkit-ews
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch
none
patch
none
patch
none
patch
none
patch
darin: review-
patch darin: review+

Description chris fleizach 2011-03-08 12:12:44 PST
When focus changes, VoiceOver does not follow in WK2
Comment 1 chris fleizach 2011-03-08 14:57:59 PST
Created attachment 85097 [details]
patch
Comment 2 chris fleizach 2011-03-08 17:40:53 PST
Created attachment 85116 [details]
patch

first patch didn't svn apply
Comment 3 chris fleizach 2011-03-08 17:41:52 PST
Created attachment 85117 [details]
patch
Comment 4 chris fleizach 2011-03-09 11:39:55 PST
Created attachment 85206 [details]
patch
Comment 5 Build Bot 2011-03-09 12:07:40 PST
Attachment 85206 [details] did not build on win:
Build output: http://queues.webkit.org/results/8117576
Comment 6 Early Warning System Bot 2011-03-09 12:22:12 PST
Attachment 85206 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8110964
Comment 7 chris fleizach 2011-03-09 15:53:25 PST
Created attachment 85250 [details]
patch
Comment 8 Darin Adler 2011-03-10 11:49:29 PST
Comment on attachment 85250 [details]
patch

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

> Source/WebKit2/WebProcess/WebProcess.cpp:465
> +#if PLATFORM(MAC)
> +    Vector<RefPtr<WebPage> > pages;
> +    copyValuesToVector(m_pageMap, pages);
> +    for (size_t i = 0; i < pages.size(); ++i)
> +        if (pages[i]->windowIsFocused())
> +            return pages[i].get();
> +#endif
> +    return 0;

Is always zero correct for non-Mac platforms? Or is this just not implemented for those platforms? Can’t we just compile this function on all platforms? If we are having some problem compiling it on some of those other platforms where it’s also not called, we could just put the #if around the entire function definition instead, which I think would be clearer.

Do we really need to copy the map to a vector? Why can’t we just iterate the map? Can the windowIsFocused function change the contents of the map?

Need braces around the body of the for loop.

> Source/WebKit2/WebProcess/mac/NSApplicationOverride.mm:32
> +@implementation NSApplication (NSApplicationOverride)

Defining a category is not a reliable way to override the implementation of a method in an AppKit class even though it often seems to work. The reliable method involves method_setImplementation and is done in source files such as WebPluginController.mm, WebHTMLView.mm, and WebView.mm in WebKit.

> Source/WebKit2/WebProcess/mac/NSApplicationOverride.mm:39
> +    if (!WebKit::WebProcess::shared().isSeparateProcess())

It’s normally better to put using namespace WebKit at the start of a file rather than doing WebKit:: everywhere.
Comment 9 chris fleizach 2011-03-10 16:17:19 PST
Created attachment 85405 [details]
patch
Comment 10 chris fleizach 2011-03-10 16:40:37 PST
http://trac.webkit.org/changeset/80789