CMD+R / CMD+Q is considered as user interaction and beforeunload alert is shown

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

CMD+R / CMD+Q is considered as user interaction and beforeunload alert is shown. To address this issue, Firefox seems to treat keyUp keyboard events as user interaction with the page, but not keyDown ones. We should probably do the same.

[Chris Dumez]

<rdar://problem/23798897>

[Chris Dumez]

Created attachment 305166 [details]
Patch

[Chris Dumez]

Created attachment 305167 [details]
Patch

[Build Bot]

Comment on attachment 305167 [details]
Patch

Attachment 305167 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3392801

New failing tests:
editing/mac/input/kotoeri-enter-to-confirm-and-newline.html
fast/events/js-keyboard-event-creation.html
fast/events/arrow-keys-on-body.html
editing/selection/editable-div-clear-on-keydown.html
fast/events/key-events-in-input-button.html
fast/events/selectionchange-user-initiated.html
fast/events/beforeunload-alert-user-interaction.html
fast/html/details-keyboard-show-hide.html
fast/forms/radio/radio_checked_name.html
fast/events/onchange-setvalue.html
accessibility/mac/checkbox-posts-value-change-notification-after-activation-with-space.html
http/tests/navigation/keyboard-events-during-provisional-navigation.html
fast/events/special-key-events-in-input-text.html
fast/forms/button-spacebar-click.html
fast/forms/submit-to-blank-multiple-times.html
fast/events/keydown-function-keys.html
fast/frames/focus-controller-crash-change-event.html
platform/mac/fast/events/objc-event-api.html
fast/events/keyboardevent-code.html
fast/events/key-events-in-input-text.html
fast/forms/input-image-submit.html
fast/forms/enter-clicks-buttons.html
fast/events/keyboardevent-key.html

[Build Bot]

Created attachment 305170 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 305167 [details]
Patch

Attachment 305167 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3392810

New failing tests:
editing/mac/input/kotoeri-enter-to-confirm-and-newline.html
fast/events/js-keyboard-event-creation.html
fast/events/arrow-keys-on-body.html
editing/selection/editable-div-clear-on-keydown.html
fast/events/key-events-in-input-button.html
fast/events/selectionchange-user-initiated.html
fast/events/beforeunload-alert-user-interaction.html
fast/html/details-keyboard-show-hide.html
fast/forms/radio/radio_checked_name.html
fast/events/onchange-setvalue.html
accessibility/mac/checkbox-posts-value-change-notification-after-activation-with-space.html
http/tests/navigation/keyboard-events-during-provisional-navigation.html
fast/events/special-key-events-in-input-text.html
fast/forms/button-spacebar-click.html
fast/forms/submit-to-blank-multiple-times.html
fast/events/keydown-function-keys.html
fast/frames/focus-controller-crash-change-event.html
platform/mac/fast/events/objc-event-api.html
fast/events/keyboardevent-code.html
fast/events/key-events-in-input-text.html
fast/forms/input-image-submit.html
fast/forms/enter-clicks-buttons.html
fast/events/keyboardevent-key.html

[Build Bot]

Created attachment 305171 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 305167 [details]
Patch

Attachment 305167 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3392849

New failing tests:
http/tests/navigation/keyboard-events-during-provisional-navigation.html
fast/forms/submit-to-blank-multiple-times.html
fast/events/selectionchange-user-initiated.html
fast/events/js-keyboard-event-creation.html
fast/forms/radio/radio_checked_name.html
fast/events/onchange-setvalue.html
fast/events/arrow-keys-on-body.html
fast/forms/button-spacebar-click.html
fast/frames/focus-controller-crash-change-event.html
fast/html/details-keyboard-show-hide.html
fast/events/keyboardevent-code.html
accessibility/mac/checkbox-posts-value-change-notification-after-activation-with-space.html
fast/events/beforeunload-alert-user-interaction.html
fast/events/special-key-events-in-input-text.html
fast/events/key-events-in-input-button.html
fast/events/keydown-function-keys.html
fast/events/key-events-in-input-text.html
fast/forms/input-image-submit.html
editing/selection/editable-div-clear-on-keydown.html
fast/forms/enter-clicks-buttons.html
fast/events/keyboardevent-key.html

