Bug 68412 – REGRESSION (WK2): (Shift-)option-tabbing skips over elements when transitioning from chrome to webview

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Jon Lee]

In a page with buttons as the first (or last) form element, option-tabbing will skip over the button. This is a regression against WK1.

<rdar://problem/9988252>

[Jon Lee]

Created attachment 108370 [details]
Patch

[Collabora GTK+ EWS bot]

Comment on attachment 108370 [details]
Patch

Attachment 108370 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9795325

[Alexey Proskuryakov]

Comment on attachment 108370 [details]
Patch

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

Just one comment in passing, I didn't review the whole patch.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:320
> +        if ([event type] == NSKeyDown || [event type] == NSKeyUp)
> +            _data->_page->setInitialFocus(direction == NSSelectingNext, true, NativeWebKeyboardEvent(event, self));
> +        else
> +            _data->_page->setInitialFocus(direction == NSSelectingNext, false, NativeWebKeyboardEvent(0, self));

Please don't pass new mysterious true/false arguments. A named local variable would work, or you could make the argument an enum.

[Darin Adler]

Comment on attachment 108370 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/mac/WKView.mm:320
>> +            _data->_page->setInitialFocus(direction == NSSelectingNext, false, NativeWebKeyboardEvent(0, self));
> 
> Please don't pass new mysterious true/false arguments. A named local variable would work, or you could make the argument an enum.

Or the keyboard event could be a pointer and we could pass 0 to mean no keyboard event.

[Jon Lee]

Created attachment 108387 [details]
Patch

[Jon Lee]

New patch to address Alexey's comments, and to make the bots happy

[Gustavo Noronha (kov)]

Comment on attachment 108387 [details]
Patch

Attachment 108387 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9799327

[Jeff Miller]

Comment on attachment 108387 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.h:265
> +    void setInitialFocus(bool, bool, const WebKeyboardEvent&);

Since this now takes multiple bool parameters, it probably makes sense to include the parameter names for the bools in the header to avoid confusion.

> Source/WebKit2/UIProcess/win/WebView.h:82
> +    void setInitialFocus(bool, bool, const WebKeyboardEvent&);

Ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:471
> +    void setInitialFocus(bool, bool, const WebKeyboardEvent&);

Ditto.

[Jon Lee]

Created attachment 108414 [details]
Patch

[Jon Lee]

Update patch to include Jeff's comments and also fix naming in WKTR-- inconsistency between addChromeTextField and addChromeInputField.  Opted for the latter.

[Daniel Bates]

Comment on attachment 108414 [details]
Patch

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

I did not review this patch for correctness. I suggest someone more knowledgable about this area of the code look through this patch. I have some minor nits.

I am r-'ing this patch since you need to update the WebPage::setInitialFocus() call site in Source/WebKit2/UIProcess/API/C/win/WKView.cpp as pointed out in the output of the Windows EWS bot:

7>..\UIProcess\API\C\win\WKView.cpp(72) : error C2660: 'WebKit::WebView::setInitialFocus' : function does not take 1 arguments

> LayoutTests/fast/forms/focus-option-control-on-page-expected.txt:8
> +<https://bugs.webkit.org/show_bug.cgi?id=68412> This test checks to see if option(alt)-tabbing properly focuses form elements that are normally not focused. For testing, the assumption is that by default pressing tab will skip over buttons, and option-tab will include buttons.
> +
> +  
> +Tab:
> +0:2
> +1:
> +2:2
> +3:

I'm unclear how to interpret these results to determine whether this test passed or failed. The test description doesn't seem to elaborate on how to tell whether this test passed or failed.

I suggest either writing some more english to explain how to interpret these results or, even better, write make the output more straight forward to understand by using a "PASS" and "FAIL" style output. One example of "PASS"/"FAIL"-style output can be seen in the test LayoutTests/fast/forms/menulist-submit-without-selection.html: <http://trac.webkit.org/export/95777/trunk/LayoutTests/fast/forms/menulist-submit-without-selection.html>.

> LayoutTests/fast/forms/focus-option-control-on-page.html:1
> +<p>&lt;https://bugs.webkit.org/show_bug.cgi?id=68412&gt; This test checks to see if option(alt)-tabbing properly focuses form elements that are normally not focused. For testing, the assumption is that by default pressing tab will skip over buttons, and option-tab will include buttons.</p>

Nit: For new layout tests we tend to include a DOCTYPE, <html> and <body> tags.

> LayoutTests/fast/forms/focus-option-control-on-page.html:7
> +<p id="log"></p>

