Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1409183003:
Adds ripple animation to extension buttons on a toolbar (Closed)
DescriptionAdds ripple animation to extension buttons on a toolbar
Ripple animation behind the 'top-chrome-md' experimental flag.
BUG=537202
TEST=Press and hold extension button. Make sure a ripple animation is
triggered. Release the button, make sure that the animation is hidden.
Press and hold extension button. Make sure a ripple animation is
triggered. Drag the extension button and release. Make sure a ripple
animation is hidden.
Patch Set 1 : Adds ripple animation to extension buttons on a toolbar #
Total comments: 4
Patch Set 2 : Adds ripple animation to extension buttons on a toolbar #Patch Set 3 : Adds ripple animation to extension buttons on a toolbar (gesture support) #
Total comments: 2
Patch Set 4 : Adds ripple animation to extension buttons on a toolbar (gesture support) #
Total comments: 4
Patch Set 5 : Adds ripple animation to extension buttons on a toolbar (added a todo) #
Total comments: 2
Patch Set 6 : Adds ripple animation to extension buttons on a toolbar (gesture handling) #Patch Set 7 : Adds ripple animation to extension buttons on a toolbar (rebased) #
Total comments: 2
Patch Set 8 : Adds ripple animation to extension buttons on a toolbar (fix for bug 546683) #
Total comments: 8
Dependent Patchsets: Messages
Total messages: 35 (11 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
varkha@chromium.org changed reviewers: + bruthig@chromium.org
bruthig@, can you please take a look?
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1409183003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1409183003/40001
Are you planning to wire it in for touch gestures as well? https://codereview.chromium.org/1409183003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1409183003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:145: if (event.IsLeftMouseButton() || event.IsMiddleMouseButton()) { Does Middle mouse button actually trigger any actions? It doesn't appear to for me. (I think my original comment in ToolbarButton::OnMousePressed() is not accurate. But at least middle mouse clicks trigger actions on the navigation buttons.) https://codereview.chromium.org/1409183003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:153: ink_drop_animation_controller_->AnimateToState(views::InkDropState::HIDDEN); You might need to guard this with "if (!HitTestPoint(event.location()))". We shouldn't hide the ripple on a mouse release that also caused a button click.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:80001) has been deleted
> Are you planning to wire it in for touch gestures as well? Yes, please see ToolbarActionView::OnGestureEvent. https://codereview.chromium.org/1409183003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1409183003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:145: if (event.IsLeftMouseButton() || event.IsMiddleMouseButton()) { On 2015/10/19 19:29:15, bruthig wrote: > Does Middle mouse button actually trigger any actions? It doesn't appear to for > me. (I think my original comment in ToolbarButton::OnMousePressed() is not > accurate. But at least middle mouse clicks trigger actions on the navigation > buttons.) It seems MenuButtons only trigger actions on left clicks. Done. https://codereview.chromium.org/1409183003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:153: ink_drop_animation_controller_->AnimateToState(views::InkDropState::HIDDEN); On 2015/10/19 19:29:15, bruthig wrote: > You might need to guard this with "if (!HitTestPoint(event.location()))". We > shouldn't hide the ripple on a mouse release that also caused a button click. I've wired this with a help of MenuButtonListener::OnMenuButtonReleasedWithoutClick which seemed a more reliable way of detecting no-clicks.
> Are you planning to wire it in for touch gestures as well? Yes, please see ToolbarActionView::OnGestureEvent.
lgtm
varkha@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@, can you please take a look? Thanks!
I missed something on the first pass. https://codereview.chromium.org/1409183003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1409183003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:173: case ui::ET_GESTURE_TAP: It might not be necessary to animate to the QUICK_ACTION in this case. It should be handled by the OnMenuButtonClicked() method.
https://codereview.chromium.org/1409183003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1409183003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:173: case ui::ET_GESTURE_TAP: On 2015/10/20 17:42:30, bruthig wrote: > It might not be necessary to animate to the QUICK_ACTION in this case. It > should be handled by the OnMenuButtonClicked() method. Done.
https://codereview.chromium.org/1409183003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1409183003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:185: I am not a fan of how all this code looks so very close to the code in toolbar_button.cc/find_bar_view.cc Can we refactor? https://codereview.chromium.org/1409183003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:223: ink_drop_animation_controller_->AnimateToState(views::InkDropState::HIDDEN); Can this be done from OnMouseReleased instead?
https://codereview.chromium.org/1409183003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1409183003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:185: On 2015/10/21 21:35:20, sadrul wrote: > I am not a fan of how all this code looks so very close to the code in > toolbar_button.cc/find_bar_view.cc Can we refactor? We were discussing with bruthig@ how to refactor this to make adding ripple animations less onerous for the implementer. My preference would be to land this bahavioural change and refactor all the similar implementations in a followup CL. We were thinking about bringing ripple-related code into CustomButton while adding template methods that descendants would override to enable and tune the ripple animations. https://codereview.chromium.org/1409183003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:223: ink_drop_animation_controller_->AnimateToState(views::InkDropState::HIDDEN); On 2015/10/21 21:35:20, sadrul wrote: > Can this be done from OnMouseReleased instead? This is done with the new callback because it would be wrong to call it when a click is actually triggered (in that case the animation needs to go to QUICK_ACTION state and then fade away). Setting animation to HIDDEN would hide it prematurely. Since I didn't want to repeat all the logic in the base class here I have made a callback that gets called in all cases when OnMenuButtonClicked would not be called and only the MenuButton has all that logic. FindBarButton and ToolbarButton are not doing an entirely right thing either and it is possible to get into a state where the ripple shadow is stuck. See http://crbug.com/544251 which has a draft CL for the ToolbarButton (will probably need to be updated to patch FindBarButton as well.
https://codereview.chromium.org/1409183003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1409183003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:179: case ui::ET_GESTURE_TAP_CANCEL: You should probably add the same changes here that are found here: https://codereview.chromium.org/1422593003/diff/20001/chrome/browser/ui/views... And we should consider this strong evidence to consolidate this 'duplicated' logic in one spot.
https://codereview.chromium.org/1409183003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1409183003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:179: case ui::ET_GESTURE_TAP_CANCEL: On 2015/10/22 15:25:43, bruthig wrote: > You should probably add the same changes here that are found here: > https://codereview.chromium.org/1422593003/diff/20001/chrome/browser/ui/views... > > And we should consider this strong evidence to consolidate this 'duplicated' > logic in one spot. Done. Yes, there's enough evidence. Let's land the behaviour and refactor common code.
lgtm
On 2015/10/21 22:44:08, varkha wrote: > https://codereview.chromium.org/1409183003/diff/120001/chrome/browser/ui/view... > File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): > > https://codereview.chromium.org/1409183003/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/toolbar/toolbar_action_view.cc:185: > On 2015/10/21 21:35:20, sadrul wrote: > > I am not a fan of how all this code looks so very close to the code in > > toolbar_button.cc/find_bar_view.cc Can we refactor? > > We were discussing with bruthig@ how to refactor this to make adding ripple > animations less onerous for the implementer. My preference would be to land this > bahavioural change and refactor all the similar implementations in a followup > CL. > We were thinking about bringing ripple-related code into CustomButton while > adding template methods that descendants would override to enable and tune the > ripple animations. Would it be possible to come up with a plan first? I would like that to happen before we ship a behaviour that may be difficult to maintain after the refactor. We should also keep in mind that we might want this effect on non-button views (e.g. a views::Link, maybe?). So we should have a mechanism that can be used with any View. (e.g. the mechanism could implement the EventHandler interface, and set itself as the target-handler for the View to process the incoming events, etc.) > > https://codereview.chromium.org/1409183003/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/toolbar/toolbar_action_view.cc:223: > ink_drop_animation_controller_->AnimateToState(views::InkDropState::HIDDEN); > On 2015/10/21 21:35:20, sadrul wrote: > > Can this be done from OnMouseReleased instead? > > This is done with the new callback because it would be wrong to call it when a > click is actually triggered (in that case the animation needs to go to > QUICK_ACTION state and then fade away). Setting animation to HIDDEN would hide > it prematurely. Since I didn't want to repeat all the logic in the base class > here I have made a callback that gets called in all cases when > OnMenuButtonClicked would not be called and only the MenuButton has all that > logic. > > FindBarButton and ToolbarButton are not doing an entirely right thing either and > it is possible to get into a state where the ripple shadow is stuck. See > http://crbug.com/544251 which has a draft CL for the ToolbarButton (will > probably need to be updated to patch FindBarButton as well.
Hi sadrul@, do you want to proceed with this and then focus on refactoring as we discussed offline? Some of the ideas are captured in https://docs.google.com/document/d/16zqXV1jlRw3bXpHpWVgoPMhgizJK9SgtvhuCC0ZaM.... My preference would be to get this out of the way and then refactor first to consolidate existing code in one place and then to change that into a state-driven model.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1409183003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1409183003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sadrul@, The refactoring is tracked at https://codereview.chromium.org/1411833006, although that is not for review yet. My preference would be to land this CL and have Ben's CL (https://codereview.chromium.org/1422593003/) land before landing any of the refactoring. Then we could consolidate code in step 1 and extract state-driven logic in step 2. Does this sound like a plan?
tdanderson@chromium.org changed reviewers: + tdanderson@chromium.org
https://chromiumcodereview.appspot.com/1409183003/diff/180001/chrome/browser/... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://chromiumcodereview.appspot.com/1409183003/diff/180001/chrome/browser/... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:118: void ToolbarActionView::AddInkDropLayer(ui::Layer* ink_drop_layer) { Note that you should change this implementation to match ToolbarButton::AddInkDropLayer() in https://codereview.chromium.org/1460733002 .
https://chromiumcodereview.appspot.com/1409183003/diff/180001/chrome/browser/... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://chromiumcodereview.appspot.com/1409183003/diff/180001/chrome/browser/... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:118: void ToolbarActionView::AddInkDropLayer(ui::Layer* ink_drop_layer) { On 2015/11/18 16:08:21, tdanderson wrote: > Note that you should change this implementation to match > ToolbarButton::AddInkDropLayer() in https://codereview.chromium.org/1460733002 . > Done. Thanks for watching this!
On 2015/11/16 20:50:37, varkha wrote: > sadrul@, The refactoring is tracked at > https://codereview.chromium.org/1411833006, although that is not for review yet. > My preference would be to land this CL and have Ben's CL > (https://codereview.chromium.org/1422593003/) land before landing any of the > refactoring. Then we could consolidate code in step 1 and extract state-driven > logic in step 2. Does this sound like a plan? Not super excited about that. These two CLs are a lot of new duplicated code, and I would still prefer it if the refactor could happen first. Do you expect the refactor to take a long time to complete? https://codereview.chromium.org/1409183003/diff/200001/ui/views/controls/butt... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/1409183003/diff/200001/ui/views/controls/butt... ui/views/controls/button/menu_button.cc:150: listener_->OnMenuButtonClickCanceled(this); no {} for this https://codereview.chromium.org/1409183003/diff/200001/ui/views/controls/butt... ui/views/controls/button/menu_button.cc:198: WillNotActivate(); Can this directly call into listener_->OnMenuButtonClickCanceled()? https://codereview.chromium.org/1409183003/diff/200001/ui/views/controls/butt... File ui/views/controls/button/menu_button.h (right): https://codereview.chromium.org/1409183003/diff/200001/ui/views/controls/butt... ui/views/controls/button/menu_button.h:68: void WillNotActivate(); This is .. confusing. The way it is done now, it only means that the button will not be activated in the next mouse-release. But a subsequent click can still activate the button, right? https://codereview.chromium.org/1409183003/diff/200001/ui/views/controls/butt... File ui/views/controls/button/menu_button_listener.h (right): https://codereview.chromium.org/1409183003/diff/200001/ui/views/controls/butt... ui/views/controls/button/menu_button_listener.h:23: virtual void OnMenuButtonClickCanceled(View* source) {} = 0;
sadrul@, can you please see if what I am trying to do in https://codereview.chromium.org/1411833006/ makes sense. If so, I can try to land that - with or without extension button ripples (which will be just a few lines of code in the new implementation). If you prefer I can land that refactoring first with no behavioral changes and then add a few lines to implement ripples on the extension buttons in a separate CL. Let me know. https://codereview.chromium.org/1409183003/diff/200001/ui/views/controls/butt... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/1409183003/diff/200001/ui/views/controls/butt... ui/views/controls/button/menu_button.cc:150: listener_->OnMenuButtonClickCanceled(this); On 2015/11/18 22:56:31, sadrul wrote: > no {} for this This is inlined in OnMouseReleased (see https://codereview.chromium.org/1411833006/). https://codereview.chromium.org/1409183003/diff/200001/ui/views/controls/butt... ui/views/controls/button/menu_button.cc:198: WillNotActivate(); On 2015/11/18 22:56:31, sadrul wrote: > Can this directly call into listener_->OnMenuButtonClickCanceled()? Done in https://codereview.chromium.org/1411833006/. https://codereview.chromium.org/1409183003/diff/200001/ui/views/controls/butt... File ui/views/controls/button/menu_button.h (right): https://codereview.chromium.org/1409183003/diff/200001/ui/views/controls/butt... ui/views/controls/button/menu_button.h:68: void WillNotActivate(); On 2015/11/18 22:56:31, sadrul wrote: > This is .. confusing. The way it is done now, it only means that the button will > not be activated in the next mouse-release. But a subsequent click can still > activate the button, right? This is inlined in OnMouseReleased (see https://codereview.chromium.org/1411833006/). https://codereview.chromium.org/1409183003/diff/200001/ui/views/controls/butt... File ui/views/controls/button/menu_button_listener.h (right): https://codereview.chromium.org/1409183003/diff/200001/ui/views/controls/butt... ui/views/controls/button/menu_button_listener.h:23: virtual void OnMenuButtonClickCanceled(View* source) {} On 2015/11/18 22:56:31, sadrul wrote: > = 0; Done (no longer necessary - see https://codereview.chromium.org/1411833006/).
Description was changed from
==========
Adds ripple animation to extension buttons on a toolbar
Ripple animation behind the 'top-chrome-md' experimental flag.
BUG=537202
TEST=Press and hold extension button. Make sure a ripple animation is
triggered. Release the button, make sure that the animation is hidden.
Press and hold extension button. Make sure a ripple animation is
triggered. Drag the extension button and release. Make sure a ripple
animation is hidden.
==========
to
==========
Adds ripple animation to extension buttons on a toolbar
Ripple animation behind the 'top-chrome-md' experimental flag.
BUG=537202
TEST=Press and hold extension button. Make sure a ripple animation is
triggered. Release the button, make sure that the animation is hidden.
Press and hold extension button. Make sure a ripple animation is
triggered. Drag the extension button and release. Make sure a ripple
animation is hidden.
==========
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
