New issue
Advanced search Search tips

Issue 668969 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 2
Type: Bug-Regression



Sign in to add a comment

onKeyup event is broken for PrintScreen key in the latest dev version

Reported by shima...@gmail.com, Nov 28 2016

Issue description

UserAgent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.3 Safari/537.36

Steps to reproduce the problem:
1. Access to the web page: 
http://www.broadband-xp.com/test/chrome/onkeyup.html
2. Press any key and you will see an alert with keycode.
3. Press PrintScreen key.

What is the expected behavior?
After pressing printscreen key, it should show "44".

What went wrong?
After pressing printscreen key, it does not show anything.

Did this work before? Yes 56.0.2906.0

Chrome version: 56.0.2924.3  Channel: dev
OS Version: 6.1 (Windows 7, Windows Server 2008 R2)
Flash Version: Shockwave Flash 23.0 r0
 

Comment 1 by ajha@chromium.org, Nov 28 2016

Labels: Needs-Bisect M-56

Comment 2 by rbyers@chromium.org, Nov 28 2016

Components: Platform>DevTools

Comment 3 by rbyers@chromium.org, Nov 28 2016

Components: -Platform>DevTools Blink>Input
Cc: rbasuvula@chromium.org
Labels: -M-56 -Needs-Bisect M-57
Status: Untriaged (was: Unconfirmed)
Tested the issue on chrome Stable #54.0.2840.99, Canary 57.0.2935.3 in Windows 10.0 and was able to reproduce the issue.

This is a Non-Regression issue since seeing this from M30 #30.0.1549.0, Making the status to Untriaged so that the issue would get addressed.

Note : Not able to reproduce the issue in MAC 10.11.6 and Linux Ubuntu 14.04.

Thank you.

Comment 5 by shima...@gmail.com, Nov 29 2016

@rbasuvula
This is a regression issue. I do not know why your 54.0.2840.99 does not show an alert message "44".
Now I am commenting from Windows 10 with Google Chrome 54.0.2840.99. Maybe my English is misleading.

How to reproduce.
1. Go to 
http://www.broadband-xp.com/test/chrome/onkeyup.html
with 54.0.2840.99 (stable)
2. Press "a" key. You will see an alert message "65", which is the keycode of "a".
If you do not see any message, the window just loses focus. Click any point within the window to get the focus.
3. Click OK to close an alert message.
4. Press PrintScreen key. You will see an alert message "44", which is the keycode of "PrintScreen" key.
5. Then with the dev version go to 
http://www.broadband-xp.com/test/chrome/onkeyup.html
6. Press "a" key. You will see an alert message "65", which is the keycode of "a".
If you do not see any message, the window just loses focus. Click any point within the window to get the focus. This is just fine.
7. Click OK to close an alert message.
8. Press PrintScreen key. You will see "No" alert message this time.

The point is "onkeyup" event.
onKeydown event is not triggered with PrintScreen key from the very beginning. I do not mention about it.

Thank you in advance.
@ shimax21 : Thanks for your update and clear steps.
Labels: hasbisect-per-revision
Owner: foolip@chromium.org
Status: Assigned (was: Untriaged)
Tested in chrome stable #54.0.2840.99, Beta #55.0.2883.59, Dev #56.2924.10 and canary #56.0.2936.0 on Win 10.0 and was able to reproduce the issue on Dev and canary builds.

Below are the Bisect Details:

Bisect Info:
=============
Good Build: 56.0.2914.0 (Revision - 430837)
Bad Build: 56.0.2915.0 (Revision - 431137)


Bisect URL:
=========== 
You are probably looking for a change made after 430938 (known good), but no later than 430939 (first known bad).
CHANGELOG URL:
https://chromium.googlesource.com/chromium/src/+log/3fbe4d26c6c6920babf2a83b3741b4e94ce689ba..35d322e24f91a372ecdc0b152891e0635187a07e

From the CL above, assigning the issue to the concern owner

@ foolip  : 
------------------
Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.

Review-Url: https://codereview.chromium.org/2482853002
Cc: dtapu...@chromium.org
Thank you for reporting this issue. I can confirm that https://codereview.chromium.org/2482853002 is the cause and that some changes like this were expected. I did not know that PrintScreen specifically would be affected.

Note that before this change, just the keyup was fired, not the keydown or keypress events. I would argue this was a bug, if there's any good reason to suppress the first two events than the last should also be suppressed.

http://www.broadband-xp.com/test/chrome/onkeyup.html is a test page, can you explain how you first came across this issue? Has it broken a real page?

If there's a good reason to suppress keydown and keypress then I think this issue has to be WontFix, and otherwise I think this fix would be to allow all of the events.

dtapuska@, would you happen to know why PrintScreen is suppressed in the first place?

Comment 9 by shima...@gmail.com, Dec 1 2016

I am myself working on developments of many intranet sites in Japan that disable PrintScreen key or display warning when it is suppressed. So I came to know this bug.
@ foolip :
I am sorry that I was not able to understand all of your English.
Before this change, to be sure onkeydown and onkeypress events were not fired for PrintScreen key. Only onkeyup event is fired.
But now after this change, not only onkeydown and onkyepress but also onkeyup keyup were not fired for PrintScreen key. So there is no way any more to detect PrintScreen key in Google Chrome. That is the problem.
No worries about the English, let's work this out :)

Do you mean that you're trying to prevent the user from using the print screen key by calling event.preventDefault() or similar, or do the sites react somehow to it without preventing it?
@foolip :
That's right. But I can not explain about how it works in detail. It is just secret.

Comment 12 by shima...@gmail.com, Dec 21 2016

@foolip :
Since Christmas is coming, you must be busy now, but how are things going on?
Is this difficult to fix this issue while addressing to the problem at the time of the full screen?
Gentle Ping !! 