[Build Bot]

Created attachment 305172 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 305197 [details]
Patch

[Sam Weinig]

Instead of differentiating between keyup/keydown, could we instead find out if the event was handled at all by the page and base it off that?

[Chris Dumez]

(In reply to Sam Weinig from comment #11)
> Instead of differentiating between keyup/keydown, could we instead find out
> if the event was handled at all by the page and base it off that?

Probably, I was trying to match Firefox's behavior though since they are the only ones doing this at the moment.

Also wouldn't this mean that we would show the alert on CMD+Q of the body had a keydown handler?

[Chris Dumez]

(In reply to Chris Dumez from comment #12)
> (In reply to Sam Weinig from comment #11)
> > Instead of differentiating between keyup/keydown, could we instead find out
> > if the event was handled at all by the page and base it off that?
> 
> Probably, I was trying to match Firefox's behavior though since they are the
> only ones doing this at the moment.
> 
> Also wouldn't this mean that we would show the alert on CMD+Q of the body
> had a keydown handler?

Took a quick look to see how easy to see if the key event was handled. I looked at the boolean returned by dispatchEvent() and event.wasDefaultHandled() flags but they did not seem to help recognize the case where the user pressed a key in an editable field (which does not have an event handler).

[Chris Dumez]

ping?

[Chris Dumez]

Corresponding Gecko code:
    nsCOMPtr<nsINode> node = do_QueryInterface(aTargetContent);
    if (node &&
        (aEvent->mMessage == eKeyUp || aEvent->mMessage == eMouseUp ||
         aEvent->mMessage == eWheel || aEvent->mMessage == eTouchEnd ||
         aEvent->mMessage == ePointerUp)) {
      nsIDocument* doc = node->OwnerDoc();
      while (doc && !doc->UserHasInteracted()) {
        doc->SetUserHasInteracted(true);
        doc = nsContentUtils::IsChildOfSameType(doc) ?
          doc->GetParentDocument() : nullptr;
      }
    }

So they indeed treat KeyUp as user interaction with the document but not KeyDown.

[Chris Dumez]

Created attachment 306093 [details]
Patch

[Geoffrey Garen]

Comment on attachment 306093 [details]
Patch

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

r=me

> Source/WebCore/dom/Document.h:1156
> +    void markUserDidInteractWithPage() { m_userDidInteractWithPage = true; }

I would just use normal "set" syntax here: setUserDidInteractWithPage(bool).

> Source/WebCore/page/EventHandler.cpp:3153
> +    // Delay updating document flag indicating the user has interacted with the page until the KeyUp so we do not
> +    // treat keyboard shortcuts such as CMD+R / CMD+Q as user interaction with the page.

It's odd to special case Q and R. There are other user actions that we don't treat as interaction with the page -- such as Cmd-L to focus the location field and change location. I think it would be better to use the set of all key combinations that Safari treats as special.

[Geoffrey Garen]

(In reply to Geoffrey Garen from comment #17)
> It's odd to special case Q and R.

Ricky reminded me that this patch just implements the logic to wait for keyUp, not to select any specific keystrokes. (I noticed that the first time I read this patch, and then forgot. :( )

[Chris Dumez]

Created attachment 306961 [details]
Patch

[Chris Dumez]

Created attachment 306995 [details]
WIP Patch

[Chris Dumez]

Created attachment 307037 [details]
Patch

[Chris Dumez]

New (more reliable) approached based on Sam's suggestion. We now only treat as user interaction with the page key events that were actually handled.

[Build Bot]

Comment on attachment 307037 [details]
Patch

Attachment 307037 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3531085

New failing tests:
fast/events/beforeunload-alert-handled-keydown.html

[Build Bot]

Created attachment 307057 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 307060 [details]
Patch

[Sam Weinig]

Comment on attachment 307060 [details]
Patch

Nice. r=me

[WebKit Commit Bot]

Comment on attachment 307060 [details]
Patch

Clearing flags on attachment: 307060

Committed r215404: <http://trac.webkit.org/changeset/215404>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.