New issue
Advanced search Search tips

Issue 872094 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Reconcile VKEY_ mapping for Print Screen on X11 and XKB

Project Member Reported by derat@chromium.org, Aug 8

Issue description

 Issue 683097  updated keyboard_code_conversion_xkb.cc so that Print Screen keys on external keyboards attached to Chrome OS devices produce VKEY_SNAPSHOT instead of VKEY_PRINT.

When I run an OS_CHROMEOS build of Chrome on a Linux workstation (as is commonly done by Chrome OS UI developers), my keyboard's Print Screen key appears to still be mapped to VKEY_PRINT.

The upshot of this seems to be that we need to register TAKE_SCREENSHOT accelerators in ash/public/cpp/accelerators.cc for both VKEY_PRINT and VKEY_SNAPSHOT.

Kevin suggested on https://crrev.com/c/1166037 that we may need to update keyboard_code_conversion_x.cc to map XK_Print to VKEY_SNAPSHOT rather than to VKEY_PRINT.

Does that sound correct?
 
Cc: w...@chromium.org
AFAICT XK_Print is the keysym for the PrintScreen key, for which the corresponding VKEY value is VK_SNAPSHOT, so yes we should fix the converter to map XK_print to VK_SNAPSHOT, not VK_PRINT (which is the code for a potential "Print" key, rather than "PrintScreen").
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 8

Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 13

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

commit 6f7c49b40aa36dcc18b869adcd459b258fb07575
Author: Daniel Erat <derat@chromium.org>
Date: Mon Aug 13 17:50:43 2018

linux/chromeos: Map XK_Print to VKEY_SNAPSHOT.

Map the XK_Print X11 keysym to VKEY_SNAPSHOT rather than
VKEY_PRINT; VKEY_SNAPSHOT is the correct keyboard code for
the "Print Screen" key.

Also fix the mapping in the Chrome OS IME keycode table and
remove the now-unused VKEY_PRINT screenshot accelerator.

Bug:  872094 
Change-Id: If5535534a81da00500da427fb5ef5fdc0a8a58e2
Reviewed-on: https://chromium-review.googlesource.com/1170166
Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582633}
[modify] https://crrev.com/6f7c49b40aa36dcc18b869adcd459b258fb07575/ash/accelerators/accelerator_table_unittest.cc
[modify] https://crrev.com/6f7c49b40aa36dcc18b869adcd459b258fb07575/ash/public/cpp/accelerators.cc
[modify] https://crrev.com/6f7c49b40aa36dcc18b869adcd459b258fb07575/chrome/browser/ui/ash/ksv/keyboard_shortcut_viewer_metadata_unittest.cc
[modify] https://crrev.com/6f7c49b40aa36dcc18b869adcd459b258fb07575/ui/base/ime/chromeos/ime_keymap.cc
[modify] https://crrev.com/6f7c49b40aa36dcc18b869adcd459b258fb07575/ui/events/keycodes/keyboard_code_conversion_x.cc
[modify] https://crrev.com/6f7c49b40aa36dcc18b869adcd459b258fb07575/ui/events/keycodes/keyboard_codes_posix.h

Status: Fixed (was: Started)
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 22

Labels: merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/28a02e31831bdf632b495852278ac2880fa510b9

commit 28a02e31831bdf632b495852278ac2880fa510b9
Author: Daniel Erat <derat@chromium.org>
Date: Wed Aug 22 21:08:45 2018

chromeos: Fix "Print Screen" screenshot accelerator.

Make ash map VKEY_SNAPSHOT to the TAKE_SCREENSHOT action.
7ab3497a updated Chrome to map the "Print Screen" key to
VKEY_SNAPSHOT rather than VKEY_PRINT.

Bug:  683097 ,  846919 ,  872094 
Change-Id: I3e8de9e8b2d110d8e06649fc0553c6d9b7cab35a
Reviewed-on: https://chromium-review.googlesource.com/1166037
Commit-Queue: Dan Erat <derat@chromium.org>
Reviewed-by: Kevin Schoedel <kpschoedel@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#581711}(cherry picked from commit 8981b7ec500b3f476d9bdb825299a2f26bbab3d2)
Reviewed-on: https://chromium-review.googlesource.com/1185502
Reviewed-by: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#779}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/28a02e31831bdf632b495852278ac2880fa510b9/ash/accelerators/accelerator_controller_unittest.cc
[modify] https://crrev.com/28a02e31831bdf632b495852278ac2880fa510b9/ash/accelerators/accelerator_filter_unittest.cc
[modify] https://crrev.com/28a02e31831bdf632b495852278ac2880fa510b9/ash/accelerators/accelerator_interactive_uitest_chromeos.cc
[modify] https://crrev.com/28a02e31831bdf632b495852278ac2880fa510b9/ash/accelerators/accelerator_table_unittest.cc
[modify] https://crrev.com/28a02e31831bdf632b495852278ac2880fa510b9/ash/public/cpp/accelerators.cc
[modify] https://crrev.com/28a02e31831bdf632b495852278ac2880fa510b9/chrome/browser/ui/ash/ksv/keyboard_shortcut_viewer_metadata_unittest.cc

Sign in to add a comment