Adding the conops Chrome OS hotlist.

 Issue 848033  has been merged into this issue.

 Issue 837898  has been merged into this issue.

CC'ing folks with recent changes in ChromeOS Screenshot code (best I can tell)

Not sure if ScreenCapture is the right component (usually see associated with getUserMedia()). Leaving for now. 

 Issue 849643  has been merged into this issue.

Pretty wide spread. Marking p1 until I hear otherwise.

chcunningham, is it possible the screenshots are happening but the notification isn't being shown?

+derat, were you seeing cases where keyboard shortcuts were just being ignored? I see this occasionally (like Ctrl-T doesn't work) but I can't repro.

+estade, does this sound familiar?

I don't see anything revealing in the system logs. In one of the partial screenshot failure reports I see this:

[1471:1471:0519/144758.618722:ERROR:gles2_cmd_decoder.cc(18052)] [.DisplayCompositor-0x24dd8dc3b500]GL ERROR :GL_INVALID_OPERATION : glCreateAndConsumeTextureCHROMIUM: invalid mailbox name
[1471:1471:0519/144758.633216:ERROR:gles2_cmd_decoder.cc(10173)] [.DisplayCompositor-0x24dd8dc3b500]RENDER WARNING: texture bound to texture unit 0 is not renderable. It maybe non-power-of-2 and have incompatible texture filtering.

There's also some stuff like:

[4088:4200:0504/134342.167809:WARNING:name_value_pairs_parser.cc(55)] Key keyboard_layout already has value xkb:us<IPv6: 5>ng, ignoring new value: xkb:us<IPv6: 5>ng

but I suspect the <IPv6: 5> is automated PII-redaction gone wrong.

This seems like an odd "error" but I only see it once:

[4088:4088:0504/134403.514853:ERROR:global_shortcut_listener_ozone.cc(26)] GlobalShortcutListenerOzone object created

> chcunningham, is it possible the screenshots are happening but the notification isn't being shown?

I haven't reproed the issue personally. I'm just humbly triaging Internals>Media>* - I'm totally unfamiliar with Screenshot code.

doesn't sound familiar. Only recent screenshot related problem I'm aware of is  issue 846654  which has different symptoms and occurred after M65/66.

#9:  Issue 780889 , duped into  issue 780146  (which has been marked fixed) was my report about accelerators not working.

Elizabeth let us know if you see similar reports in M67,68?

Assigning to trumbull for feedback

I'm only seeing two reports on M67 Beta about Screenshot not working:
(1) 67.0.3396.69 beta - https://listnr.corp.google.com/report/85476024938
(2) 67.0.3396.57 beta - https://listnr.corp.google.com/report/85472835265

Both report that they're seeing this notification "An error occurred. Failed to save screenshot"

Not sure if that's after the merges.

I don't see any such reports from users on M68.

I've submitted feedback with Alt+Shift+i, I am having this same issue for about a week now.

msolaridesign@gmail.com's report is:
https://listnr.corp.google.com/report/85553971574

on 67.0.3396.99

Another report on 67.0.3396.99: https://listnr.corp.google.com/report/85537476856

I really don't see other user reports about this, so this *may* be fixed on M68

Got customer reporting this on 67.0.3396.99, reproed in 69.0.3497.21 and M68 too. Ctrl+F5 works on external keyboard, Print Screen only hides cursor (no image is created in Downloads).

Which key is 'Print Screen'?  I've never seen such a key on a Chromebook.

The Ctrl-Other Windows button works for me on 68.0.3440.87 beta.

#21: I think this is referring to external keyboards. Several of the original feedback reports were sent from Chromeboxes.

Ah, got it.  If it works from the built-in keyboard, this doesn't seem rooted in the screen capture implementation itself, then.

When I make AcceleratorController::Process in ash/accelerators/accelerator_controller.cc log the events that it receives and then connect an external HP USB keyboard to a caroline device and hit the "Print Scrn / SysRq" key, I see keycode 44, which is mapped to VKEY_SNAPSHOT in ui/events/keycodes/keyboard_codes_posix.h.

In ash/public/cpp/accelerators.cc, we map VKEY_PRINT to TAKE_SCREENSHOT, though:

    {true, ui::VKEY_PRINT, ui::EF_NONE, TAKE_SCREENSHOT},

We've used VKEY_PRINT in the accelerator since before September 2012 (r158524).

Similarly, the mappings in keyboard_codes_posix.h date way back to 2009:

  VKEY_PRINT = 0x2A,
  ...
  VKEY_SNAPSHOT = 0x2C,

https://docs.microsoft.com/en-us/windows/desktop/inputdev/virtual-key-codes documents VK_PRINT 0x2A as "PRINT key" and VK_SNAPSHOT 0x2C as "PRINT SCREEN key". So it seems reasonable to me that this key produces VKEY_SNAPSHOT, and it's also expected that the accelerator doesn't work since it's watching for VKEY_PRINT. If this worked at some point in the past, presumably it was because the key was producing VKEY_PRINT then (or events were being rewritten).

The easy fix is to update the accelerator in ash/public/cpp/accelerators.cc to use VKEY_SNAPSHOT instead of VKEY_PRINT. I can confirm that the "Print Scrn / SysRq" key takes screenshots after I do that. I don't know if doing so will have negative side effects (is VKEY_PRINT still produced in some cases, for instance?).

It'd be good to know what changed here.

Here's my understanding of how things work.

A normal USB keyboard will send keyboard HID code 0x07:0046 'Keyboard PrintScreen'.

The Linux kernel maps this to evdev code 0x63 KEY_SYSRQ (not to be confused with evdev code 0xD2 KEY_PRINT which comes from consumer control HID code 0C:0208 'AC Print', and let us never speak of it again).

ChromeOS maps this (via the source of truth keycode_converter_data.inc) to the platform-independent W3C KeyboardEvent.code 'PrintScreen' aka DomCode::PRINT_SCREEN.

That was all at the physical key level. Now the user's selected layout applies. But in the normal case, there is no specific layout entry for that key (since it's not a language-varying writing system key). Assuming that, |kNonPrintableCodeMap| maps KeyboardEvent.code 'PrintScreen' to KeyboardEvent.key 'PrintScreen' and finally |kDomKeyToKeyboardCodeMap| maps the latter to the legacy Windows keycode VKEY_SNAPSHOT.

