Bug 68412 - REGRESSION (WK2): (Shift-)option-tabbing skips over elements when transitioning from chrome to webview
Summary: REGRESSION (WK2): (Shift-)option-tabbing skips over elements when transitioni...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.7
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on:
Blocks: 69362
  Show dependency treegraph
 
Reported: 2011-09-19 16:41 PDT by Jon Lee
Modified: 2011-10-04 14:22 PDT (History)
9 users (show)

See Also:


Attachments
Patch (30.02 KB, patch)
2011-09-22 11:35 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (34.93 KB, patch)
2011-09-22 13:11 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (34.95 KB, patch)
2011-09-22 15:52 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (42.30 KB, patch)
2011-09-23 11:23 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (42.12 KB, patch)
2011-09-26 14:41 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (42.08 KB, patch)
2011-09-26 16:19 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (42.33 KB, patch)
2011-09-27 14:14 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (42.18 KB, patch)
2011-10-03 17:49 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (44.02 KB, patch)
2011-10-04 00:26 PDT, Jon Lee
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2011-09-19 16:41:36 PDT
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>
Comment 1 Jon Lee 2011-09-22 11:35:40 PDT
Created attachment 108370 [details]
Patch
Comment 2 Collabora GTK+ EWS bot 2011-09-22 11:52:55 PDT
Comment on attachment 108370 [details]
Patch

Attachment 108370 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9795325
Comment 3 Alexey Proskuryakov 2011-09-22 11:56:19 PDT
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.
Comment 4 Darin Adler 2011-09-22 12:17:41 PDT
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.
Comment 5 Jon Lee 2011-09-22 13:11:22 PDT
Created attachment 108387 [details]
Patch
Comment 6 Jon Lee 2011-09-22 13:12:06 PDT
New patch to address Alexey's comments, and to make the bots happy
Comment 7 Gustavo Noronha (kov) 2011-09-22 15:35:36 PDT
Comment on attachment 108387 [details]
Patch

Attachment 108387 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9799327
Comment 8 Jeff Miller 2011-09-22 15:38:57 PDT
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.
Comment 9 Jon Lee 2011-09-22 15:52:28 PDT
Created attachment 108414 [details]
Patch
Comment 10 Jon Lee 2011-09-22 15:53:33 PDT
Update patch to include Jeff's comments and also fix naming in WKTR-- inconsistency between addChromeTextField and addChromeInputField.  Opted for the latter.
Comment 11 Daniel Bates 2011-09-22 20:25:02 PDT
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.
Comment 12 Jon Lee 2011-09-23 11:23:27 PDT
Created attachment 108497 [details]
Patch
Comment 13 Jon Lee 2011-09-23 11:24:37 PDT
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.
Comment 14 WebKit Review Bot 2011-09-23 19:38:47 PDT
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
Comment 15 Jon Lee 2011-09-26 14:41:06 PDT
Created attachment 108725 [details]
Patch
Comment 16 WebKit Review Bot 2011-09-26 16:05:02 PDT
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
Comment 17 Jon Lee 2011-09-26 16:19:56 PDT
Created attachment 108750 [details]
Patch
Comment 18 WebKit Review Bot 2011-09-27 00:19:50 PDT
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
Comment 19 Dimitri Glazkov (Google) 2011-09-27 13:38:29 PDT
(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.
Comment 20 Jon Lee 2011-09-27 14:00:43 PDT
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.
Comment 21 Dimitri Glazkov (Google) 2011-09-27 14:02:45 PDT
(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!
Comment 22 Jon Lee 2011-09-27 14:14:07 PDT
Created attachment 108894 [details]
Patch
Comment 23 Anders Carlsson 2011-10-03 17:07:39 PDT
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.
Comment 24 Jon Lee 2011-10-03 17:43:40 PDT
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.
Comment 25 Jon Lee 2011-10-03 17:49:33 PDT
Created attachment 109562 [details]
Patch
Comment 26 Anders Carlsson 2011-10-03 17:58:41 PDT
(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.
Comment 27 Jon Lee 2011-10-04 00:26:01 PDT
Created attachment 109587 [details]
Patch
Comment 28 Darin Adler 2011-10-04 11:40:37 PDT
Comment on attachment 109587 [details]
Patch

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

> Source/WebKit2/ChangeLog:53
> +2011-09-23  Jon Lee  <jonlee@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;
Comment 29 Jon Lee 2011-10-04 12:54:21 PDT
(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.
Comment 30 Jon Lee 2011-10-04 14:22:36 PDT
Committed r96645: <http://trac.webkit.org/changeset/96645>