Command/control-R should not be the gesture that allows a beforeunload dialog |
|||||||||||
Issue descriptionIf the user types command/control+Q/W/T/N/R, that command is counted as a user gesture. That's not what we want for projects like requiring a user gesture for beforeunload dialogs ( issue 707007 ) and other things that gate power features on user gesture, as those commands aren't an intent by the user to interact with the page and shouldn't count as such. (Mozilla has this issue with their corresponding implementation of user gesture; see https://bugzilla.mozilla.org/show_bug.cgi?id=1345830 .) In case you're wondering how to get the focus in a page without a user gesture so that there's the case that there's not already a user gesture, try <input autofocus>. Repro: - Open the attached file - Type command/control-W - If you have logging you'll see that the command key creates a user gesture - If you have the patch https://codereview.chromium.org/2801813005/ installed, you'll still get the beforeunload dialog
,
Apr 8 2017
The beforeunload change is targeting M60; this should too.
,
Apr 8 2017
In dropping a log statement in DocumentUserGestureToken::create, here are problematic gesture creations: 1. On the press and on the release of the command key 2. On the press and on the release of the R key (for some command key combination, like Q, W, T, and N, we don't get those secondary gesture creations, as it seems like those are handled in the browser) It's that initial modifiers-changed event that is the one that really causes us problems with the beforeunload dialog. I'd like to see #2 fixed, but I propose that modifiers-changed events not being gestures would solve most our problems here.
,
Apr 8 2017
Woot! isBrowserShortcut on the event is set for the case of #2, so I can fix both. Patch soon.
,
May 11 2017
isBrowserShortcut (or as it's known post-Renaming is_browser_shortcut) is broken currently. There will be two patches. The first is just to prevent modifiers from making gestures, which fixes Command+W, where the W is held in the browser. The second will be to fix is_browser_shortcut, and then we can use that.
,
May 11 2017
,
May 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/42fad93f54e4ee681975517e2831614b5dbade05 commit 42fad93f54e4ee681975517e2831614b5dbade05 Author: avi <avi@chromium.org> Date: Thu May 18 18:16:27 2017 Don't count modifiers-changed events as being user gestures. BUG=709765 TEST=as in bug Review-Url: https://codereview.chromium.org/2874263002 Cr-Commit-Position: refs/heads/master@{#472876} [modify] https://crrev.com/42fad93f54e4ee681975517e2831614b5dbade05/content/child/blink_platform_impl.cc [modify] https://crrev.com/42fad93f54e4ee681975517e2831614b5dbade05/content/child/blink_platform_impl.h [modify] https://crrev.com/42fad93f54e4ee681975517e2831614b5dbade05/third_party/WebKit/Source/core/input/KeyboardEventManager.cpp [modify] https://crrev.com/42fad93f54e4ee681975517e2831614b5dbade05/third_party/WebKit/public/platform/Platform.h
,
Jun 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/56f9a641544d2cafa37142545bb39254f7bd98b3 commit 56f9a641544d2cafa37142545bb39254f7bd98b3 Author: avi <avi@chromium.org> Date: Tue Jun 27 15:01:49 2017 Don't count browser shortcut events as being user gestures. BUG=709765 TEST=as in bug Review-Url: https://codereview.chromium.org/2957923002 Cr-Commit-Position: refs/heads/master@{#482626} [modify] https://crrev.com/56f9a641544d2cafa37142545bb39254f7bd98b3/third_party/WebKit/Source/core/input/KeyboardEventManager.cpp
,
Jun 27 2017
Calling this fixed; splitting off the need to have a test for is_browser_shortcut into bug 737122.
,
Aug 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8b750849271adebe4dc164d1101ccaaa3790f922 commit 8b750849271adebe4dc164d1101ccaaa3790f922 Author: Avi Drissman <avi@chromium.org> Date: Thu Aug 10 21:18:26 2017 Revert "Don't count browser shortcut events as being user gestures." This reverts commit 56f9a641544d2cafa37142545bb39254f7bd98b3. The lack of a user gesture for browser shortcut events causes web compatibility issues. This needs a better-engineered approach. BUG=709765, 749005 , 753612 , 741612 TEST=as in bugs Change-Id: I2faee9623597561297e01756b4bdf6f56c049eeb Reviewed-on: https://chromium-review.googlesource.com/610829 Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#493549} [modify] https://crrev.com/8b750849271adebe4dc164d1101ccaaa3790f922/third_party/WebKit/Source/core/input/KeyboardEventManager.cpp
,
Aug 10 2017
The change was reverted; we'll need to find a better way of doing this.
,
Aug 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0234225c87bd459b2039d642921e9a7ad502eb89 commit 0234225c87bd459b2039d642921e9a7ad502eb89 Author: Avi Drissman <avi@chromium.org> Date: Fri Aug 11 17:35:51 2017 Revert "Don't count browser shortcut events as being user gestures." This reverts commit 56f9a641544d2cafa37142545bb39254f7bd98b3. The lack of a user gesture for browser shortcut events causes web compatibility issues. This needs a better-engineered approach. BUG=709765, 749005 , 753612 , 741612 TEST=as in bugs TBR=avi@chromium.org Change-Id: I2faee9623597561297e01756b4bdf6f56c049eeb Reviewed-on: https://chromium-review.googlesource.com/610829 Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#493549}(cherry picked from commit 8b750849271adebe4dc164d1101ccaaa3790f922) Reviewed-on: https://chromium-review.googlesource.com/611668 Reviewed-by: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/branch-heads/3163@{#490} Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528} [modify] https://crrev.com/0234225c87bd459b2039d642921e9a7ad502eb89/third_party/WebKit/Source/core/input/KeyboardEventManager.cpp
,
Aug 16 2017
Tested in chrome latest beta # 61.0.3163.49 win 10.0,Mac 10.12.6 & Ubuntu 14.04. As per comment#0 followed the below steps 1.Open the attached file 2.Type command/control-W Automatically browser window closed.Unable to find the exact behavior of the issue. @avi: Could you please provide us with a sample test steps of the issue which would help us to verify the issue further from TE end. Thanks in Advance.
,
Aug 16 2017
To test: 1. Load the attached file. 2. Type command-R or control-R What _should_ happen is that the page refreshes with no dialog. What happens now (because the change was reverted) is that the page is allowed to show a dialog. I need to work on a new way of accomplishing what I want to do.
,
Aug 16 2017
Re comment 14, to be clear, between steps 1 and 2, be super careful to not click in the page or type in the page or do *anything* between those steps.
,
Aug 16 2017
Verified the fix on Chrome version 61.0.3163.49 on Windows 7,10, Mac and Linux, below are the steps followed : 1. Visit the test page from comment#14 2. without any interaction within the page type command-R or control-R Observed behavior : "Do you want to reload this site?" dialog shows up.
,
Aug 16 2017
Correct, because as you may have noticed in comment 12, the fix for this was reverted. This bug is not marked as fixed because it is not fixed.
,
Jan 9 2018
,
Feb 27 2018
Issue 814349 has been merged into this issue.
,
May 8 2018
,
Aug 1
,
Jan 3
avi@: Wondering if it's time to revisit this after UAv2, possibly for Issue 848778. We seemed to have 3 bugs with the past attempt: ctrl-o (in devtools), ctrl-c and backspace. Looks like these shortcuts can be overridden in JS, is this spec compliant?
,
Jan 3
"Looks like these shortcuts can be overridden in JS, is this spec compliant?" The general answer to the question of, "Is this terrible thing that web browsers do relied on by lots of important web pages and therefore unbreakable?" is almost always "yes", spec or not. 😢 I'd love some improvement here but it's been quite a while and my mental state here is all swapped out, so I'm not sure how to reply to that bug. I'm hoping that with UAv2 we can be smarter than we were with my first attempt to fix this bug. |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by a...@chromium.org
, Apr 8 2017