Issue metadata
Sign in to add a comment
|
Keyboard shortcuts don't work when a textbox is focused |
||||||||||||||||||||
Issue descriptionChrome Version: 71.0.3578.20 OS: Linux64 (GLinux Rodete Desktop), with i3 window manager. What steps will reproduce the problem? (1) Focus any textbox (either the URL-bar, or a textbox on the page). (2) Try a Chrome shortcut such as Ctrl-T, Ctrl-L or Ctrl-N. (3) Observe nothing happens. Notes: The shortcuts for copy-paste using Ctrl-C/Ctrl-X/Ctrl-V still work. Also, all shortcuts work when no textbox is focused. And this issue only affects desktop Linux. I bisected the bug exactly to http://crrev.com/595454 "[Reland 1] Use accurate X11 event timestamp computation." using the bisect python script.
,
Oct 30
Also unable to repro on 71.0.3578.20 beta. Not sure how your CL could be causing this issue.
,
Oct 30
+sadrul, in case he has any ideas.
,
Oct 30
Does this perhaps happen after hibernating the chrome/X11 session?
,
Nov 5
M71 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.
,
Nov 8
M71 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.
,
Nov 9
Since this only happens with textboxes, this suggests that the problem lies with sending events to the renderer. This CL only modifies the timestamp of the event [to make it more accurate]. I wonder if we have some logic that throws out keyboard events based on the timestamp? Not sure whether to leave this CL in or to revert...sadrul?
,
Nov 13
Reminder M71 Stable is approaching VERY soon. Please review this bug and assess if this is indeed a RBS. If not, please remove the RBS label. If so, please make sure any planned work will be tested in Beta and verified before the Stable date. Thank you. Requesting to take a look at M71 blockers ASAP due to upcoming Thanksgiving holidays next week.
,
Nov 15
Given lack of response from original poster, and our inability to repro, let's leave the CL merged.
,
Nov 16
Sorry I didn't realize you expected me to reply, since you didn't ask me any question. I can still repro consistently (no weird scenarios needed, happens after restarting X and Chrome), there are two independent report, I have an exact bisect and the original CL doesn't do anything useful, so all indications are that we should revert. I guess I'll redownload a Chrome instance and upload a revert to make sure this moves forward. With the low population of Linux users with unclear % of them impacted, it may not be worth emergency cherry-pick at this point so removing RBS, but I would like to fix it moving forward so I don't have to live with this annoying bug indefinitely.
,
Nov 17
Hmm, perhaps we don't need to revert, I investigated a bit and I have some more clues about what's going on. This is a broken interaction with the "xim" input method. This bug should repro for you locally if you set the environment variable GTK_IM_MODULE=xim (non-English IME). I noticed that when a textbox is focused and any key is pressed, Chrome spams stderr with the following message. This happens even if I locally revert http://crrev.com/595454, but it doesn't have any visible symptoms if key events have the old incorrect timestamp. It doesn't happen if a textbox is not focused. (chrome:141726): Gdk-WARNING **: gdk_window_set_user_time called on non-toplevel https://unix.stackexchange.com/questions/180067/chrome-gdk-gdk-window-set-user-time-called-on-non-toplevel says that doing "unset GTK_IM_MODULE" fixes the logspam, and indeed I also found that works around the bug.
,
Nov 20
Thanks for the investigation. This sounds like a longstanding GDK/Chrome issue which has rather surprising side effects. I would guess that this current bug is simply one visible manifestation. Do we have any idea how many users this effects? That might help us assign a priority & severity. This wikipedia article implies that it's rarely used: https://en.wikipedia.org/wiki/X_Input_Method
,
Nov 28
I set it up originally to type Japanese, but I don't know if it's popular or I picked something weird. yoichio@, would you know if anybody in Tokyo office uses XIM anymore to input Japanese on Linux, or does everybody use something else?
,
Dec 13
I have the same issue, but my GTK_IM_MODULE is set to "ibus" and not "xim". When I unset GTK_IM_MODULE, the keyboard shortcuts work as expected.
,
Dec 13
Some more details: - The bug does not happen at all when my keyboard layout is "English (US)", but does happen when my keyboard layout is "French (Canada)". Unsetting GTK_IM_MODULE, which is normally set to "ibus", does solve the problem. I also do not see the Gdk_Warning referred to above in my terminal. - When the bug does happen, if I keep spamming the keyboard shortcut that is not working, it will eventually work once at some point. The issue thus seems at least partially non-deterministic and may be related to timing or random events. After looking at the code of the commit that potentially introduced the issue, it seems possible that clamping the delta to the range [-30, 30] (in ClamDeltaFromExternalSource) instead of [0, 60] (in ValidateEventTimeClock https://cs.chromium.org/chromium/src/ui/events/event_utils.cc?type=cs&q=ValidateEventTimeClock&sq=package:chromium&g=0&l=70) may cause the issue. The differences are: - [-30, 0]: the old code will fix these values, the new code will not. - [30, 60]: the old code will not fix these values, the new code will. Perhaps my timestamps are usually, but not always, in these two ranges. I suspect they are more likely to be slightly negative than off by 30+ seconds.
,
Dec 13
,
Dec 13
(removed yoichio@ from CC accidentally, re-adding)
,
Dec 13
,
Dec 17
FWIW I'm seeing the same issue in 71 and just checked and I had: GTK_IM_MODULE=xim QT4_IM_MODULE=xim QT_IM_MODULE=ibus XMODIFIERS=@im=ibus After launching chrome with all of them set to ibus it seems to work fine. However looking at im-config details, the reason why it is setting GTK_IM_MODULE=xim is since I didn't have the the im-ibus.so plugin installed. In Debian these are provided by ibus-gtk and ibus-gtk3, so after installing the missing packages I'm now getting: GTK_IM_MODULE=ibus QT4_IM_MODULE=ibus QT_IM_MODULE=ibus XMODIFIERS=@im=ibus And everything is fine.
,
Dec 17
For anyone experiencing this issue, please try getting a trace of the events from the X server with xtrace: $ xtrace google-chrome-stable > log.txt And then upload log.txt
,
Dec 17
> After looking at the code of the commit that potentially introduced the issue, it seems possible that clamping the delta to the range [-30, 30] This is not the issue. I specifically tried making that change in my earlier investigation and it doesn't matter. The problem is caused simply by setting correct timestamps instead of wildly incorrect timestamps, so inspecting the patch itself isn't very revealing. The old incorrect timestamps must've accidentally worked around another bug. The root cause relates to "(chrome:141726): Gdk-WARNING **: gdk_window_set_user_time called on non-toplevel" (I'm guessing it needs to be called on the toplevel instead?). > For anyone experiencing this issue, please try getting a trace of the events from the X server with xtrace: I'm OOO right now, but I'll try when I get back to my Linux machine.
,
Dec 17
xtrace logs are attached to this comment. - log-new-session.txt contains the logs of xtrace when xtrace starts a whole new Chrome session. When Chrome is started in this manner, the bug does not happen. - log-existing-session.txt contains the logs of xtrace when xtrace simply opens a new window in an existing Chrome session. The bug happens in this case. In both cases, I simply clicked on the omnibar, then pressed CTRL+T three times, followed by CTRL+W three times, then closed the opened window by clicking on the X button.
,
Dec 18
Thanks for the traces! Hm.. it's odd that the bug disappears when running under xtrace. The only thing I can think of is that xtrace adds some extra latency. (Also when opening a browser window in an existing session, the new window gets created by the original browser process, so the communication with the X server doesn't go through xtrace). Does the bug reproduce reliably? What about when running a stress test in the background?
,
Dec 18
The bug can be reproduced reliably on my computer. I tried running a CPU stress test but it didn't impact the behavior of Chrome. https://superuser.com/questions/1382314/google-chrome-blocks-ctrlletter-and-altarrow-shortcuts-when-in-text-fi These users seem to have the same issue and appear to have solved it by re-adding their problematic keyboard layout.
,
Dec 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/faa579def265753007f87c3ea85a907f0ed469b0 commit faa579def265753007f87c3ea85a907f0ed469b0 Author: Thomas Anderson <thomasanderson@chromium.org> Date: Tue Dec 18 19:44:53 2018 Revert "[Reland 1] Use accurate X11 event timestamp computation.""" This reverts commit 2c72cd9089bc3278bf3d2852c971325a9c07f097. Reason for revert: bugs 915277 and 899881 Original change's description: > [Reland 1] Use accurate X11 event timestamp computation."" > > The original CL didn't allow negative time skew from the time server. While this > is likely correct, this caused unit tests to fail as they relied on negative time > skew. This CL allows negative time skew, and also removes another source of > non-determinism from the tests. > > Original change's description: > > Use accurate X11 event timestamp computation. > > > > X events have a timestamp which is only well defined relative to the X11 Server > > time. The previous computation for timestamp for X11 events was making the > > assumption that Server time and Chrome time were the same. This assumption is > > not always true -- this is likely the root cause of "bad" timestamps observed in > > https://bugs.chromium.org/p/chromium/issues/detail?id=650338#c1 > > > > This CL changes event timestamp computation to make a roundtrip to the X11 > > Server to get an accurate base::TimeTicks. This logic was lifted out of the > > responsiveness calculator, which was already doing this computation. The latter > > will subsequently be changed to use the computation in this CL. > > > > Change-Id: I963019cd8bfb8ce14e06b3743a159c9c85f2cb82 > > Bug: 859155 > > Reviewed-on: https://chromium-review.googlesource.com/1249383 > > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > > Reviewed-by: Avi Drissman <avi@chromium.org> > > Commit-Queue: Erik Chen <erikchen@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#594844} > > Change-Id: I6595f6aa163a18c9724c50546ac8c84bc5c84a03 > Bug: 859155 > Reviewed-on: https://chromium-review.googlesource.com/1252685 > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > Reviewed-by: Avi Drissman <avi@chromium.org> > Commit-Queue: Erik Chen <erikchen@chromium.org> > Cr-Commit-Position: refs/heads/master@{#595454} TBR=avi@chromium.org,erikchen@chromium.org,thomasanderson@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 859155 , 899881 , 915277 Change-Id: Ib85a5f5d7bf9469076a334d77ef92771199d6a2d Reviewed-on: https://chromium-review.googlesource.com/c/1382868 Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> Commit-Queue: Thomas Anderson <thomasanderson@chromium.org> Cr-Commit-Position: refs/heads/master@{#617593} [modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/event_unittest.cc [modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/platform/x11/x11_event_source.cc [modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/platform/x11/x11_event_source.h [modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/x/events_x.cc [modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/x/events_x_unittest.cc [modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/x/events_x_utils.cc [modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/x/events_x_utils.h
,
Dec 18
,
Dec 18
thomasanderson@ how safe is this change to land into M72 ? This issue seems to be there since M71, how critical is the fix to go out in M72 can it wait until M73?
,
Dec 18
It should be pretty safe since it's a clean revert. I'd like to get this into M72 if possible.
,
Dec 18
thomasanderson@ can you please update the canary results here once available tomorrow , so I can review/approve merge request
,
Dec 19
BTW, this is really annoying in M71, is there any chance to merge it there? That would be awesome. Thank you very much.
,
Dec 19
,
Dec 19
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 19
Verified on 73.0.3645.0. Set GTK_IM_MODULE=xim and Ctrl-T works from the omnibox
,
Dec 20
branch:3626
,
Dec 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6ad9f6333b2a7d81bbbf926529e560269e05f439 commit 6ad9f6333b2a7d81bbbf926529e560269e05f439 Author: Tom Anderson <thomasanderson@chromium.org> Date: Thu Dec 20 19:15:13 2018 [Merge to M72] Revert "[Reland 1] Use accurate X11 event timestamp computation.""" > This reverts commit 2c72cd9089bc3278bf3d2852c971325a9c07f097. > > Reason for revert: bugs 915277 and 899881 > > Original change's description: > > [Reland 1] Use accurate X11 event timestamp computation."" > > > > The original CL didn't allow negative time skew from the time server. While this > > is likely correct, this caused unit tests to fail as they relied on negative time > > skew. This CL allows negative time skew, and also removes another source of > > non-determinism from the tests. > > > > Original change's description: > > > Use accurate X11 event timestamp computation. > > > > > > X events have a timestamp which is only well defined relative to the X11 Server > > > time. The previous computation for timestamp for X11 events was making the > > > assumption that Server time and Chrome time were the same. This assumption is > > > not always true -- this is likely the root cause of "bad" timestamps observed in > > > https://bugs.chromium.org/p/chromium/issues/detail?id=650338#c1 > > > > > > This CL changes event timestamp computation to make a roundtrip to the X11 > > > Server to get an accurate base::TimeTicks. This logic was lifted out of the > > > responsiveness calculator, which was already doing this computation. The latter > > > will subsequently be changed to use the computation in this CL. > > > > > > Change-Id: I963019cd8bfb8ce14e06b3743a159c9c85f2cb82 > > > Bug: 859155 > > > Reviewed-on: https://chromium-review.googlesource.com/1249383 > > > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > > > Reviewed-by: Avi Drissman <avi@chromium.org> > > > Commit-Queue: Erik Chen <erikchen@chromium.org> > > > Cr-Commit-Position: refs/heads/master@{#594844} > > > > Change-Id: I6595f6aa163a18c9724c50546ac8c84bc5c84a03 > > Bug: 859155 > > Reviewed-on: https://chromium-review.googlesource.com/1252685 > > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > > Reviewed-by: Avi Drissman <avi@chromium.org> > > Commit-Queue: Erik Chen <erikchen@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#595454} > > TBR=avi@chromium.org,erikchen@chromium.org,thomasanderson@chromium.org > > # Not skipping CQ checks because original CL landed > 1 day ago. > > Bug: 859155 , 899881 , 915277 > Change-Id: Ib85a5f5d7bf9469076a334d77ef92771199d6a2d > Reviewed-on: https://chromium-review.googlesource.com/c/1382868 > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > Commit-Queue: Thomas Anderson <thomasanderson@chromium.org> > Cr-Commit-Position: refs/heads/master@{#617593} TBR=avi@chromium.org,erikchen@chromium.org,thomasanderson@chromium.org NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true Bug: 859155 , 899881 , 915277 Change-Id: I325a7b1587a8dace272d2b2ac8a2dc0ea0082218 Reviewed-on: https://chromium-review.googlesource.com/c/1387545 Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#487} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/event_unittest.cc [modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/platform/x11/x11_event_source.cc [modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/platform/x11/x11_event_source.h [modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/x/events_x.cc [modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/x/events_x_unittest.cc [modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/x/events_x_utils.cc [modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/x/events_x_utils.h
,
Dec 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6ad9f6333b2a7d81bbbf926529e560269e05f439 Commit: 6ad9f6333b2a7d81bbbf926529e560269e05f439 Author: thomasanderson@chromium.org Commiter: thomasanderson@chromium.org Date: 2018-12-20 19:15:13 +0000 UTC [Merge to M72] Revert "[Reland 1] Use accurate X11 event timestamp computation.""" > This reverts commit 2c72cd9089bc3278bf3d2852c971325a9c07f097. > > Reason for revert: bugs 915277 and 899881 > > Original change's description: > > [Reland 1] Use accurate X11 event timestamp computation."" > > > > The original CL didn't allow negative time skew from the time server. While this > > is likely correct, this caused unit tests to fail as they relied on negative time > > skew. This CL allows negative time skew, and also removes another source of > > non-determinism from the tests. > > > > Original change's description: > > > Use accurate X11 event timestamp computation. > > > > > > X events have a timestamp which is only well defined relative to the X11 Server > > > time. The previous computation for timestamp for X11 events was making the > > > assumption that Server time and Chrome time were the same. This assumption is > > > not always true -- this is likely the root cause of "bad" timestamps observed in > > > https://bugs.chromium.org/p/chromium/issues/detail?id=650338#c1 > > > > > > This CL changes event timestamp computation to make a roundtrip to the X11 > > > Server to get an accurate base::TimeTicks. This logic was lifted out of the > > > responsiveness calculator, which was already doing this computation. The latter > > > will subsequently be changed to use the computation in this CL. > > > > > > Change-Id: I963019cd8bfb8ce14e06b3743a159c9c85f2cb82 > > > Bug: 859155 > > > Reviewed-on: https://chromium-review.googlesource.com/1249383 > > > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > > > Reviewed-by: Avi Drissman <avi@chromium.org> > > > Commit-Queue: Erik Chen <erikchen@chromium.org> > > > Cr-Commit-Position: refs/heads/master@{#594844} > > > > Change-Id: I6595f6aa163a18c9724c50546ac8c84bc5c84a03 > > Bug: 859155 > > Reviewed-on: https://chromium-review.googlesource.com/1252685 > > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > > Reviewed-by: Avi Drissman <avi@chromium.org> > > Commit-Queue: Erik Chen <erikchen@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#595454} > > TBR=avi@chromium.org,erikchen@chromium.org,thomasanderson@chromium.org > > # Not skipping CQ checks because original CL landed > 1 day ago. > > Bug: 859155 , 899881 , 915277 > Change-Id: Ib85a5f5d7bf9469076a334d77ef92771199d6a2d > Reviewed-on: https://chromium-review.googlesource.com/c/1382868 > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > Commit-Queue: Thomas Anderson <thomasanderson@chromium.org> > Cr-Commit-Position: refs/heads/master@{#617593} TBR=avi@chromium.org,erikchen@chromium.org,thomasanderson@chromium.org NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true Bug: 859155 , 899881 , 915277 Change-Id: I325a7b1587a8dace272d2b2ac8a2dc0ea0082218 Reviewed-on: https://chromium-review.googlesource.com/c/1387545 Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#487} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Dec 28
,
Dec 28
Issue 913551 has been merged into this issue.
,
Dec 28
Issue 918047 has been merged into this issue.
,
Jan 1
Issue 918238 has been merged into this issue.
,
Jan 16
(6 days ago)
|
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by erikc...@chromium.org
, Oct 29