Can we write this test using the js-test constructs provided by LayoutTests/fast/js/resources/js-test-{pre, post}.js? This will make this test more consistent with other DRT tests in this directory. These scripts also provide machinery to make PASS/FAIL-style tests. See my comment on focus-option-control-on-page-expected.txt (above) for an example test case that uses this machinery.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1319
> +    

Did you mean to add this tab-indented empty line? Having one empty line seems sufficient to visually separate the early returns from the actual body of this function. I suggest removing this line.

> Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:1208
> +    NSTextField* textField = [[NSTextField alloc] initWithFrame:NSMakeRect(0, 0, 100, 20)];

Nit: As far as I know, we put the '*' on the right in Objective-C/C++ code.

[Jon Lee]

Created attachment 108497 [details]
Patch

[Jon Lee]

Thanks for the feedback. I've uploaded a new patch that takes into account your comments, and I refactored the test to follow the mold of other tests in that directory.

In terms of style re: pointers, there is inconsistency, so I adjusted those also.

[WebKit Review Bot]

Comment on attachment 108497 [details]
Patch

Attachment 108497 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9840117

New failing tests:
fast/forms/focus-option-control-on-page.html

[Jon Lee]

Created attachment 108725 [details]
Patch

[WebKit Review Bot]

Comment on attachment 108725 [details]
Patch

Attachment 108725 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9883023

New failing tests:
fast/forms/focus-option-control-on-page.html

[Jon Lee]

Created attachment 108750 [details]
Patch

[WebKit Review Bot]

Comment on attachment 108750 [details]
Patch

Attachment 108750 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9877230

New failing tests:
fast/forms/focus-option-control-on-page.html

[Dimitri Glazkov (Google)]

(In reply to comment #18)
> (From update of attachment 108750 [details])
> Attachment 108750 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/9877230
> 
> New failing tests:
> fast/forms/focus-option-control-on-page.html

Is this a Mac-specific behavior? Perhaps the test should live in platform/mac?

If not, I think it's totally fine to add this test to platform/chromium/test_expectations.txt and file a bug against it.

[Jon Lee]

I mistakenly thought this was something cross-platform, but it looks like a Mac-specific behavior (see EventHandler::tabsToAllFormControls), so I will move the test.

[Dimitri Glazkov (Google)]

(In reply to comment #20)
> I mistakenly thought this was something cross-platform, but it looks like a Mac-specific behavior (see EventHandler::tabsToAllFormControls), so I will move the test.

SGTM!

[Jon Lee]

Created attachment 108894 [details]
Patch

[Anders Carlsson]

Comment on attachment 108894 [details]
Patch

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

> LayoutTests/platform/mac/fast/forms/script-tests/focus-option-control-on-page.js:57
> +    setTimeout(function() {layoutTestController.removeChromeInputField(notifyDone)}, 2000);

Why is this here? Will this always be called or only when the test times out?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1326
> +    } else

You should use an early return instead of an else here.

[Jon Lee]

Comment on attachment 108894 [details]
Patch

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

>> LayoutTests/platform/mac/fast/forms/script-tests/focus-option-control-on-page.js:57
>> +    setTimeout(function() {layoutTestController.removeChromeInputField(notifyDone)}, 2000);
> 
> Why is this here? Will this always be called or only when the test times out?

Yes, the intent is for this to always call in the case of a time out. The test should complete successfully within 2 seconds. If it doesn't, then we make sure to remove the input field widget, and call notifyDone.

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1326
>> +    } else
> 
> You should use an early return instead of an else here.

Ok, done.

[Jon Lee]

Created attachment 109562 [details]
Patch

[Anders Carlsson]

(In reply to comment #24)
> (From update of attachment 108894 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108894&action=review
> 
> >> LayoutTests/platform/mac/fast/forms/script-tests/focus-option-control-on-page.js:57
> >> +    setTimeout(function() {layoutTestController.removeChromeInputField(notifyDone)}, 2000);
> > 
> > Why is this here? Will this always be called or only when the test times out?
> 
> Yes, the intent is for this to always call in the case of a time out. The test should complete successfully within 2 seconds. If it doesn't, then we make sure to remove the input field widget, and call notifyDone.
> 

Both DumpRenderTree and WebKitTestRunner have built-in facilities for detecting a timeout, so I don't think this is needed.

[Jon Lee]

Created attachment 109587 [details]
Patch

[Darin Adler]

Comment on attachment 109587 [details]
Patch

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

> Source/WebKit2/ChangeLog:53
> [phone]  Jon Lee  <[e-mail]>
> +
> +        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;

[Jon Lee]

(In reply to comment #28)
> > 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.

Filed bug 69362 to fix this.

The rest of the cosmetic and refactoring changes have been made.

[Jon Lee]

Committed r96645: <http://trac.webkit.org/changeset/96645>