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>
Created attachment 108370 [details] Patch
Comment on attachment 108370 [details] Patch Attachment 108370 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9795325
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 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.
Created attachment 108387 [details] Patch
New patch to address Alexey's comments, and to make the bots happy
Comment on attachment 108387 [details] Patch Attachment 108387 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9799327
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.
Created attachment 108414 [details] Patch
Update patch to include Jeff's comments and also fix naming in WKTR-- inconsistency between addChromeTextField and addChromeInputField. Opted for the latter.
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><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.</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.
Created attachment 108497 [details] Patch
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 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
Created attachment 108725 [details] Patch
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
Created attachment 108750 [details] Patch
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
(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.
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.
(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!
Created attachment 108894 [details] Patch
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 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.
Created attachment 109562 [details] Patch
(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.
Created attachment 109587 [details] Patch
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;
(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.
Committed r96645: <http://trac.webkit.org/changeset/96645>