Bug 165830 – Add a setting to suppress keyboard input during provisional navigation

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

Add a setting to suppress keyboard DOM events during provisional navigation

[Andy Estes]

Created attachment 297051 [details]
Patch

[Andy Estes]

rdar://problem/26929398

[Andy Estes]

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.

[Andy Estes]

Created attachment 297054 [details]
Patch

[Andy Estes]

Created attachment 297055 [details]
Patch

[Andreas Kling]

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?

[Andy Estes]

(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.

[Andy Estes]

Created attachment 297132 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Andy Estes]

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.

[Brent Fulgham]

(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.

[Brent Fulgham]

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.

[Andy Estes]

(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,

[Andy Estes]

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!

[Ryosuke Niwa]

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...

[Ryosuke Niwa]

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.

[Andy Estes]

(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.

[Andy Estes]

(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.

[Andy Estes]

Created attachment 297355 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 297355 [details]
Patch

Clearing flags on attachment: 297355

Committed r209943: <http://trac.webkit.org/changeset/209943>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

(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.