Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2127263002:
Keep the SystemModalContainerEventHandler added (Closed)
DescriptionKeep the SystemModalContainerEventHandler added, and check the modality in the event handler, instead of adding/removing the event handler.
Ensures that pointer/touch events on Arc windows are not registered when a system modal is open.
BUG=624168
TEST=PointerTest.IgnorePointerEventDuringModal,TouchTest.IgnoreTouchEventDuringModal
Committed: https://crrev.com/0dd03dd13d630af3457d4f43beead3b83e95956a
Cr-Commit-Position: refs/heads/master@{#405632}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Ignore touch/pointer events on Arc windows if modal open #
Total comments: 6
Patch Set 3 : Ignore touch/pointer events on Arc windows if modal open #
Total comments: 17
Patch Set 4 : Ignore touch/pointer events on Arc windows if modal open #Patch Set 5 : Ignore touch/pointer events on Arc windows if modal open #
Total comments: 1
Patch Set 6 : Keep the SystemModalContainerEventHandler added #
Messages
Total messages: 37 (17 generated)
Description was changed from ========== Ignore touch/pointer events on Arc windows if modal open BUG=624168 ========== to ========== Ignore touch/pointer events on Arc windows if modal open BUG=624168 ==========
hariank@google.com changed reviewers: + oshima@chromium.org
https://codereview.chromium.org/2127263002/diff/1/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/2127263002/diff/1/ash/shell.h#newcode530 ash/shell.h:530: // ash::SystemModalContainerEventFilterDelegate overrides: nit: // SystemModalContainerEventFilterDelegate: (it's new style) https://codereview.chromium.org/2127263002/diff/1/components/exo/pointer_unit... File components/exo/pointer_unittest.cc (right): https://codereview.chromium.org/2127263002/diff/1/components/exo/pointer_unit... components/exo/pointer_unittest.cc:318: // Open modal. Make the windowmodal. (The window is already opened at this point) https://codereview.chromium.org/2127263002/diff/1/components/exo/pointer_unit... components/exo/pointer_unittest.cc:343: EXPECT_CALL(delegate, OnPointerFrame()).Times(0); can you also test touch & gesture? https://codereview.chromium.org/2127263002/diff/1/components/exo/pointer_unit... components/exo/pointer_unittest.cc:345: generator.MoveMouseTo(location); can you group the event and expectation together , instead of mixing all? something like set expectation for mouse click mouse click set expectation for mouse wheel mouse wheel set expectation for touch touch event https://codereview.chromium.org/2127263002/diff/1/components/exo/pointer_unit... components/exo/pointer_unittest.cc:348: can you also close the modal dialog and test if they receive events correctly?
https://codereview.chromium.org/2127263002/diff/1/components/exo/touch_unitte... File components/exo/touch_unittest.cc (right): https://codereview.chromium.org/2127263002/diff/1/components/exo/touch_unitte... components/exo/touch_unittest.cc:217: // Open modal. ditto. https://codereview.chromium.org/2127263002/diff/1/components/exo/touch_unitte... components/exo/touch_unittest.cc:235: EXPECT_CALL(delegate, OnTouchDestroying(touch.get())); ditto. close the modal and verify that it'll receive events.
https://codereview.chromium.org/2127263002/diff/20001/components/exo/pointer_... File components/exo/pointer_unittest.cc (right): https://codereview.chromium.org/2127263002/diff/20001/components/exo/pointer_... components/exo/pointer_unittest.cc:303: surface->window()->GetBoundsInScreen().origin() + gfx::Vector2d(1, 1); since this is used only once, you can just inline moveMouseTo(location + gfx::VEctor2d(1, 1)) and so forth. https://codereview.chromium.org/2127263002/diff/20001/components/exo/pointer_... components/exo/pointer_unittest.cc:304: gfx::Point location3 = surface->window()->GetBoundsInScreen().bottom_right(); ditto. https://codereview.chromium.org/2127263002/diff/20001/components/exo/pointer_... components/exo/pointer_unittest.cc:329: testing::Sequence s1, s2; Can you do the following? I think it's easier to understand and read. { testing::Sequence seq; EXPECT_ EXPECT_ } generator.MoveMouseTo() { testing::Sequence seq; EXPECT... EXPECT... } generator.ScrollSequence(..) same for the touch_unittest.cc
https://codereview.chromium.org/2127263002/diff/20001/components/exo/pointer_... File components/exo/pointer_unittest.cc (right): https://codereview.chromium.org/2127263002/diff/20001/components/exo/pointer_... components/exo/pointer_unittest.cc:303: surface->window()->GetBoundsInScreen().origin() + gfx::Vector2d(1, 1); On 2016/07/08 21:26:51, oshima wrote: > since this is used only once, you can just inline > > moveMouseTo(location + gfx::VEctor2d(1, 1)) > > and so forth. Done. https://codereview.chromium.org/2127263002/diff/20001/components/exo/pointer_... components/exo/pointer_unittest.cc:304: gfx::Point location3 = surface->window()->GetBoundsInScreen().bottom_right(); On 2016/07/08 21:26:51, oshima wrote: > ditto. Done. https://codereview.chromium.org/2127263002/diff/20001/components/exo/pointer_... components/exo/pointer_unittest.cc:329: testing::Sequence s1, s2; On 2016/07/08 21:26:51, oshima wrote: > Can you do the following? I think it's easier to understand and read. > > { > testing::Sequence seq; > EXPECT_ > EXPECT_ > } > generator.MoveMouseTo() > > { > testing::Sequence seq; > EXPECT... > EXPECT... > } > generator.ScrollSequence(..) > > same for the touch_unittest.cc Done.
The CQ bit was checked by hariank@google.com 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
Forgot to mention. I'm not the owner of exo. Please send it to reveman@ as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hariank@google.com changed reviewers: + reveman@chromium.org
https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.... components/exo/pointer.cc:128: if (!ash::Shell::GetInstance()->CanWindowReceiveEvents( Can you add a short comment above this to explain why this might return false and why we should then ignore the event? https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.... components/exo/pointer.cc:130: event->StopPropagation(); Is it really correct to call StopPropagation here? Isn't that supposed to mean that the event was consumed? and we're not really consuming the event, just ignoring it. https://codereview.chromium.org/2127263002/diff/40001/components/exo/touch.cc File components/exo/touch.cc (right): https://codereview.chromium.org/2127263002/diff/40001/components/exo/touch.cc... components/exo/touch.cc:49: if (!ash::Shell::GetInstance()->CanWindowReceiveEvents( Same short comment here https://codereview.chromium.org/2127263002/diff/40001/components/exo/touch.cc... components/exo/touch.cc:50: (aura::Window*)event->target())) { use static_cast instead https://codereview.chromium.org/2127263002/diff/40001/components/exo/touch.cc... components/exo/touch.cc:51: event->StopPropagation(); Same question as for exo::Pointer. Should we be calling this?
https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.... components/exo/pointer.cc:126: Surface* target = GetEffectiveTargetForEvent(event); can you move this after the following check? same for the touch.cc https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.... components/exo/pointer.cc:130: event->StopPropagation(); On 2016/07/13 18:55:09, reveman wrote: > Is it really correct to call StopPropagation here? Isn't that supposed to mean > that the event was consumed? and we're not really consuming the event, just > ignoring it. Shell::CanWindowReceiveEvents is used to block event in system modal. Note that the event->target() isn't the exo widget, so this is ok. (and this is same as what SystemModalContainerEventFilter does). That's being said, we should test the case where it does not block events that are targeted to arc++ modal windows. hariank@, can you add a test case for it?
https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.... components/exo/pointer.cc:126: Surface* target = GetEffectiveTargetForEvent(event); On 2016/07/13 21:34:00, oshima wrote: > can you move this after the following check? > > same for the touch.cc Done. https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.... components/exo/pointer.cc:126: Surface* target = GetEffectiveTargetForEvent(event); On 2016/07/13 21:34:00, oshima wrote: > can you move this after the following check? > > same for the touch.cc Done. https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.... components/exo/pointer.cc:128: if (!ash::Shell::GetInstance()->CanWindowReceiveEvents( On 2016/07/13 18:55:09, reveman wrote: > Can you add a short comment above this to explain why this might return false > and why we should then ignore the event? Done. https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.... components/exo/pointer.cc:130: event->StopPropagation(); On 2016/07/13 21:34:00, oshima wrote: > On 2016/07/13 18:55:09, reveman wrote: > > Is it really correct to call StopPropagation here? Isn't that supposed to mean > > that the event was consumed? and we're not really consuming the event, just > > ignoring it. > > Shell::CanWindowReceiveEvents is used to block event in system modal. Note that > the event->target() isn't the exo widget, so this is ok. > (and this is same as what SystemModalContainerEventFilter does). > > That's being said, we should test the case where it does not block events that > are targeted to arc++ modal windows. hariank@, can you add a test case for it? Done. https://codereview.chromium.org/2127263002/diff/40001/components/exo/touch.cc File components/exo/touch.cc (right): https://codereview.chromium.org/2127263002/diff/40001/components/exo/touch.cc... components/exo/touch.cc:49: if (!ash::Shell::GetInstance()->CanWindowReceiveEvents( On 2016/07/13 18:55:09, reveman wrote: > Same short comment here Done. https://codereview.chromium.org/2127263002/diff/40001/components/exo/touch.cc... components/exo/touch.cc:50: (aura::Window*)event->target())) { On 2016/07/13 18:55:09, reveman wrote: > use static_cast instead Done.
https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.... components/exo/pointer.cc:130: event->StopPropagation(); On 2016/07/13 at 21:34:00, oshima wrote: > On 2016/07/13 18:55:09, reveman wrote: > > Is it really correct to call StopPropagation here? Isn't that supposed to mean > > that the event was consumed? and we're not really consuming the event, just > > ignoring it. > > Shell::CanWindowReceiveEvents is used to block event in system modal. Note that the event->target() isn't the exo widget, so this is ok. > (and this is same as what SystemModalContainerEventFilter does). I'm still failing to see why StopPropagation should be called here. I understand that it doesn't really do any real harm now but it still seems incorrect to me. It makes sense that SystemModalContainerEventFilter consumes the event as it's supposed to prevent the event from being delivered to other event handlers. However, that's not the purpose of Pointer::OnMouseEvent, Pointer::OnMouseEvent just does pre processing of events potentially targeted at exo windows. I would not expect the presence of an exo::Pointer instance to have an impact on further processing of the event. Keep in mind that we might have exo windows but zero exo pointer instances or N pointer instances. I'd expect the state of the event to be the same when it's processed by pointer 1 and pointer 2 and not be marked as consumed when it hits pointer 2 when it was not marked this way when hitting pointer 1. > > That's being said, we should test the case where it does not block events that are targeted to arc++ modal windows. hariank@, can you add a test case for it?
https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.... components/exo/pointer.cc:130: event->StopPropagation(); On 2016/07/14 11:15:40, reveman wrote: > On 2016/07/13 at 21:34:00, oshima wrote: > > On 2016/07/13 18:55:09, reveman wrote: > > > Is it really correct to call StopPropagation here? Isn't that supposed to > mean > > > that the event was consumed? and we're not really consuming the event, just > > > ignoring it. > > > > Shell::CanWindowReceiveEvents is used to block event in system modal. Note > that the event->target() isn't the exo widget, so this is ok. > > (and this is same as what SystemModalContainerEventFilter does). > > I'm still failing to see why StopPropagation should be called here. I understand > that it doesn't really do any real harm now but it still seems incorrect to me. > It makes sense that SystemModalContainerEventFilter consumes the event as it's > supposed to prevent the event from being delivered to other event handlers. > However, that's not the purpose of Pointer::OnMouseEvent, Pointer::OnMouseEvent > just does pre processing of events potentially targeted at exo windows. I think this is misunderstanding. The event received in pre target handler may or may not be exo windows. In other words, it's called for every targets, before the event is being sent to that target, which may or may not be exo window. And CanWindowReceiveEvents checks if that target window can receive an event, not an exo pointer. GetEffectiveTargetForEvent(event); filters out non exo windows, and that's why the reset of the code works for pointer. I still think this should not use pre target handler, but I think that this is safer to merge. We can revisit this for m54 though. > I would > not expect the presence of an exo::Pointer instance to have an impact on further > processing of the event. Keep in mind that we might have exo windows but zero > exo pointer instances or N pointer instances. I'd expect the state of the event > to be the same when it's processed by pointer 1 and pointer 2 and not be marked > as consumed when it hits pointer 2 when it was not marked this way when hitting > pointer 1. > > > > > That's being said, we should test the case where it does not block events that > are targeted to arc++ modal windows. hariank@, can you add a test case for it? >
https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.... components/exo/pointer.cc:130: event->StopPropagation(); On 2016/07/14 at 12:21:27, oshima wrote: > On 2016/07/14 11:15:40, reveman wrote: > > On 2016/07/13 at 21:34:00, oshima wrote: > > > On 2016/07/13 18:55:09, reveman wrote: > > > > Is it really correct to call StopPropagation here? Isn't that supposed to > > mean > > > > that the event was consumed? and we're not really consuming the event, just > > > > ignoring it. > > > > > > Shell::CanWindowReceiveEvents is used to block event in system modal. Note > > that the event->target() isn't the exo widget, so this is ok. > > > (and this is same as what SystemModalContainerEventFilter does). > > > > I'm still failing to see why StopPropagation should be called here. I understand > > that it doesn't really do any real harm now but it still seems incorrect to me. > > It makes sense that SystemModalContainerEventFilter consumes the event as it's > > supposed to prevent the event from being delivered to other event handlers. > > However, that's not the purpose of Pointer::OnMouseEvent, Pointer::OnMouseEvent > > just does pre processing of events potentially targeted at exo windows. > > I think this is misunderstanding. The event received in pre target handler may or may not be > exo windows. In other words, it's called for every targets, before the event is being > sent to that target, which may or may not be exo window. > And CanWindowReceiveEvents checks if that target window can receive an event, not an exo pointer. Right, pre target handler is for all windows. I have no problem with calling CanWindowReceiveEvents here, that makes perfect sense. What I don't understand is why calling StopPropagation is correct, especially before determining if the target is an exo window. To me it seems more correct to not call StopPropagation at all. Please explain why it makes sense to have this call here. > > GetEffectiveTargetForEvent(event); > > filters out non exo windows, and that's why the reset of the code works for pointer. > > I still think this should not use pre target handler, but I think that this is safer to merge. > We can revisit this for m54 though. > > > I would > > not expect the presence of an exo::Pointer instance to have an impact on further > > processing of the event. Keep in mind that we might have exo windows but zero > > exo pointer instances or N pointer instances. I'd expect the state of the event > > to be the same when it's processed by pointer 1 and pointer 2 and not be marked > > as consumed when it hits pointer 2 when it was not marked this way when hitting > > pointer 1. > > > > > > > > That's being said, we should test the case where it does not block events that > > are targeted to arc++ modal windows. hariank@, can you add a test case for it? > >
https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2127263002/diff/40001/components/exo/pointer.... components/exo/pointer.cc:130: event->StopPropagation(); On 2016/07/14 13:59:49, reveman wrote: > On 2016/07/14 at 12:21:27, oshima wrote: > > On 2016/07/14 11:15:40, reveman wrote: > > > On 2016/07/13 at 21:34:00, oshima wrote: > > > > On 2016/07/13 18:55:09, reveman wrote: > > > > > Is it really correct to call StopPropagation here? Isn't that supposed > to > > > mean > > > > > that the event was consumed? and we're not really consuming the event, > just > > > > > ignoring it. > > > > > > > > Shell::CanWindowReceiveEvents is used to block event in system modal. Note > > > that the event->target() isn't the exo widget, so this is ok. > > > > (and this is same as what SystemModalContainerEventFilter does). > > > > > > I'm still failing to see why StopPropagation should be called here. I > understand > > > that it doesn't really do any real harm now but it still seems incorrect to > me. > > > It makes sense that SystemModalContainerEventFilter consumes the event as > it's > > > supposed to prevent the event from being delivered to other event handlers. > > > However, that's not the purpose of Pointer::OnMouseEvent, > Pointer::OnMouseEvent > > > just does pre processing of events potentially targeted at exo windows. > > > > I think this is misunderstanding. The event received in pre target handler may > or may not be > > exo windows. In other words, it's called for every targets, before the event > is being > > sent to that target, which may or may not be exo window. > > And CanWindowReceiveEvents checks if that target window can receive an event, > not an exo pointer. > > Right, pre target handler is for all windows. I have no problem with calling > CanWindowReceiveEvents here, that makes perfect sense. What I don't understand > is why calling StopPropagation is correct, especially before determining if the > target is an exo window. To me it seems more correct to not call StopPropagation > at all. Please explain why it makes sense to have this call here. We can skip it if you feel strongly about it, it will be called in SystemModalContainerEventHandler again anyway, which is a bit waste. This peace of code is generic and unrelated to exo, and we copied it from SystemModalContainerEventHandler because this handler handles events before SystemModalContainerEventHandler process the event. (which is added on the fly) Actually I have an alternative idea how to fix this. Please hold on. > > > > GetEffectiveTargetForEvent(event); > > > > filters out non exo windows, and that's why the reset of the code works for > pointer. > > > > I still think this should not use pre target handler, but I think that this is > safer to merge. > > We can revisit this for m54 though. > > > > > I would > > > not expect the presence of an exo::Pointer instance to have an impact on > further > > > processing of the event. Keep in mind that we might have exo windows but > zero > > > exo pointer instances or N pointer instances. I'd expect the state of the > event > > > to be the same when it's processed by pointer 1 and pointer 2 and not be > marked > > > as consumed when it hits pointer 2 when it was not marked this way when > hitting > > > pointer 1. > > > > > > > > > > > That's being said, we should test the case where it does not block events > that > > > are targeted to arc++ modal windows. hariank@, can you add a test case for > it? > > > >
The CQ bit was checked by hariank@google.com 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.
lgtm could you please update the CL description as well? It should be something like Keep the SystemModalCOntainerEventHandler added, and check the modality in the event handler, instead of adding/removing the event handler. https://codereview.chromium.org/2127263002/diff/80001/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/2127263002/diff/80001/ash/shell.cc#newcode750 ash/shell.cc:750: reset the event handler here.
Description was changed from ========== Ignore touch/pointer events on Arc windows if modal open BUG=624168 ========== to ========== Keep the SystemModalContainerEventHandler added, and check the modality in the event handler, instead of adding/removing the event handler. BUG=624168 TEST=PointerTest.IgnorePointerEventDuringModal,TouchTest.IgnoreTouchEventDuringModal ==========
Description was changed from ========== Keep the SystemModalContainerEventHandler added, and check the modality in the event handler, instead of adding/removing the event handler. BUG=624168 TEST=PointerTest.IgnorePointerEventDuringModal,TouchTest.IgnoreTouchEventDuringModal ========== to ========== Keep the SystemModalContainerEventHandler added, and check the modality in the event handler, instead of adding/removing the event handler. Ensures that pointer/touch events on Arc windows are not registered when a system modal is open. BUG=624168 TEST=PointerTest.IgnorePointerEventDuringModal,TouchTest.IgnoreTouchEventDuringModal ==========
components/exo lgtm
The CQ bit was checked by hariank@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2127263002/#ps100001 (title: "Keep the SystemModalContainerEventHandler added")
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.
Description was changed from ========== Keep the SystemModalContainerEventHandler added, and check the modality in the event handler, instead of adding/removing the event handler. Ensures that pointer/touch events on Arc windows are not registered when a system modal is open. BUG=624168 TEST=PointerTest.IgnorePointerEventDuringModal,TouchTest.IgnoreTouchEventDuringModal ========== to ========== Keep the SystemModalContainerEventHandler added, and check the modality in the event handler, instead of adding/removing the event handler. Ensures that pointer/touch events on Arc windows are not registered when a system modal is open. BUG=624168 TEST=PointerTest.IgnorePointerEventDuringModal,TouchTest.IgnoreTouchEventDuringModal ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Keep the SystemModalContainerEventHandler added, and check the modality in the event handler, instead of adding/removing the event handler. Ensures that pointer/touch events on Arc windows are not registered when a system modal is open. BUG=624168 TEST=PointerTest.IgnorePointerEventDuringModal,TouchTest.IgnoreTouchEventDuringModal ========== to ========== Keep the SystemModalContainerEventHandler added, and check the modality in the event handler, instead of adding/removing the event handler. Ensures that pointer/touch events on Arc windows are not registered when a system modal is open. BUG=624168 TEST=PointerTest.IgnorePointerEventDuringModal,TouchTest.IgnoreTouchEventDuringModal Committed: https://crrev.com/0dd03dd13d630af3457d4f43beead3b83e95956a Cr-Commit-Position: refs/heads/master@{#405632} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0dd03dd13d630af3457d4f43beead3b83e95956a Cr-Commit-Position: refs/heads/master@{#405632} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
