Add a setting to suppress keyboard DOM events during provisional navigation
Created attachment 297051 [details] Patch
rdar://problem/26929398
Comment on attachment 297051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297051&action=review > Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:458 > +WK_EXPORT bool WKPreferencesShouldSuppressKeyboardDOMEventsDuringProvisionalNavigation(WKPreferencesRef); Oops, this should have a "Get" in the name.
Created attachment 297054 [details] Patch
Created attachment 297055 [details] Patch
Comment on attachment 297055 [details] Patch In EventHandler::keyEvent(), if (initialKeyEvent.type() == PlatformEvent::RawKeyDown), we may dispatch the keydown event without ever calling stopPropagation() on it. Do we need to cover that code path as well?
(In reply to comment #6) > Comment on attachment 297055 [details] > Patch > > In EventHandler::keyEvent(), if (initialKeyEvent.type() == > PlatformEvent::RawKeyDown), we may dispatch the keydown event without ever > calling stopPropagation() on it. > Do we need to cover that code path as well? Oh, good catch. From what I can tell, that path is for platforms that have separate keydown and keypress/char events, such as Windows. On Cocoa platforms we get a single event for "key down" that we disambiguate into separate keydown and keypress events, but since the initialKeyEvent is never RawKeyDown, these platforms won't take that path. I restructured things so that stopPropogation() is called on the keydown KeyboardEvent before we encounter that branch. I then added another call to stopPropogation() for the handledByInputMethod case, since we reassign keydown in that block. I'll also add an input method composition to my test.
Created attachment 297132 [details] Patch
Comment on attachment 297132 [details] Patch Attachment 297132 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2723878 New failing tests: http/tests/navigation/keyboard-events-during-provisional-navigation.html
Created attachment 297140 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 297132 [details] Patch Attachment 297132 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2723919 New failing tests: http/tests/navigation/keyboard-events-during-provisional-navigation.html
Created attachment 297142 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
The mac and mac-debug EWS failures are due to my new test failing with this error: CONSOLE MESSAGE: line 10: TypeError: internals.settings.setShouldSuppressKeyboardInputDuringProvisionalNavigation is not a function. (In 'internals.settings.setShouldSuppressKeyboardInputDuringProvisionalNavigation(true)', 'internals.settings.setShouldSuppressKeyboardInputDuringProvisionalNavigation' is undefined) FAIL: Timed out waiting for notifyDone to be called That suggests that EWS miscompiled the patch. On my local machine I have setShouldSuppressKeyboardInputDuringProvisionalNavigation() in InternalSettingsGenerated.h, and the test passes both in DumpRenderTree and WebKitTestRunner.
(In reply to comment #13) > The mac and mac-debug EWS failures are due to my new test failing with this > error: > > CONSOLE MESSAGE: line 10: TypeError: > internals.settings.setShouldSuppressKeyboardInputDuringProvisionalNavigation > is not a function. (In > 'internals.settings. > setShouldSuppressKeyboardInputDuringProvisionalNavigation(true)', > 'internals.settings. > setShouldSuppressKeyboardInputDuringProvisionalNavigation' is undefined) > FAIL: Timed out waiting for notifyDone to be called > > That suggests that EWS miscompiled the patch. On my local machine I have > setShouldSuppressKeyboardInputDuringProvisionalNavigation() in > InternalSettingsGenerated.h, and the test passes both in DumpRenderTree and > WebKitTestRunner. It's weird that both of these WK1 tests are unhappy. If mac-32bit is also WK1, then it seems much more likely that a bad build is at fault. If you are confident the WK1 code is correct go ahead and land it.
Comment on attachment 297132 [details] Patch I think this change looks good. I don't see any obvious reason why WK1 would be a problem. As far as I can tell, the new Internal settings changes are identical to other "known good" cases.
(In reply to comment #14) > (In reply to comment #13) > > The mac and mac-debug EWS failures are due to my new test failing with this > > error: > > > > CONSOLE MESSAGE: line 10: TypeError: > > internals.settings.setShouldSuppressKeyboardInputDuringProvisionalNavigation > > is not a function. (In > > 'internals.settings. > > setShouldSuppressKeyboardInputDuringProvisionalNavigation(true)', > > 'internals.settings. > > setShouldSuppressKeyboardInputDuringProvisionalNavigation' is undefined) > > FAIL: Timed out waiting for notifyDone to be called > > > > That suggests that EWS miscompiled the patch. On my local machine I have > > setShouldSuppressKeyboardInputDuringProvisionalNavigation() in > > InternalSettingsGenerated.h, and the test passes both in DumpRenderTree and > > WebKitTestRunner. > > It's weird that both of these WK1 tests are unhappy. If mac-32bit is also > WK1, then it seems much more likely that a bad build is at fault. I also thought that was suspicious, but I couldn't reproduce the failure locally in DRY,
Oops, saved too soon. I couldn't reproduce locally, nor could I see any obvious issue with the patch. I believe mac-32bit doesn't run tests, which is why we didn't see issues there. Simon also mentioned to me on IRC that there are known issues with InternalSettingsGenerated and incremental builds. I'll land this and monitor the bots for failures in case I need to roll out. Thanks for the review!
Comment on attachment 297132 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297132&action=review > Source/WebCore/dom/EventDispatcher.cpp:159 > + if (shouldSuppressEventDispatchInDOM(node, event)) > + event.stopPropagation(); > + I don’t think this is the right layer to handle this problem. Doing this in EventHandler as you attempted in a previous patch is better. e.g. with this approach, we may still try to select text, drag text, etc...
Source/WebCore/replay/UserInputBridge has a nice of user inputs you want to block. We should also block accessibility related user interactions initiated from accessibilityPerformAction in Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm and friends.
(In reply to comment #18) > Comment on attachment 297132 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=297132&action=review > > > Source/WebCore/dom/EventDispatcher.cpp:159 > > + if (shouldSuppressEventDispatchInDOM(node, event)) > > + event.stopPropagation(); > > + > > I don’t think this is the right layer to handle this problem. > Doing this in EventHandler as you attempted in a previous patch is better. Putting a check in EventHandler is insufficient to block all the events covered by the test case, so it's not clear to me how the previous patch was better. What's wrong with blocking event dispatch in EventDispatcher? > e.g. with this approach, we may still try to select text, drag text, etc... These actions are allowed by design. We're specifically targeting text input from the keyboard.
(In reply to comment #19) > Source/WebCore/replay/UserInputBridge has a nice of user inputs you want to > block. > > We should also block accessibility related user interactions initiated from > accessibilityPerformAction in > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm and > friends. As far as I can tell, these actions result in things like menu interactions, touch events, and access key invocations, not observable text input. Those things are outside the scope of what we're trying to fix with this bug.
Created attachment 297355 [details] Patch
Comment on attachment 297355 [details] Patch Clearing flags on attachment: 297355 Committed r209943: <http://trac.webkit.org/changeset/209943>
All reviewed patches have been landed. Closing bug.
Comment on attachment 297355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297355&action=review > Source/WebCore/dom/EventDispatcher.cpp:158 > if (is<HTMLInputElement>(node)) > downcast<HTMLInputElement>(node).willDispatchEvent(event, clickHandlingState); > > + if (shouldSuppressEventDispatchInDOM(node, event)) > + event.stopPropagation(); This would probably mean that checkbox state would have been updated already.
(In reply to comment #20) > (In reply to comment #18) > > Comment on attachment 297132 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=297132&action=review > > > > > Source/WebCore/dom/EventDispatcher.cpp:159 > > > + if (shouldSuppressEventDispatchInDOM(node, event)) > > > + event.stopPropagation(); > > > + > > > > I don’t think this is the right layer to handle this problem. > > Doing this in EventHandler as you attempted in a previous patch is better. > > Putting a check in EventHandler is insufficient to block all the events > covered by the test case, so it's not clear to me how the previous patch was > better. What's wrong with blocking event dispatch in EventDispatcher? The problem is that EventDispatcher is supposed to be a low level primitive the rest of WebCore uses, and I can’t reckon all the implications of checking FrameLoader’s state like this. For example, EventHandler has a bunch of states that get updated before events are dispatched. > > e.g. with this approach, we may still try to select text, drag text, etc... > > These actions are allowed by design. We're specifically targeting text input > from the keyboard. In that case, it would have been better to do this check in the default event handlers which are responsible for text inputs.