[webkit-reviews] review granted: [Bug 68412] REGRESSION (WK2): (Shift-)option-tabbing skips over elements when transitioning from chrome to webview : [Attachment 109587] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jon Lee <jonlee at apple.com>'s request
for review:
Bug 68412: REGRESSION (WK2): (Shift-)option-tabbing skips over elements when
transitioning from chrome to webview
https://bugs.webkit.org/show_bug.cgi?id=68412

Attachment 109587: Patch
https://bugs.webkit.org/attachment.cgi?id=109587&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=109587&action=review


> Source/WebKit2/ChangeLog:53
> +2011-09-23  Jon Lee	<jonlee at apple.com>
> +
> +	   REGRESSION (WK2): (Shift-)option-tabbing skips over elements when
transitioning from chrome to webview
> +	   https://bugs.webkit.org/show_bug.cgi?id=68412
> +	   <rdar://problem/9988252>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   In WK1 setInitialFocus() is called on FocusController with the key
event that
> +	   caused the web view to become first responder. In WK2 no event is
sent. So if
> +	   the key stroke that caused the change in first responder status
contains the
> +	   option modifier key, FocusController did not know that it had to
switch behavior.
> +
> +	   Because there are multiple ways that the WKView can
becomeFirstResponder, I changed
> +	   the signature to setInitialFocus to express whether the key event
parameter is an
> +	   actual key event.

Double change log.

> Source/WebKit2/UIProcess/API/C/win/WKView.cpp:73
> +    bool isKeyboardEventValid = false;
> +    toImpl(viewRef)->setInitialFocus(forward, isKeyboardEventValid,
WebKeyboardEvent());

This is awkward.

I think we’d be better off with two separate setInitialFocus functions and two
separate messages, one for non-keyboard events and another for keyboard events.
We could avoid the whole boolean thing that way, and we could do whatever code
sharing we wanted in the WebPage class.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:307
> -
> +    

gratuitous whitespace change

> Source/WebKit2/UIProcess/API/mac/WKView.mm:312
> -
> +    

gratuitous whitespace change

> Source/WebKit2/UIProcess/API/mac/WKView.mm:319
> +	   bool becomingFirstResponderFromKeyboard = [event type] == NSKeyDown
|| [event type] == NSKeyUp;
> +	   if (!becomingFirstResponderFromKeyboard)
> +	       event = nil;

I think this is a confusing way to write it. I would instead have a local
variable called keyboardEvent.

    NSEvent *keyboardEvent = nil;
    if ([event type] == NSKeyDown || [event type] == NSKeyUp)
	keyboardEvent = event;