Bug 55281

Summary: WK2: Cannot set focus on an element when focus is outside of WKView
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, buildbot, sam, webkit-ews
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch
none
patch sam: review+

Description chris fleizach 2011-02-25 17:48:46 PST
If focus is set on a DOM object programmatically (for example, through accessibility) and focus is outside of the web area, 
then focus does not actually change.

This is because in Widget:setFocus() it assumes there is a platformWidget() to call

In this case we need to propagate the message back to the WKView so it can become the first responder again and focus will be updated.
Comment 1 chris fleizach 2011-02-25 17:53:20 PST
Created attachment 83909 [details]
patch

Not sure if this is the best way to do it, but it does address the issue
Comment 2 chris fleizach 2011-02-25 17:54:19 PST
ignore the momentumPhase change, that won't be committed
Comment 3 Build Bot 2011-02-25 18:43:42 PST
Attachment 83909 [details] did not build on win:
Build output: http://queues.webkit.org/results/8035504
Comment 4 Early Warning System Bot 2011-02-25 18:58:59 PST
Attachment 83909 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8032509
Comment 5 chris fleizach 2011-02-25 19:17:40 PST
Created attachment 83916 [details]
patch
Comment 6 Alexey Proskuryakov 2011-03-07 22:33:49 PST
I don't understand how focusing works, so I can only offer some nitpicks. Sam or Anders would be better reviewers.

+        WK2: Cannot set focus on an element when focus is outside of WKView
+        https://bugs.webkit.org/show_bug.cgi?id=55281

Please cross-link this with Radar bug(s).

+    // If there's no platformWidget(), we are likely in WK2 and should call
+    // focus to propagate the message.

This comment is somewhat confusing - it's unclear what a message is in this context.

+void PageClientImpl::setFocus(bool focused)
+{
+    if (focused)
+        [[m_wkView window] makeFirstResponder:m_wkView];
+    else
+        takeFocus(true);
+}

It's surprising that setFocus(false) turns into takeFocus(true)!
Comment 7 chris fleizach 2011-03-08 07:35:59 PST
(In reply to comment #6)
> I don't understand how focusing works, so I can only offer some nitpicks. Sam or Anders would be better reviewers.
> 
> +        WK2: Cannot set focus on an element when focus is outside of WKView
> +        https://bugs.webkit.org/show_bug.cgi?id=55281
> 
> Please cross-link this with Radar bug(s).
> 
> +    // If there's no platformWidget(), we are likely in WK2 and should call
> +    // focus to propagate the message.
> 
> This comment is somewhat confusing - it's unclear what a message is in this context.
> 
> +void PageClientImpl::setFocus(bool focused)
> +{
> +    if (focused)
> +        [[m_wkView window] makeFirstResponder:m_wkView];
> +    else
> +        takeFocus(true);
> +}
> 

This is strange, but I think takeFocus will takeFocus away from WKView and send it forward to the next view. I may be wrong however and this may not be the right way to accomplish removing focus.

> It's surprising that setFocus(false) turns into takeFocus(true)!
Comment 8 chris fleizach 2011-03-08 10:23:20 PST
rdar://9055113
Comment 9 Anders Carlsson 2011-03-08 10:53:11 PST
Comment on attachment 83916 [details]
patch

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

> WebKit2/UIProcess/API/mac/PageClientImpl.mm:204
> +}

I don't understand the logic here, could you elaborate?
Comment 10 chris fleizach 2011-03-08 10:55:59 PST
(In reply to comment #9)
> (From update of attachment 83916 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83916&action=review
> 
> > WebKit2/UIProcess/API/mac/PageClientImpl.mm:204
> > +}
> 
> I don't understand the logic here, could you elaborate?

If we want to focus onto the WKView, makeFirstResponder is the way to do it. If we want to remove focus from the WKView, takeFocus actually takes focus away from the current view

- (void)_takeFocus:(BOOL)forward
{
    if (forward)
        [[self window] selectKeyViewFollowingView:self];
    else
        [[self window] selectKeyViewPrecedingView:self];
}
Comment 11 chris fleizach 2011-03-08 12:05:03 PST
Thanks!
http://trac.webkit.org/changeset/80578