Could we get any update on this issue.

This is on my list of things to take care of.  I haven't looked into it yet but I trust it can be fixed one way or another for M57.

Comment 15 by shima...@gmail.com, Jan 16 2017

I uploaded another test case in order to illustrate this problem from different perspectives.
http://www.broadband-xp.com/test/chrome/onkeyup_dom.html

In Google Chrome 55, as long as the window has a focus, PrintScreen key can always be detected. On the other hand, in Google Chrome 56 or above, once the window has lost the focus, getting back the focus is not enough. Instead a normal key such as "a" or "z" must be pressed in advance in order to detect PrintScreen key.

Labels: -M-57 M-56
Labels: ReleaseBlock-Stable
Labels: OS-Linux
https://jsfiddle.net/w8pmbgtn/ logs keydown, keypress and keyup events. Using this, it looks like Firefox also only fires a keyup event. Although a bare keyup event is pretty strange, preserving the old behavior and matching Firefox is probably best.

It looks as though the RawKeyDown and Char events actually never reach the code that I changed in https://codereview.chromium.org/2482853002, investigating now.
Edge also only fires a keyup event.
The culprit here is the RenderWidgetHostImpl::SuppressEventsUntilKeyDown() call from RenderWidgetHostViewAura::OnWindowFocused that happens on first load. Any bare keypress or keyup events will be dropped until there's a keydown event because of this.
My plan now is to exclude PrintScreen specifically from the suppression in a first CL, and then try to remove SuppressEventsUntilKeyDown() entirely in a follow-up which is more risky but seems like a better fix. It was introduced in https://codereview.chromium.org/12221133 but seemingly isn't needed any more in some ad-hoc testing.

To be continued tomorrow.
Labels: -M-56 M-57
Let's punt to M57, we're four days from cutting the first stable candidate for M56 and I don't think is severe for a merge to make sense.  We're generally only accepting high severity security fixes / crashes at this point.
#23, understood. OP, I'm sorry, but this means that the problem will be in Chrome 56. You reported this early on and it was confirmed to be a problem, but due to the wrong milestone label being applied I thought I could defer this for a while and still make it. Please accept my apologies for this situation.
In fixing this, I've noticed that on Linux, event.keyCode is 42, while on Windows it is 44. This was tested with the same USB keyboard. In both cases, however, event.key and event.code are "PrintScreen", so there is some mapping to harmonize the two platforms. Mac not tested.
Oops, on Linux, actually event.key is "Print", filed  issue 683097 .
Cc: sadrul@chromium.org
Reviews: https://codereview.chromium.org/2641313002/ and https://codereview.chromium.org/2643243002 (will send to sadrul@ as it touches aura)
Project Member

Comment 28 by bugdroid1@chromium.org, Jan 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e5fd75af8171718a84d387486cfba0ed225c6f5f

commit e5fd75af8171718a84d387486cfba0ed225c6f5f
Author: foolip <foolip@chromium.org>
Date: Wed Jan 25 15:24:33 2017

Remove RenderWidgetHostImpl::SuppressEventsUntilKeyDown()

This revert https://codereview.chromium.org/12221133 where the
suppression on focus was originally added. No tests were added, but the
manual test described now passes without this code. There is of course
some chance that it is needed for some other case, so this CL should be
revert at the first sign of trouble.

The reason for this change is that the suppression on focus causes the
PrintScreen KeyUp event to be dropped.

BUG= 668969 

Review-Url: https://codereview.chromium.org/2643243002
Cr-Commit-Position: refs/heads/master@{#446023}

[modify] https://crrev.com/e5fd75af8171718a84d387486cfba0ed225c6f5f/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/e5fd75af8171718a84d387486cfba0ed225c6f5f/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/e5fd75af8171718a84d387486cfba0ed225c6f5f/content/browser/renderer_host/render_widget_host_view_aura.cc

Labels: Merge-Request-57
Project Member

Comment 30 by sheriffbot@chromium.org, Jan 26 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 31 by bugdroid1@chromium.org, Jan 26 2017

Labels: -merge-approved-57 merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eef386e8b37005016c26e10e839e8984b98ad4da

commit eef386e8b37005016c26e10e839e8984b98ad4da
Author: Philip Jägenstedt <foolip@chromium.org>
Date: Thu Jan 26 17:50:27 2017

Remove RenderWidgetHostImpl::SuppressEventsUntilKeyDown()

This revert https://codereview.chromium.org/12221133 where the
suppression on focus was originally added. No tests were added, but the
manual test described now passes without this code. There is of course
some chance that it is needed for some other case, so this CL should be
revert at the first sign of trouble.

The reason for this change is that the suppression on focus causes the
PrintScreen KeyUp event to be dropped.

BUG= 668969 

Review-Url: https://codereview.chromium.org/2643243002
Cr-Commit-Position: refs/heads/master@{#446023}
(cherry picked from commit e5fd75af8171718a84d387486cfba0ed225c6f5f)

Review-Url: https://codereview.chromium.org/2652893012 .
Cr-Commit-Position: refs/branch-heads/2987@{#109}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/eef386e8b37005016c26e10e839e8984b98ad4da/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/eef386e8b37005016c26e10e839e8984b98ad4da/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/eef386e8b37005016c26e10e839e8984b98ad4da/content/browser/renderer_host/render_widget_host_view_aura.cc

Status: Fixed (was: Assigned)

Comment 33 by shima...@gmail.com, Jan 29 2017

I also confirmed that this bug had been fixed in the latest 57.0.2987.13(Official Build)dev (64bit).

Thank you so much for all of those who have contributed to the fix.
 Issue 686685  has been merged into this issue.

Sign in to add a comment