WK2: Cannot set focus on an element when focus is outside of WKView

[chris fleizach]

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.

[chris fleizach]

Created attachment 83909 [details]
patch

Not sure if this is the best way to do it, but it does address the issue

[chris fleizach]

ignore the momentumPhase change, that won't be committed

[Build Bot]

Attachment 83909 [details] did not build on win:
Build output: http://queues.webkit.org/results/8035504

[Early Warning System Bot]

Attachment 83909 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8032509

[chris fleizach]

Created attachment 83916 [details]
patch

[Alexey Proskuryakov]

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)!

[chris fleizach]

(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)!

[chris fleizach]

rdar://9055113

[Anders Carlsson]

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?

[chris fleizach]

(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];
}

[chris fleizach]

Thanks!
http://trac.webkit.org/changeset/80578