Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2095873004:
Ink drops with touch input are redundant on Windows since the OS does something similar (Closed)
DescriptionDon't show ink drops with touch events on Windows
BUG=595315
TBR=bruthig@chromium.org
Committed: https://crrev.com/d480e04cbde45c22342599468c3081f586b4523c
Cr-Commit-Position: refs/heads/master@{#402239}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Added comment to explain somewhat obtuse 'if' statement #
Total comments: 1
Patch Set 3 : Rework comments and the code based on feedback in the review #
Total comments: 1
Patch Set 4 : Spelling error correction #
Total comments: 8
Messages
Total messages: 32 (11 generated)
Description was changed from ========== don't show ink drops with touch events on Windows...mostly BUG= ========== to ========== Don't show ink drops with touch events on Windows BUG=595315 ==========
kylixrd@chromium.org changed reviewers: + sky@chromium.org
sky@, bruthig@ requested that you provide review cover for this CL since he's OOO until the 27th.
sky@chromium.org changed reviewers: + estade@chromium.org
+estade as he knows this code more than I. https://codereview.chromium.org/2095873004/diff/1/ui/views/animation/ink_drop... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2095873004/diff/1/ui/views/animation/ink_drop... ui/views/animation/ink_drop_host_view.cc:176: if ((state == InkDropState::ACTION_PENDING || Is there a reason why you care about the state in this case? Isn't the event enough?
estade@chromium.org changed reviewers: + bruthig@chromium.org
+bruthig (sadly ooo) is the only one who truly understands the InkDropState enum. https://codereview.chromium.org/2095873004/diff/1/ui/views/animation/ink_drop... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2095873004/diff/1/ui/views/animation/ink_drop... ui/views/animation/ink_drop_host_view.cc:176: if ((state == InkDropState::ACTION_PENDING || On 2016/06/24 19:21:11, sky wrote: > Is there a reason why you care about the state in this case? Isn't the event > enough? I imagine this is to handle cases like if you were to press down with the mouse then tap the view before releasing the mouse button? I'd go for simpler code instead of trying to support a crazy use case like this, unless the simpler code makes things look totally broken. Either way, docs would be nice because this block is hard to grok.
https://codereview.chromium.org/2095873004/diff/1/ui/views/animation/ink_drop... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2095873004/diff/1/ui/views/animation/ink_drop... ui/views/animation/ink_drop_host_view.cc:176: if ((state == InkDropState::ACTION_PENDING || On 2016/06/24 19:21:11, sky wrote: > Is there a reason why you care about the state in this case? Isn't the event > enough? Only certain states or actions should be allowed to propagate when the event source is touch (or gestures). Because Windows provides its own touch feedback that is similar to the ink-drop feedback, it is favored. Only when the ink is already present (from a mouse-click or focus event) should the state be transitioned regardless of the event source. At least that is my understanding of the requirement.
On 2016/06/24 20:28:30, Evan Stade wrote: > +bruthig (sadly ooo) is the only one who truly understands the InkDropState > enum. > > https://codereview.chromium.org/2095873004/diff/1/ui/views/animation/ink_drop... > File ui/views/animation/ink_drop_host_view.cc (right): > > https://codereview.chromium.org/2095873004/diff/1/ui/views/animation/ink_drop... > ui/views/animation/ink_drop_host_view.cc:176: if ((state == > InkDropState::ACTION_PENDING || > On 2016/06/24 19:21:11, sky wrote: > > Is there a reason why you care about the state in this case? Isn't the event > > enough? > > I imagine this is to handle cases like if you were to press down with the mouse > then tap the view before releasing the mouse button? > > I'd go for simpler code instead of trying to support a crazy use case like this, > unless the simpler code makes things look totally broken. Either way, docs would > be nice because this block is hard to grok. This code was the result of the discussion here: https://bugs.chromium.org/p/chromium/issues/detail?id=595315. The primary reason for this code is to ensure the ink-drop never gets "stuck" because of mismatched events or states. The idea is that the ink-drop should never transition to a "pending" state from a touch event. This is the first part of the conditional: state == InkDropState::ACTION_PENDING || state == InkDropState::ALTERNATE_ACTION_PENDING If the state is already "pending", presumably, from a non-touch event such as mouse or keyboard, then a triggered action state should be allowed. The second part of the conditional: (state == InkDropState::ACTION_TRIGGERED || status == InkDropState::ALTERNATE_ACTION_TRIGGERED) && ink_drop_->GetTargetInkDropState() == InkDropState::HIDDEN Finally, all of the above is predicated on the event itself being a gesture/touch event: event && event->IsGestureEvent() When you say "docs", are you referring to a comment in the code or some actual doc with a link in a comment in the code? Either way, I can clean up the above and put it wherever.
On 2016/06/24 20:44:36, kylix_rd wrote: > On 2016/06/24 20:28:30, Evan Stade wrote: > > +bruthig (sadly ooo) is the only one who truly understands the InkDropState > > enum. > > > > > https://codereview.chromium.org/2095873004/diff/1/ui/views/animation/ink_drop... > > File ui/views/animation/ink_drop_host_view.cc (right): > > > > > https://codereview.chromium.org/2095873004/diff/1/ui/views/animation/ink_drop... > > ui/views/animation/ink_drop_host_view.cc:176: if ((state == > > InkDropState::ACTION_PENDING || > > On 2016/06/24 19:21:11, sky wrote: > > > Is there a reason why you care about the state in this case? Isn't the event > > > enough? > > > > I imagine this is to handle cases like if you were to press down with the > mouse > > then tap the view before releasing the mouse button? > > > > I'd go for simpler code instead of trying to support a crazy use case like > this, > > unless the simpler code makes things look totally broken. Either way, docs > would > > be nice because this block is hard to grok. > > This code was the result of the discussion here: > https://bugs.chromium.org/p/chromium/issues/detail?id=595315. The primary reason > for this code is to ensure the ink-drop never gets "stuck" because of mismatched > events or states. > > The idea is that the ink-drop should never transition to a "pending" state from > a touch event. This is the first part of the conditional: > > state == InkDropState::ACTION_PENDING || state == > InkDropState::ALTERNATE_ACTION_PENDING > > If the state is already "pending", presumably, from a non-touch event such as > mouse or keyboard, then a triggered action state should be allowed. The second > part of the conditional: > > (state == InkDropState::ACTION_TRIGGERED || status == > InkDropState::ALTERNATE_ACTION_TRIGGERED) && ink_drop_->GetTargetInkDropState() > == InkDropState::HIDDEN > > Finally, all of the above is predicated on the event itself being a > gesture/touch event: > > event && event->IsGestureEvent() > > When you say "docs", are you referring to a comment in the code or some actual > doc with a link in a comment in the code? Either way, I can clean up the above > and put it wherever. comments in the code or self documenting code. One way to make it easier to follow is to break it up into multiple conditions (even though it's possible to make one big block of boolean logic, it's not always the easiest to follow). Your explanation here is good so copying some form of that into the code would be nice.
Added comment to the code explaining logic behind the 'if' statement.
https://codereview.chromium.org/2095873004/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2095873004/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:176: // The reason for this code is to ensure the ink-drop never gets "stuck" I think you're skipping ahead a bit here, imo the right lead would be "Don't initiate ink drops for touch events on Windows." Also I'd much prefer if you didn't copy the code into the comments but integrated the code and comments instead. How about this: // Don't create new ink drops for touch events on Windows. However, touch events must dismiss existing ink drop animations. if (event && event->IsGestureEvent()) { // The ink-drop should never transition to a "pending" state from a touch // event. if (state == InkDropState::ACTION_PENDING || state == InkDropState::ALTERNATE_ACTION_PENDING) return; // If the state is already "pending", presumably from a non-touch event such // as mouse or keyboard, then a triggered action state should be allowed. Conversely if the state is already targeted to hidden then the touch should be ignored. if ((state == InkDropState::ACTION_TRIGGERED || state == InkDropState::ALTERNATE_ACTION_TRIGGERED) && ink_drop_->GetTargetInkDropState() == InkDropState::HIDDEN) return; }
On 2016/06/27 15:19:46, Evan Stade wrote: > I think you're skipping ahead a bit here, imo the right lead would be "Don't > initiate ink drops for touch events on Windows." Also I'd much prefer if you > didn't copy the code into the comments but integrated the code and comments > instead. The feedback is appreciated. This latest patch should be much more clear and concise.
lgtm https://codereview.chromium.org/2095873004/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2095873004/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:179: // Don't transtion the ink-drop to a "pending" state from a touch event. nit: sp (transition)
kylixrd@chromium.org changed reviewers: - bruthig@chromium.org, sky@chromium.org
Description was changed from ========== Don't show ink drops with touch events on Windows BUG=595315 ========== to ========== Don't show ink drops with touch events on Windows BUG=595315 TBR=bruthig@chromium.org ==========
I moved sky@ to the CC who deferred to estade@. TBR'd bruthig@ who is OOO.
The CQ bit was checked by kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org Link to the patchset: https://codereview.chromium.org/2095873004/#ps60001 (title: "Spelling error correction")
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 ========== Don't show ink drops with touch events on Windows BUG=595315 TBR=bruthig@chromium.org ========== to ========== Don't show ink drops with touch events on Windows BUG=595315 TBR=bruthig@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Don't show ink drops with touch events on Windows BUG=595315 TBR=bruthig@chromium.org ========== to ========== Don't show ink drops with touch events on Windows BUG=595315 TBR=bruthig@chromium.org Committed: https://crrev.com/d480e04cbde45c22342599468c3081f586b4523c Cr-Commit-Position: refs/heads/master@{#402239} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d480e04cbde45c22342599468c3081f586b4523c Cr-Commit-Position: refs/heads/master@{#402239}
Message was sent while issue was closed.
bruthig@chromium.org changed reviewers: + bruthig@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:178: if (event && event->IsGestureEvent()) { This would definitely benefit from some tests. https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:189: ink_drop_->GetTargetInkDropState() == InkDropState::HIDDEN) What is the use-case for the "&& HIDDEN" clause here? Should it be a '!=' instead of an '=='? After thinking about this a bit more, does the following logic simplify things? Essentially we would only prevent ripples from transitioning from HIDDEN to visible on gesture events, unless it is an ACTIVATED state. if (event && event->IsGestureEvent() && ink_drop_->GetTargetInkDropState() == HIDDEN && state != ACTIVATED) { return; }
Message was sent while issue was closed.
https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:189: ink_drop_->GetTargetInkDropState() == InkDropState::HIDDEN) On 2016/06/30 00:23:11, bruthig wrote: > What is the use-case for the "&& HIDDEN" clause here? Should it be a '!=' > instead of an '=='? The idea here is that a triggered action from a touch event should only animate if the state is PENDING. If the current state is HIDDEN, then don't show the TRIGGERED animation. > After thinking about this a bit more, does the following logic simplify things? > Essentially we would only prevent ripples from transitioning from HIDDEN to > visible on gesture events, unless it is an ACTIVATED state. > > if (event && > event->IsGestureEvent() && > ink_drop_->GetTargetInkDropState() == HIDDEN && > state != ACTIVATED) { > return; > } What about DEACTIVATED? Would you not still want to allow that through as well? At least according to the comments in ink_drop_state.h.
Message was sent while issue was closed.
https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:189: ink_drop_->GetTargetInkDropState() == InkDropState::HIDDEN) On 2016/06/30 13:39:37, kylix_rd wrote: > On 2016/06/30 00:23:11, bruthig wrote: > > What is the use-case for the "&& HIDDEN" clause here? Should it be a '!=' > > instead of an '=='? > > The idea here is that a triggered action from a touch event should only animate > if the state is PENDING. If the current state is HIDDEN, then don't show the > TRIGGERED animation. Ah right, makes sense. I must have been tired. > > > After thinking about this a bit more, does the following logic simplify > things? > > Essentially we would only prevent ripples from transitioning from HIDDEN to > > visible on gesture events, unless it is an ACTIVATED state. > > > > if (event && > > event->IsGestureEvent() && > > ink_drop_->GetTargetInkDropState() == HIDDEN && > > state != ACTIVATED) { > > return; > > } > > What about DEACTIVATED? Would you not still want to allow that through as well? > At least according to the comments in ink_drop_state.h. The DEACTIVATED animations should all make it through because the ink_drop_->GetTargetInkDropState() should never be HIDDEN when a request for DEACTIVED happens. i.e. the DEACTIVATED state should be preceded by an ACTIVATED state and the ACTIVATED animations are shown for gestures. Even if we somehow got a request to show the DEACTIVATED animation when GetTargetInkDropState() is HIDDEN, I think it would be reasonable to not show it.
Message was sent while issue was closed.
https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:178: if (event && event->IsGestureEvent()) { On 2016/06/30 00:23:11, bruthig wrote: > This would definitely benefit from some tests. I presume you're referring to the whole block and not just the above test? https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:189: ink_drop_->GetTargetInkDropState() == InkDropState::HIDDEN) On 2016/06/30 17:21:49, bruthig wrote: > > > After thinking about this a bit more, does the following logic simplify > > things? > > > Essentially we would only prevent ripples from transitioning from HIDDEN to > > > visible on gesture events, unless it is an ACTIVATED state. > > > > > > if (event && > > > event->IsGestureEvent() && > > > ink_drop_->GetTargetInkDropState() == HIDDEN && > > > state != ACTIVATED) { > > > return; > > > } > > > > What about DEACTIVATED? Would you not still want to allow that through as > well? > > At least according to the comments in ink_drop_state.h. > > The DEACTIVATED animations should all make it through because the > ink_drop_->GetTargetInkDropState() should never be HIDDEN when a request for > DEACTIVED happens. i.e. the DEACTIVATED state should be preceded by an ACTIVATED > state and the ACTIVATED animations are shown for gestures. Even if we somehow > got a request to show the DEACTIVATED animation when GetTargetInkDropState() is > HIDDEN, I think it would be reasonable to not show it. Aside from this logic being a little more complicated than necessary, are there any cases not taken into account? Either way, I can create a new CL with your suggestions.
Message was sent while issue was closed.
https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:178: if (event && event->IsGestureEvent()) { On 2016/06/30 17:31:25, kylix_rd wrote: > On 2016/06/30 00:23:11, bruthig wrote: > > This would definitely benefit from some tests. > > I presume you're referring to the whole block and not just the above test? Correct, but at least the number of test cases required should be reduced with the simplified logic. https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:189: ink_drop_->GetTargetInkDropState() == InkDropState::HIDDEN) On 2016/06/30 17:31:25, kylix_rd wrote: > On 2016/06/30 17:21:49, bruthig wrote: > > > > After thinking about this a bit more, does the following logic simplify > > > things? > > > > Essentially we would only prevent ripples from transitioning from HIDDEN > to > > > > visible on gesture events, unless it is an ACTIVATED state. > > > > > > > > if (event && > > > > event->IsGestureEvent() && > > > > ink_drop_->GetTargetInkDropState() == HIDDEN && > > > > state != ACTIVATED) { > > > > return; > > > > } > > > > > > What about DEACTIVATED? Would you not still want to allow that through as > > well? > > > At least according to the comments in ink_drop_state.h. > > > > The DEACTIVATED animations should all make it through because the > > ink_drop_->GetTargetInkDropState() should never be HIDDEN when a request for > > DEACTIVED happens. i.e. the DEACTIVATED state should be preceded by an > ACTIVATED > > state and the ACTIVATED animations are shown for gestures. Even if we somehow > > got a request to show the DEACTIVATED animation when GetTargetInkDropState() > is > > HIDDEN, I think it would be reasonable to not show it. > > Aside from this logic being a little more complicated than necessary, are there > any cases not taken into account? Nothing specific to this CL that I can think of. There are still problems with multiple input sources interacting with a View but that is definitely outside the scope of this change. > > Either way, I can create a new CL with your suggestions. Awesome, thanks :) Sorry I wasn't more on the ball earlier.
Message was sent while issue was closed.
On 2016/06/30 17:35:36, bruthig wrote: > https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... > File ui/views/animation/ink_drop_host_view.cc (right): > > https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... > ui/views/animation/ink_drop_host_view.cc:178: if (event && > event->IsGestureEvent()) { > On 2016/06/30 17:31:25, kylix_rd wrote: > > On 2016/06/30 00:23:11, bruthig wrote: > > > This would definitely benefit from some tests. > > > > I presume you're referring to the whole block and not just the above test? > > Correct, but at least the number of test cases required should be reduced with > the simplified logic. > > https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... > ui/views/animation/ink_drop_host_view.cc:189: ink_drop_->GetTargetInkDropState() > == InkDropState::HIDDEN) > On 2016/06/30 17:31:25, kylix_rd wrote: > > On 2016/06/30 17:21:49, bruthig wrote: > > > > > After thinking about this a bit more, does the following logic simplify > > > > things? > > > > > Essentially we would only prevent ripples from transitioning from HIDDEN > > to > > > > > visible on gesture events, unless it is an ACTIVATED state. > > > > > > > > > > if (event && > > > > > event->IsGestureEvent() && > > > > > ink_drop_->GetTargetInkDropState() == HIDDEN && > > > > > state != ACTIVATED) { > > > > > return; > > > > > } > > > > > > > > What about DEACTIVATED? Would you not still want to allow that through as > > > well? > > > > At least according to the comments in ink_drop_state.h. > > > > > > The DEACTIVATED animations should all make it through because the > > > ink_drop_->GetTargetInkDropState() should never be HIDDEN when a request for > > > DEACTIVED happens. i.e. the DEACTIVATED state should be preceded by an > > ACTIVATED > > > state and the ACTIVATED animations are shown for gestures. Even if we > somehow > > > got a request to show the DEACTIVATED animation when GetTargetInkDropState() > > is > > > HIDDEN, I think it would be reasonable to not show it. > > > > Aside from this logic being a little more complicated than necessary, are > there > > any cases not taken into account? > > Nothing specific to this CL that I can think of. There are still problems with > multiple input sources interacting with a View but that is definitely outside > the scope of this change. > > > > > Either way, I can create a new CL with your suggestions. > > Awesome, thanks :) Sorry I wasn't more on the ball earlier. Do you have any objections to this code going out in M53. I can get the tests written and the logic tweaked by late next week (U.S. Holidays on 4th & 5th). The expected M53 branch, however, is tomorrow. Otherwise, I'll need to get it merged over to the branch.
Message was sent while issue was closed.
On 2016/06/30 18:27:58, kylix_rd wrote: > On 2016/06/30 17:35:36, bruthig wrote: > > > https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... > > File ui/views/animation/ink_drop_host_view.cc (right): > > > > > https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... > > ui/views/animation/ink_drop_host_view.cc:178: if (event && > > event->IsGestureEvent()) { > > On 2016/06/30 17:31:25, kylix_rd wrote: > > > On 2016/06/30 00:23:11, bruthig wrote: > > > > This would definitely benefit from some tests. > > > > > > I presume you're referring to the whole block and not just the above test? > > > > Correct, but at least the number of test cases required should be reduced with > > the simplified logic. > > > > > https://codereview.chromium.org/2095873004/diff/60001/ui/views/animation/ink_... > > ui/views/animation/ink_drop_host_view.cc:189: > ink_drop_->GetTargetInkDropState() > > == InkDropState::HIDDEN) > > On 2016/06/30 17:31:25, kylix_rd wrote: > > > On 2016/06/30 17:21:49, bruthig wrote: > > > > > > After thinking about this a bit more, does the following logic > simplify > > > > > things? > > > > > > Essentially we would only prevent ripples from transitioning from > HIDDEN > > > to > > > > > > visible on gesture events, unless it is an ACTIVATED state. > > > > > > > > > > > > if (event && > > > > > > event->IsGestureEvent() && > > > > > > ink_drop_->GetTargetInkDropState() == HIDDEN && > > > > > > state != ACTIVATED) { > > > > > > return; > > > > > > } > > > > > > > > > > What about DEACTIVATED? Would you not still want to allow that through > as > > > > well? > > > > > At least according to the comments in ink_drop_state.h. > > > > > > > > The DEACTIVATED animations should all make it through because the > > > > ink_drop_->GetTargetInkDropState() should never be HIDDEN when a request > for > > > > DEACTIVED happens. i.e. the DEACTIVATED state should be preceded by an > > > ACTIVATED > > > > state and the ACTIVATED animations are shown for gestures. Even if we > > somehow > > > > got a request to show the DEACTIVATED animation when > GetTargetInkDropState() > > > is > > > > HIDDEN, I think it would be reasonable to not show it. > > > > > > Aside from this logic being a little more complicated than necessary, are > > there > > > any cases not taken into account? > > > > Nothing specific to this CL that I can think of. There are still problems > with > > multiple input sources interacting with a View but that is definitely outside > > the scope of this change. > > > > > > > > Either way, I can create a new CL with your suggestions. > > > > Awesome, thanks :) Sorry I wasn't more on the ball earlier. > > Do you have any objections to this code going out in M53. I can get the tests > written and the logic tweaked by late next week (U.S. Holidays on 4th & 5th). > The expected M53 branch, however, is tomorrow. Otherwise, I'll need to get it > merged over to the branch. This is quite a small change and since branch point just happened. It probably makes sense to merge your fixes back. |
