New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 863728 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Could not take window screenshot of shortcut app window

Project Member Reported by warx@chromium.org, Jul 15

Issue description

Chrome Version: Version 69.0.3486.0 (Official Build) dev (64-bit)
OS: 10866.1.0 (Official Build) dev-channel eve

ctrl+alt+f4 to take window screenshot of shortcut app window. Shortcut app window could not be correctly selected and it fails on saving as well.


 
Owner: warx@chromium.org
why ctrl+alt+f4 goes into terminal mode?
Cc: jamescook@chromium.org
Labels: -Pri-3 M-72 Pri-2
Owner: mukai@chromium.org
looks like mojo app issue. mukai-san, can you take a look?
Cc: rcui@chromium.org mukai@chromium.org
Components: Internals>Services>Ash
Labels: Proj-Mash-KSV
Owner: msw@chromium.org
msw, could you take a look?  Or rcui?
Is this with Mash, SingleProcessMash, or neither?  Screenshots don't work in Mash yet (crbug.com/557397).
I think neither -- but the keyboard shortcut viewer is still a Mojo process and that could be affected?
#5, Yes, that's my guess.
Cc: -rcui@chromium.org msw@chromium.org
Owner: rcui@chromium.org
To clarify, Ctrl-F4 (full screenshot) and Ctrl-Shift-F4 (region selected screenshot) are working, right?

If so, this would be essentially a dupe for issue 557397. Reassigning to rcui. Ryan, feel free to mark as duplicated.
Owner: jamescook@chromium.org
Some observations (via Chrome 71.0.3567.0 on dev channel):

1. Ctrl-F4 (full screenshot) works, with and without shortcut viewer.
2. Ctrl-Shift-F4 (region selection) works, with and without shortcut viewer.
3. Ctrl-Alt-F4 (window selection) works fine without shortcut viewer.  With the shortcut viewer open, it's very difficult to select the shortcut viewer window, and the code keeps highlighting a 'ghost' window in the top-left corner.  See pic attached.  When I click on the ghost window, I get a "Failed to save screenshot" error.

As this happens without Mash or SingleProcessMash enabled, it's not a dupe of issue 557397.  Re-assigning to jamescook@ for thoughts here.

MVIMG_20181008_124421.jpg
6.8 MB View Download
Owner: rcui@chromium.org
Ryan, would you mind looking a bit closer?
How does the window selection screenshot codepath work?
It's not clear what is actually broken when handling mojo app windows like the ksv.
If you can point out some relevant codepaths, I might be able to investigate further.
https://cs.chromium.org/chromium/src/ui/snapshot/snapshot.cc?rcl=f52d6249596258ba819c6f6a3f081594711ff61f&l=52

|source_rect| is relative to the |window|, but it seems to be used as
screen coords for mojo app. 
sorry, correction.

> but it seems to be used as screen coords for mojo app. 

it seems that the screen coords rect is passed for mojo app.


https://cs.chromium.org/chromium/src/ui/snapshot/snapshot_aura.cc?rcl=4e72f72afe4b4c78a7e210b68992ebc72de0a526&l=31

This API may also have an issue if the |window| is mojo app's root
because it gives error even if I move the window to top,left.


Digging into this a bit more.  Re: oshima@'s comment in #12, I believe the coordinate rect is simply the Size of the aura::Window being captured[1].  I don't know enough yet about how mojo apps handle windows to provide more details.  Will look at dumping the window hierarchy and see if that provides more clues.

[1] https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/chrome_screenshot_grabber.cc?rcl=71ca70dd801046e681ff7fc2a81a2f5c64dfa3bc&l=455
Cc: sky@chromium.org fsam...@chromium.org
Labels: Proj-Mash-SingleProcess
I'm not familiar with that code (nor compositing/layers), but clicking through took me here:
 https://cs.chromium.org/chromium/src/ui/snapshot/snapshot_aura.cc?rcl=d8f40759b05fdf720be7ca17537dd722505bd81d&l=75
Perhaps the window's layer (as seen by Ash/Chrome) is not valid for screenshotting in that way?
Do the other screenshot codepaths use individual window layers or some other composited surface?
#14: other screenshot uses root windows (of Ash) to capture the screenshot. The bounds wierdness and failure might mean that the 'window' in #13 could be a window in Browser and not in Ash?
The window in question is for the KSV app, which is running as a mojo app in its own process.
We should solve this however we intend to support per-window screenshots in Mash generally.
Cc: rcui@chromium.org
Owner: mukai@chromium.org
Okay, I guess the primary reason wasn't Mash-not-supporting-screenshot. It looks like due to a change on window structure caused by the keyboard-shortcut viewer.

Previously it was just a window (with views::Widget) directly under DefaultContainer, and the current screenshot code probably assumes the window is toplevel.
But now KSV consists of two windows in Ash, one is KeyboardShortcutWidget (i.e. window frame, which is toplevel) and the content window (drawn by a mojo app). If the mouse moves around the content area of KSV, the event target becomes the content window, and starts misbehaving.

When the mouse cursor moves around the title bar of the KSV, the selector properly highlight the window and the screenshot can be taken successfully. Maybe we need to fix on Ash side. I can take a look.
Project Member

Comment 18 by bugdroid1@chromium.org, Oct 12

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

commit 965bb35d095c9cf3e31c12e43bdbe82cdff4da00
Author: Jun Mukai <mukai@chromium.org>
Date: Fri Oct 12 22:57:47 2018

Skip non-toplevel child windows for screenshot window selector

The Keyboard Shortcut Viewer hits this condition -- now it has
a toplevel widget with child content window, and when the mouse
pointer comes to the content window's area, it is unexpectedly
selected, but the toplevel one should be selected.

BUG= 863728 
TEST=manually, and the new test case covers

Change-Id: I9962114c6b14df412148cf9e6545f32fc5eaa845
Reviewed-on: https://chromium-review.googlesource.com/c/1272089
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Jun Mukai <mukai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599395}
[modify] https://crrev.com/965bb35d095c9cf3e31c12e43bdbe82cdff4da00/ash/utility/screenshot_controller.cc
[modify] https://crrev.com/965bb35d095c9cf3e31c12e43bdbe82cdff4da00/ash/utility/screenshot_controller_unittest.cc
[modify] https://crrev.com/965bb35d095c9cf3e31c12e43bdbe82cdff4da00/services/ws/window_service.cc
[modify] https://crrev.com/965bb35d095c9cf3e31c12e43bdbe82cdff4da00/services/ws/window_service.h

Status: Fixed (was: Assigned)

Sign in to add a comment