Bug 169995 - CMD+R / CMD+Q is considered as user interaction and beforeunload alert is shown
Summary: CMD+R / CMD+Q is considered as user interaction and beforeunload alert is shown
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 169936
Blocks: 178183
  Show dependency treegraph
 
Reported: 2017-03-22 22:20 PDT by Chris Dumez
Modified: 2017-10-11 12:43 PDT (History)
14 users (show)

See Also:


Attachments
Patch (25.80 KB, patch)
2017-03-22 22:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (25.80 KB, patch)
2017-03-22 22:32 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (1.06 MB, application/zip)
2017-03-22 23:51 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (1.90 MB, application/zip)
2017-03-23 00:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.37 MB, application/zip)
2017-03-23 00:06 PDT, Build Bot
no flags Details
Patch (25.62 KB, patch)
2017-03-23 08:53 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (25.52 KB, patch)
2017-04-03 12:21 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (25.09 KB, patch)
2017-04-12 19:02 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (6.25 KB, patch)
2017-04-13 10:47 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.39 KB, patch)
2017-04-13 15:38 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (14.15 MB, application/zip)
2017-04-13 17:28 PDT, Build Bot
no flags Details
Patch (12.18 KB, patch)
2017-04-13 18:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-03-22 22:20:00 PDT
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.
Comment 1 Chris Dumez 2017-03-22 22:20:08 PDT
<rdar://problem/23798897>
Comment 2 Chris Dumez 2017-03-22 22:29:07 PDT
Created attachment 305166 [details]
Patch
Comment 3 Chris Dumez 2017-03-22 22:32:14 PDT
Created attachment 305167 [details]
Patch
Comment 4 Build Bot 2017-03-22 23:51:42 PDT
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
Comment 5 Build Bot 2017-03-22 23:51:46 PDT
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
Comment 6 Build Bot 2017-03-23 00:01:08 PDT
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
Comment 7 Build Bot 2017-03-23 00:01:12 PDT
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
Comment 8 Build Bot 2017-03-23 00:06:05 PDT
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
Comment 9 Build Bot 2017-03-23 00:06:08 PDT
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
Comment 10 Chris Dumez 2017-03-23 08:53:04 PDT
Created attachment 305197 [details]
Patch
Comment 11 Sam Weinig 2017-03-24 05:35:22 PDT
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?
Comment 12 Chris Dumez 2017-03-24 07:28:11 PDT
(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?
Comment 13 Chris Dumez 2017-03-24 12:15:43 PDT
(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).
Comment 14 Chris Dumez 2017-03-30 15:03:08 PDT
ping?
Comment 15 Chris Dumez 2017-04-03 12:13:48 PDT
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.
Comment 16 Chris Dumez 2017-04-03 12:21:43 PDT
Created attachment 306093 [details]
Patch
Comment 17 Geoffrey Garen 2017-04-11 21:18:44 PDT
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.
Comment 18 Geoffrey Garen 2017-04-12 08:30:14 PDT
(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. :( )
Comment 19 Chris Dumez 2017-04-12 19:02:24 PDT
Created attachment 306961 [details]
Patch
Comment 20 Chris Dumez 2017-04-13 10:47:15 PDT
Created attachment 306995 [details]
WIP Patch
Comment 21 Chris Dumez 2017-04-13 15:38:05 PDT
Created attachment 307037 [details]
Patch
Comment 22 Chris Dumez 2017-04-13 15:39:13 PDT
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.
Comment 23 Build Bot 2017-04-13 17:28:12 PDT
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
Comment 24 Build Bot 2017-04-13 17:28:14 PDT
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
Comment 25 Chris Dumez 2017-04-13 18:03:30 PDT
Created attachment 307060 [details]
Patch
Comment 26 Sam Weinig 2017-04-16 17:26:29 PDT
Comment on attachment 307060 [details]
Patch

Nice. r=me
Comment 27 WebKit Commit Bot 2017-04-16 17:56:50 PDT
Comment on attachment 307060 [details]
Patch

Clearing flags on attachment: 307060

Committed r215404: <http://trac.webkit.org/changeset/215404>
Comment 28 WebKit Commit Bot 2017-04-16 17:56:52 PDT
All reviewed patches have been landed.  Closing bug.