Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2393703002:
Fix partial screenshot/window screenshot when holding Alt+Tab. (Closed)
|
Created:
4 years, 2 months ago by Evan Stade Modified:
4 years, 2 months ago CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix partial screenshot/window screenshot when holding Alt+Tab.
Normal screenshot (i.e. whole desktop screenshot) already works
normally, although AFAIK it's impossible to trigger while showing
Alt+ since that modifies the screenshot type.
Partial and window screenshots take over mouse events without breaking
existing mouse captures, so Alt+Tab doesn't know it should close. Fix
this by explicitly breaking existing mouse captures when a screenshot
session starts.
BUG=651939
Committed: https://crrev.com/3be101d020da2ce8eb7c0ab7d92708b8d86acd95
Cr-Commit-Position: refs/heads/master@{#425328}
Patch Set 1 #Patch Set 2 : unit test #
Total comments: 4
Patch Set 3 : capture #
Total comments: 4
Patch Set 4 : break capture after screenshot #Patch Set 5 : update test #Messages
Total messages: 30 (12 generated)
estade@chromium.org changed reviewers: + sky@chromium.org
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2393703002/diff/20001/ash/utility/screenshot_... File ash/utility/screenshot_controller.cc (right): https://codereview.chromium.org/2393703002/diff/20001/ash/utility/screenshot_... ash/utility/screenshot_controller.cc:265: aura::client::GetCaptureClient(root)->SetCapture(nullptr); I'm mildly worried about side effects this may have and calling it part way through Start. Also, could you use CancelMode instead? See DispatchCancelMode() and make alt-tab respect that?
https://codereview.chromium.org/2393703002/diff/20001/ash/utility/screenshot_... File ash/utility/screenshot_controller.cc (right): https://codereview.chromium.org/2393703002/diff/20001/ash/utility/screenshot_... ash/utility/screenshot_controller.cc:265: aura::client::GetCaptureClient(root)->SetCapture(nullptr); On 2016/10/05 15:57:35, sky wrote: > I'm mildly worried about side effects this may have and calling it part way > through Start. Also, could you use CancelMode instead? See DispatchCancelMode() > and make alt-tab respect that? I was actually thinking that any side effects would be desired. For example, I just tested and found we have some kooky behavior if you trigger a screenshot session while dragging tabs (tab is frozen in place and stays there after the screenshot session is over). This fixes that by aborting the tab dragging.
https://codereview.chromium.org/2393703002/diff/20001/ash/utility/screenshot_... File ash/utility/screenshot_controller.cc (right): https://codereview.chromium.org/2393703002/diff/20001/ash/utility/screenshot_... ash/utility/screenshot_controller.cc:265: aura::client::GetCaptureClient(root)->SetCapture(nullptr); On 2016/10/05 17:21:37, Evan Stade wrote: > On 2016/10/05 15:57:35, sky wrote: > > I'm mildly worried about side effects this may have and calling it part way > > through Start. Also, could you use CancelMode instead? See > DispatchCancelMode() > > and make alt-tab respect that? > > I was actually thinking that any side effects would be desired. For example, I > just tested and found we have some kooky behavior if you trigger a screenshot > session while dragging tabs (tab is frozen in place and stays there after the > screenshot session is over). This fixes that by aborting the tab dragging. Sorry for not being clear on side effects. I was more worried about side effects in this class. I agree that canceling tab dragging is what you want to happen. I was hoping ET_CANCEL_MODE would trigger canceling drag and drop, but it doesn't look like it does. So, it seems like canceling capture is the right thing. My only request is ti do this before setting the delegate/mode.
estade@chromium.org changed reviewers: + oshima@chromium.org
https://codereview.chromium.org/2393703002/diff/20001/ash/utility/screenshot_... File ash/utility/screenshot_controller.cc (right): https://codereview.chromium.org/2393703002/diff/20001/ash/utility/screenshot_... ash/utility/screenshot_controller.cc:265: aura::client::GetCaptureClient(root)->SetCapture(nullptr); On 2016/10/06 15:45:50, sky wrote: > On 2016/10/05 17:21:37, Evan Stade wrote: > > On 2016/10/05 15:57:35, sky wrote: > > > I'm mildly worried about side effects this may have and calling it part way > > > through Start. Also, could you use CancelMode instead? See > > DispatchCancelMode() > > > and make alt-tab respect that? > > > > I was actually thinking that any side effects would be desired. For example, I > > just tested and found we have some kooky behavior if you trigger a screenshot > > session while dragging tabs (tab is frozen in place and stays there after the > > screenshot session is over). This fixes that by aborting the tab dragging. > > Sorry for not being clear on side effects. I was more worried about side effects > in this class. I agree that canceling tab dragging is what you want to happen. > > I was hoping ET_CANCEL_MODE would trigger canceling drag and drop, but it > doesn't look like it does. So, it seems like canceling capture is the right > thing. My only request is ti do this before setting the delegate/mode. done (I assume you meant to move the whole loop?) https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... File ash/utility/screenshot_controller.cc (left): https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... ash/utility/screenshot_controller.cc:66: // Ignore capture window when finding the target for located event. so I noticed this bit of code as well, which seems to also be dealing with capture but in a different way. +oshima who wrote this. Do you remember what this was intended to deal with? It seems unnecessary now if we just explicitly break capture. Do you know what behavior I should test to make sure we're not regressing something?
not lgtm This will close menu when this mode is activated, which means that you can't take a screenshot of a menu. https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... File ash/utility/screenshot_controller.cc (left): https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... ash/utility/screenshot_controller.cc:66: // Ignore capture window when finding the target for located event. On 2016/10/06 16:32:34, Evan Stade wrote: > so I noticed this bit of code as well, which seems to also be dealing with > capture but in a different way. +oshima who wrote this. Do you remember what > this was intended to deal with? It seems unnecessary now if we just explicitly > break capture. Do you know what behavior I should test to make sure we're not > regressing something? This is to deal with menu windows, so please do not remove. https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... File ash/utility/screenshot_controller.cc (right): https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... ash/utility/screenshot_controller.cc:446: event->StopPropagation(); how about passing through the event that is required to close the alt-tab when necessary?
> This will close menu when this mode is activated, which means that you can't > take a screenshot of a menu. you can, just not using one of these special modes. You can still do full-screen screenshots then crop to the part you care about. How often are people screenshotting just a menu? https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... File ash/utility/screenshot_controller.cc (right): https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... ash/utility/screenshot_controller.cc:446: event->StopPropagation(); On 2016/10/06 20:07:53, oshima wrote: > how about passing through the event that is required to close the alt-tab when > necessary? I don't think that would fix the tab dragging case, would it? And what would that look like? Don't stop propagation if it's a release of the alt key? That seems fairly fragile and specific to this particular problem. If you don't stop propagation of any key release, that seems likely to cause bad things to happen.
On 2016/10/06 20:21:19, Evan Stade wrote: > > This will close menu when this mode is activated, which means that you can't > > take a screenshot of a menu. > > you can, just not using one of these special modes. You can still do full-screen > screenshots then crop to the part you care about. How often are people > screenshotting just a menu? > > https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... > File ash/utility/screenshot_controller.cc (right): > > https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... > ash/utility/screenshot_controller.cc:446: event->StopPropagation(); > On 2016/10/06 20:07:53, oshima wrote: > > how about passing through the event that is required to close the alt-tab when > > necessary? > > I don't think that would fix the tab dragging case, would it? And what would > that look like? Don't stop propagation if it's a release of the alt key? That > seems fairly fragile and specific to this particular problem. If you don't stop > propagation of any key release, that seems likely to cause bad things to happen. ping on this CL. What's the best way forward? I'd like to avoid something that only fixes alt-tab and leaves the door open for similar issues in the future. I think the best way is to steal mouse capture, but that does break the menu case as oshima@ has pointed out, so we would need to judge how important it is to be able to take a screenshot of a menu in the select-window and region screenshot modes.
On 2016/10/10 18:00:18, Evan Stade wrote: > On 2016/10/06 20:21:19, Evan Stade wrote: > > > This will close menu when this mode is activated, which means that you can't > > > take a screenshot of a menu. > > > > you can, just not using one of these special modes. You can still do > full-screen > > screenshots then crop to the part you care about. How often are people > > screenshotting just a menu? > > > > > https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... > > File ash/utility/screenshot_controller.cc (right): > > > > > https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... > > ash/utility/screenshot_controller.cc:446: event->StopPropagation(); > > On 2016/10/06 20:07:53, oshima wrote: > > > how about passing through the event that is required to close the alt-tab > when > > > necessary? > > > > I don't think that would fix the tab dragging case, would it? And what would > > that look like? Don't stop propagation if it's a release of the alt key? That > > seems fairly fragile and specific to this particular problem. If you don't > stop > > propagation of any key release, that seems likely to cause bad things to > happen. > > ping on this CL. What's the best way forward? I'd like to avoid something that > only fixes alt-tab and leaves the door open for similar issues in the future. I > think the best way is to steal mouse capture, but that does break the menu case > as oshima@ has pointed out, so we would need to judge how important it is to be > able to take a screenshot of a menu in the select-window and region screenshot > modes. Have you considered closing out the bug as works as intended? Otherwise how could you take a screenshot of alt-tab? The only way to get in this mode is to hold down alt-tab *and* Ctrl+F5, do we really think someone holding both accelerators down is doing it unintentionally?
On 2016/10/10 20:03:56, sky wrote: > On 2016/10/10 18:00:18, Evan Stade wrote: > > On 2016/10/06 20:21:19, Evan Stade wrote: > > > > This will close menu when this mode is activated, which means that you > can't > > > > take a screenshot of a menu. > > > > > > you can, just not using one of these special modes. You can still do > > full-screen > > > screenshots then crop to the part you care about. How often are people > > > screenshotting just a menu? > > > > > > > > > https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... > > > File ash/utility/screenshot_controller.cc (right): > > > > > > > > > https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... > > > ash/utility/screenshot_controller.cc:446: event->StopPropagation(); > > > On 2016/10/06 20:07:53, oshima wrote: > > > > how about passing through the event that is required to close the alt-tab > > when > > > > necessary? > > > > > > I don't think that would fix the tab dragging case, would it? And what would > > > that look like? Don't stop propagation if it's a release of the alt key? > That > > > seems fairly fragile and specific to this particular problem. If you don't > > stop > > > propagation of any key release, that seems likely to cause bad things to > > happen. > > > > ping on this CL. What's the best way forward? I'd like to avoid something that > > only fixes alt-tab and leaves the door open for similar issues in the future. > I > > think the best way is to steal mouse capture, but that does break the menu > case > > as oshima@ has pointed out, so we would need to judge how important it is to > be > > able to take a screenshot of a menu in the select-window and region screenshot > > modes. > > Have you considered closing out the bug as works as intended? Otherwise how > could you take a screenshot of alt-tab? by just pressing f5 (whole desktop screenshot), without ctrl > The only way to get in this mode is to > hold down alt-tab *and* Ctrl+F5, do we really think someone holding both > accelerators down is doing it unintentionally? It's certainly not the end of the world if we don't fix it. All you have to do to dismiss alt-tab is to press alt again. But it also feels broken that you've released alt and the ui is still present.
On 2016/10/06 20:21:19, Evan Stade wrote: > > This will close menu when this mode is activated, which means that you can't > > take a screenshot of a menu. > > you can, just not using one of these special modes. You can still do full-screen > screenshots then crop to the part you care about. How often are people > screenshotting just a menu? This approach also breaks partial screenshot. > https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... > File ash/utility/screenshot_controller.cc (right): > > https://codereview.chromium.org/2393703002/diff/40001/ash/utility/screenshot_... > ash/utility/screenshot_controller.cc:446: event->StopPropagation(); > On 2016/10/06 20:07:53, oshima wrote: > > how about passing through the event that is required to close the alt-tab when > > necessary? > > I don't think that would fix the tab dragging case, would it? It will not, but we can fix it in a different way if that's important. > And what would > that look like? Don't stop propagation if it's a release of the alt key? That > seems fairly fragile and specific to this particular problem. I still think it's better than being unable to take a partial screenshot with menus. > If you don't stop > propagation of any key release, that seems likely to cause bad things to happen.
ok, had an idea and updated the patch accordingly. Break capture *after* the screenshot session. This maintains the ability to take screenshots of menus (only difference is now the menu automatically dismisses after the screenshot session is over). It also allows you to take screenshots of alt tab (I mis-spoke before, you can't trigger full screen screenshot with alt held down).
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/11 00:30:04, Evan Stade (ooo till 10-20) wrote: > ok, had an idea and updated the patch accordingly. Break capture *after* the > screenshot session. This maintains the ability to take screenshots of menus > (only difference is now the menu automatically dismisses after the screenshot > session is over). It also allows you to take screenshots of alt tab (I mis-spoke > before, you can't trigger full screen screenshot with alt held down). ping
LGTM - but wait for Oshima
thanks, lgtm
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix partial screenshot/window screenshot when holding Alt+Tab. Normal screenshot (i.e. whole desktop screenshot) already works normally, although AFAIK it's impossible to trigger while showing Alt+ since that modifies the screenshot type. Partial and window screenshots take over mouse events without breaking existing mouse captures, so Alt+Tab doesn't know it should close. Fix this by explicitly breaking existing mouse captures when a screenshot session starts. BUG=651939 ========== to ========== Fix partial screenshot/window screenshot when holding Alt+Tab. Normal screenshot (i.e. whole desktop screenshot) already works normally, although AFAIK it's impossible to trigger while showing Alt+ since that modifies the screenshot type. Partial and window screenshots take over mouse events without breaking existing mouse captures, so Alt+Tab doesn't know it should close. Fix this by explicitly breaking existing mouse captures when a screenshot session starts. BUG=651939 Committed: https://crrev.com/3be101d020da2ce8eb7c0ab7d92708b8d86acd95 Cr-Commit-Position: refs/heads/master@{#425328} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3be101d020da2ce8eb7c0ab7d92708b8d86acd95 Cr-Commit-Position: refs/heads/master@{#425328} |
