Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2284763002:
Redirect all mouse input to Alt+Tab window when it's visible, (Closed)
|
Created:
4 years, 3 months ago by Evan Stade Modified:
4 years, 3 months ago Reviewers:
sky CC:
chromium-reviews, kalyank, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRedirect all mouse input to Alt+Tab window when it's visible,
effectively disabling the mouse. Capture is reset when the widget is
destroyed.
BUG=641171
Committed: https://crrev.com/c7e292ea58a76fe27c6c247bc640054bcf4cb6aa
Cr-Commit-Position: refs/heads/master@{#415096}
Patch Set 1 #Patch Set 2 : test #
Total comments: 3
Patch Set 3 : handle lost capture #
Messages
Total messages: 20 (10 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.
One meta question, are you sure you want to eat pointer events outside the widget? I would expect them to cancel the alt-tab. https://codereview.chromium.org/2284763002/diff/20001/ash/common/wm/window_cy... File ash/common/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2284763002/diff/20001/ash/common/wm/window_cy... ash/common/wm/window_cycle_list.cc:598: widget->SetCapture(nullptr); This seems like a round about way to eat events. This level of consuming is generally done with an EventHandler. See SystemModalContainerEventFilter for what I'm referring to. This begs the question though, should this widget by system modal? That gives you the filtering you want.
> One meta question, are you sure you want to eat pointer events outside the > widget? I would expect them to cancel the alt-tab. I don't find it very likely that you'd be holding alt and then click around with the mouse, so I don't think the behavior matters all that much. But I chose to do nothing because as I was racking my brain for situations where this could plausibly come up, and I decided that a user could be alt+tabbing on a system with a touch screen, then someone else comes up behind and taps on a window with their finger which can generate a pointer event on some systems. Are they trying to select the window or just indicate which window they're talking about? My guess is the latter. I couldn't think of a situation where you'd be unhappy just letting go of alt before clicking. https://codereview.chromium.org/2284763002/diff/20001/ash/common/wm/window_cy... File ash/common/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2284763002/diff/20001/ash/common/wm/window_cy... ash/common/wm/window_cycle_list.cc:598: widget->SetCapture(nullptr); On 2016/08/26 19:31:48, sky wrote: > This seems like a round about way to eat events. This level of consuming is > generally done with an EventHandler. See SystemModalContainerEventFilter for > what I'm referring to. This begs the question though, should this widget by > system modal? That gives you the filtering you want. It appears that system modality only works if I put this in kShellWindowId_SystemModalContainer. Putting it in that container means the rest of the desktop gets dimmed while Alt+Tab is active, which AFAIK we don't want. It also breaks some other stuff like accelerator handling (so only the first Alt+Tab works, which we could probably fix if we're keen on rewriting how Alt+Tabbing works). If I understand your EventHandler suggestion, we'd have to do WmShell::Get()->PrependPreTargetHandler(handlerthateatseverything) and later WmShell::Get()->RemovePreTargetHandler(...). I don't know what you mean by roundabout exactly but that approach looks to me like more lines of code and more memory management.
FWIW I tried one OS, and while mouse clicking with alt-tab up the alt-tab goes away. https://codereview.chromium.org/2284763002/diff/20001/ash/common/wm/window_cy... File ash/common/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2284763002/diff/20001/ash/common/wm/window_cy... ash/common/wm/window_cycle_list.cc:598: widget->SetCapture(nullptr); On 2016/08/26 22:23:22, Evan Stade (ooo wed-thurs) wrote: > On 2016/08/26 19:31:48, sky wrote: > > This seems like a round about way to eat events. This level of consuming is > > generally done with an EventHandler. See SystemModalContainerEventFilter for > > what I'm referring to. This begs the question though, should this widget by > > system modal? That gives you the filtering you want. > > It appears that system modality only works if I put this in > kShellWindowId_SystemModalContainer. Putting it in that container means the rest > of the desktop gets dimmed while Alt+Tab is active, which AFAIK we don't want. > It also breaks some other stuff like accelerator handling (so only the first > Alt+Tab works, which we could probably fix if we're keen on rewriting how > Alt+Tabbing works). > > If I understand your EventHandler suggestion, we'd have to do > WmShell::Get()->PrependPreTargetHandler(handlerthateatseverything) and later > WmShell::Get()->RemovePreTargetHandler(...). I don't know what you mean by > roundabout exactly but that approach looks to me like more lines of code and > more memory management. Sorry if I wasn't clear. I suggested looking at SystemModalContainerEventFilter as a way that we typically restrict events, not that it would work for you as is, just the general principal. I said a round about way because you don't really want all pointer events to go to widget, rather you want any pointer events outside of widget to be dropped. EventFilter lets you do that. But in writing this up, I do see why you want capture and it makes sense. Seems like you should close if you unexpectedly lose capture though, right?
New patch handles lost capture case. I don't know how to test this manually but I added a unit test which seems to confirm it works. I've pinged Sebastien on the bug regarding the desired behavior.
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...
LGTM
The CQ bit was unchecked by estade@chromium.org
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 #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Redirect all mouse input to Alt+Tab window when it's visible, effectively disabling the mouse. Capture is reset when the widget is destroyed. BUG=641171 ========== to ========== Redirect all mouse input to Alt+Tab window when it's visible, effectively disabling the mouse. Capture is reset when the widget is destroyed. BUG=641171 Committed: https://crrev.com/c7e292ea58a76fe27c6c247bc640054bcf4cb6aa Cr-Commit-Position: refs/heads/master@{#415096} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c7e292ea58a76fe27c6c247bc640054bcf4cb6aa Cr-Commit-Position: refs/heads/master@{#415096}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2294723002/ by perkj@chromium.org. The reason for reverting is: WindowCycleControllerTest.MouseCaptureLost fails on Chrome os and Asan. https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