I don't see any of the files having been touched at all recently.

#25: Thanks for all the details! So if VKEY_SNAPSHOT is the expected ui::KeyboardCode here, should I just update the accelerator to watch for that instead of VKEY_PRINT?

(While there's at least one feedback report stating that this key used to work for taking screenshots, for all I know, it could've never worked. That would explain the lack of changes that could've triggered a regression.)

It always worked for me (before the 2nd to last update) for taking
screenshots on my Asus Chromebox since 2014.


Sincerely,
Craig Murway, FC, CCS, CPT
www.craigmurway.com

OK, got it.

I was mistaken XKB layout not handling the key. I prefer to think of XKB as a black box (with venomous spiders inside) and the relevant CrOS packages (libxkbcommon and especially xkeyboard-config) have also not been changed in a while.

So, the physical KeyboardEvent.code 'PrintScreen' is XKB 107 <PRSC> and maps via /usr/share/X11/xkb/symbols/pc to XKeysym 'Print'. That gets converted to a KeyboardEvent.key (aka DomKey) by NonPrintableXKeySymToDomKey(), and THAT changed recently —  https://crbug.com/683097  http://crrev.com/537512 — from returning 'Print' (which mapped to Windows VKEY_PRINT) to returning 'PrintScreen' (Windows VKEY_SNAPSHOT).

I think it would be safe and effective to have both VKEY_PRINT and VKEY_SNAPSHOT trigger TAKE_SCREENSHOT.

There are a few other occurrences of VKEY_PRINT that should be looked at, including KeyboardCodeFromXKeysym(). I'll back away now, but please cc me on reviews.

Thanks! I've sent https://crrev.com/c/1166037 for review.

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

commit 8981b7ec500b3f476d9bdb825299a2f26bbab3d2
Author: Daniel Erat <derat@chromium.org>
Date: Wed Aug 08 22:49:47 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-Commit-Position: refs/heads/master@{#581711}
[modify] https://crrev.com/8981b7ec500b3f476d9bdb825299a2f26bbab3d2/ash/accelerators/accelerator_controller_unittest.cc
[modify] https://crrev.com/8981b7ec500b3f476d9bdb825299a2f26bbab3d2/ash/accelerators/accelerator_filter_unittest.cc
[modify] https://crrev.com/8981b7ec500b3f476d9bdb825299a2f26bbab3d2/ash/accelerators/accelerator_interactive_uitest_chromeos.cc
[modify] https://crrev.com/8981b7ec500b3f476d9bdb825299a2f26bbab3d2/ash/accelerators/accelerator_table_unittest.cc
[modify] https://crrev.com/8981b7ec500b3f476d9bdb825299a2f26bbab3d2/ash/public/cpp/accelerators.cc
[modify] https://crrev.com/8981b7ec500b3f476d9bdb825299a2f26bbab3d2/chrome/browser/ui/ash/ksv/keyboard_shortcut_viewer_metadata_unittest.cc

When will the fix rollout? (Just received an update but highly doubt that
was attached). Thank you.


Sincerely,
Craig Murway, FC, CCS, CPT
www.craigmurway.com

Requesting a merge to M69, which is scheduled to go to the stable channel in mid-September.

Ohh thank you :) Tonight's update seemed to resolve my Gmail "loading"
issue, really happy about that!


Sincerely,
Craig Murway, FC, CCS, CPT
www.craigmurway.com

Spoke too soon about the Gmail loading issues...oh whelp. Least the screenshot issue w/ be resolved ;)

This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

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

Merge approved, M69.

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