Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
560473003:
DateView Touch Feedback (Closed)
DescriptionDateView Touch Feedback
Add visual feedback to touch input on DateView.
TEST=DateViewTest.TouchFeedback, DateViewTest.TouchFeedbackCancellation
BUG=398398
Committed: https://crrev.com/426a9fde95b04bbcfb1adfbf05003243f8c80593
Cr-Commit-Position: refs/heads/master@{#298702}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 5
Patch Set 3 : #
Total comments: 3
Patch Set 4 : Start using new switch #
Total comments: 1
Patch Set 5 : Remove unneeded flag and test #Patch Set 6 : Rebase #Messages
Total messages: 21 (4 generated)
jonross@chromium.org changed reviewers: + flackr@chromium.org
And another https://codereview.chromium.org/560473003/diff/1/ash/system/date/date_view.cc File ash/system/date/date_view.cc (right): https://codereview.chromium.org/560473003/diff/1/ash/system/date/date_view.cc... ash/system/date/date_view.cc:297: void TimeView::UpdateClockLayout(TrayDate::ClockLayout clock_layout) { cpplint
/cc +tdanderson@ https://codereview.chromium.org/560473003/diff/1/ash/system/date/date_view.cc File ash/system/date/date_view.cc (right): https://codereview.chromium.org/560473003/diff/1/ash/system/date/date_view.cc... ash/system/date/date_view.cc:227: event->type() == ui::ET_GESTURE_TAP) { Can you use ET_GESTURE_END intead?
tdanderson@chromium.org changed reviewers: + tdanderson@chromium.org
https://codereview.chromium.org/560473003/diff/1/ash/system/date/date_view.cc File ash/system/date/date_view.cc (right): https://codereview.chromium.org/560473003/diff/1/ash/system/date/date_view.cc... ash/system/date/date_view.cc:227: event->type() == ui::ET_GESTURE_TAP) { On 2014/09/09 19:07:18, sadrul wrote: > Can you use ET_GESTURE_END intead? From looking at your test coverage, it seems you want to cancel the active effect if the finger moves too. So if you call SetActive(false) on SCROLL_BEGIN || TAP_CANCEL || END, I believe that should be sufficient.
https://codereview.chromium.org/560473003/diff/1/ash/system/date/date_view.cc File ash/system/date/date_view.cc (right): https://codereview.chromium.org/560473003/diff/1/ash/system/date/date_view.cc... ash/system/date/date_view.cc:227: event->type() == ui::ET_GESTURE_TAP) { On 2014/09/09 19:46:47, tdanderson wrote: > On 2014/09/09 19:07:18, sadrul wrote: > > Can you use ET_GESTURE_END intead? > > From looking at your test coverage, it seems you want to cancel the active > effect if the finger moves too. So if you call SetActive(false) on SCROLL_BEGIN > || TAP_CANCEL || END, I believe that should be sufficient. Done.
https://codereview.chromium.org/560473003/diff/20001/ash/system/date/date_vie... File ash/system/date/date_view.cc (right): https://codereview.chromium.org/560473003/diff/20001/ash/system/date/date_vie... ash/system/date/date_view.cc:227: event->type() == ui::ET_GESTURE_END) { According to http://www.chromium.org/developers/design-documents/aura/gesture-recognizer TAP_CANCEL is fired everytime a TAP_DOWN doesn't turn into a tap, so you should be able to cancel the effect on either TAP_CANCEL, or TAP.
https://codereview.chromium.org/560473003/diff/20001/ash/system/date/date_vie... File ash/system/date/date_view.cc (right): https://codereview.chromium.org/560473003/diff/20001/ash/system/date/date_vie... ash/system/date/date_view.cc:227: event->type() == ui::ET_GESTURE_END) { On 2014/09/09 21:58:09, flackr wrote: > According to > http://www.chromium.org/developers/design-documents/aura/gesture-recognizer > TAP_CANCEL is fired everytime a TAP_DOWN doesn't turn into a tap, so you should > be able to cancel the effect on either TAP_CANCEL, or TAP. On previous patch it was request to go from END to TAP as a condition. SCROLL_BEGIN and TAP_CANCEL cover our early dismissal for non-tap gestures. When do we want to stop visuals for the normal case? TAP or END?
https://codereview.chromium.org/560473003/diff/20001/ash/system/date/date_vie... File ash/system/date/date_view.cc (right): https://codereview.chromium.org/560473003/diff/20001/ash/system/date/date_vie... ash/system/date/date_view.cc:227: event->type() == ui::ET_GESTURE_END) { On 2014/09/11 14:35:18, jonross wrote: > On 2014/09/09 21:58:09, flackr wrote: > > According to > > http://www.chromium.org/developers/design-documents/aura/gesture-recognizer > > TAP_CANCEL is fired everytime a TAP_DOWN doesn't turn into a tap, so you > should > > be able to cancel the effect on either TAP_CANCEL, or TAP. > > On previous patch it was request to go from END to TAP as a condition. > > SCROLL_BEGIN and TAP_CANCEL cover our early dismissal for non-tap gestures. Don't we get TAP_CANCEL before SCROLL_BEGIN? > > When do we want to stop visuals for the normal case? TAP or END? Ah I see, sadrul why do we want to wait until END to cancel? I thought ending on tap and tap_cancel nicely encapsulates the intended effect of indicating when your touch will turn into a tap.
https://codereview.chromium.org/560473003/diff/20001/ash/system/date/date_vie... File ash/system/date/date_view.cc (right): https://codereview.chromium.org/560473003/diff/20001/ash/system/date/date_vie... ash/system/date/date_view.cc:227: event->type() == ui::ET_GESTURE_END) { On 2014/09/11 21:07:42, flackr wrote: > On 2014/09/11 14:35:18, jonross wrote: > > On 2014/09/09 21:58:09, flackr wrote: > > > According to > > > http://www.chromium.org/developers/design-documents/aura/gesture-recognizer > > > TAP_CANCEL is fired everytime a TAP_DOWN doesn't turn into a tap, so you > > should > > > be able to cancel the effect on either TAP_CANCEL, or TAP. > > > > On previous patch it was request to go from END to TAP as a condition. > > > > SCROLL_BEGIN and TAP_CANCEL cover our early dismissal for non-tap gestures. > > Don't we get TAP_CANCEL before SCROLL_BEGIN? I believe we do. > > > > > When do we want to stop visuals for the normal case? TAP or END? > > Ah I see, sadrul why do we want to wait until END to cancel? I thought ending on > tap and tap_cancel nicely encapsulates the intended effect of indicating when > your touch will turn into a tap. ISTR there are cases where we don't trigger TAP_CANCEL (e.g. for long-press). Terry can confirm. So instead of having to enumerate all the terminal gesture events in code like this, we allow the GESTURE_END for the last touch-point to be dispatched for convenience/clarity.
https://codereview.chromium.org/560473003/diff/20001/ash/system/date/date_vie... File ash/system/date/date_view.cc (right): https://codereview.chromium.org/560473003/diff/20001/ash/system/date/date_vie... ash/system/date/date_view.cc:227: event->type() == ui::ET_GESTURE_END) { On 2014/09/11 21:17:57, sadrul wrote: > On 2014/09/11 21:07:42, flackr wrote: > > On 2014/09/11 14:35:18, jonross wrote: > > > On 2014/09/09 21:58:09, flackr wrote: > > > > According to > > > > > http://www.chromium.org/developers/design-documents/aura/gesture-recognizer > > > > TAP_CANCEL is fired everytime a TAP_DOWN doesn't turn into a tap, so you > > > should > > > > be able to cancel the effect on either TAP_CANCEL, or TAP. > > > > > > On previous patch it was request to go from END to TAP as a condition. > > > > > > SCROLL_BEGIN and TAP_CANCEL cover our early dismissal for non-tap gestures. > > > > Don't we get TAP_CANCEL before SCROLL_BEGIN? > > I believe we do. > > > > > > > > > When do we want to stop visuals for the normal case? TAP or END? > > > > Ah I see, sadrul why do we want to wait until END to cancel? I thought ending > on > > tap and tap_cancel nicely encapsulates the intended effect of indicating when > > your touch will turn into a tap. > > ISTR there are cases where we don't trigger TAP_CANCEL (e.g. for long-press). > Terry can confirm. So instead of having to enumerate all the terminal gesture > events in code like this, we allow the GESTURE_END for the last touch-point to > be dispatched for convenience/clarity. Using TAP_CANCEL lets us exit out early for scrolling gestures, as well as other gestures which properly send this. GESTURE_END captures all other cases. https://codereview.chromium.org/560473003/diff/40001/ash/system/date/date_vie... File ash/system/date/date_view_unittest.cc (right): https://codereview.chromium.org/560473003/diff/40001/ash/system/date/date_vie... ash/system/date/date_view_unittest.cc:139: gfx::Point move_point(view_bounds.x(), view_bounds.CenterPoint().y()); Due to crbug.com/413844 and the removal of acting on SCROLL_BEGIN
https://codereview.chromium.org/560473003/diff/40001/ash/system/date/date_vie... File ash/system/date/date_view.cc (right): https://codereview.chromium.org/560473003/diff/40001/ash/system/date/date_vie... ash/system/date/date_view.cc:147: touch_feedback_enabled_ = true; Sorry for only recognizing this now but you can create a helper function to query the flag which has a static initializer to avoid repeated commandline checks. See for example bool IsDraggingTrayEnabled(): https://code.google.com/p/chromium/codesearch#chromium/src/ash/shelf/shelf_la... This can be done once in a helper file (similar to https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ui_base_sw...) and will avoid us saving a separate flag status with each instance of each object type which needs to know about it.
https://codereview.chromium.org/560473003/diff/40001/ash/system/date/date_vie... File ash/system/date/date_view.cc (right): https://codereview.chromium.org/560473003/diff/40001/ash/system/date/date_vie... ash/system/date/date_view.cc:147: touch_feedback_enabled_ = true; On 2014/09/17 18:30:06, flackr wrote: > Sorry for only recognizing this now but you can create a helper function to > query the flag which has a static initializer to avoid repeated commandline > checks. See for example bool IsDraggingTrayEnabled(): > https://code.google.com/p/chromium/codesearch#chromium/src/ash/shelf/shelf_la... > > This can be done once in a helper file (similar to > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ui_base_sw...) > and will avoid us saving a separate flag status with each instance of each > object type which needs to know about it. Done.
lgtm https://codereview.chromium.org/560473003/diff/60001/ash/system/date/date_vie... File ash/system/date/date_view.cc (right): https://codereview.chromium.org/560473003/diff/60001/ash/system/date/date_vie... ash/system/date/date_view.cc:176: date_label_->SetEnabledColor(kHeaderTextColorHover); If all we do based on this is update date_label_ we probably don't need to track active_.
jonross@chromium.org changed reviewers: + skuhne@chromium.org
skuhne@chromium.org: Please review changes in ash/system/date/ Another addition of touch feedback.
lgtm.
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/560473003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as 446cac3fa9b1c28258a959608e03af8a270b1eb4
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/426a9fde95b04bbcfb1adfbf05003243f8c80593 Cr-Commit-Position: refs/heads/master@{#298702} |
