Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1411523009:
Adds OnClickCanceled callback to views::Button (Closed)
DescriptionAdds OnClickCanceled callback to views::Button
Wires an additional callback to ensure that descendants of a CustomButton
get notified if a mouse is released without triggering an action. Uses
that new callback to hide ripple animation used in toolbar buttons.
BUG=544251
Committed: https://crrev.com/9db9996e921e1b90170daa84d2efe74926b17fc2
Cr-Commit-Position: refs/heads/master@{#362568}
Patch Set 1 #Patch Set 2 : Adds NotifyReleasedWithoutClick callback to views::Button (corrections) #
Total comments: 2
Patch Set 3 : Adds NotifyReleasedWithoutClick callback to views::Button (unit test) #
Total comments: 2
Patch Set 4 : Adds NotifyReleasedWithoutClick callback to views::Button (comment) #
Total comments: 4
Patch Set 5 : Adds NotifyReleasedWithoutClick callback to views::Button (nit) #Patch Set 6 : Adds NotifyReleasedWithoutClick callback to views::Button (rebased) #Patch Set 7 : Adds NotifyReleasedWithoutClick callback to views::Button (one more case handled) #
Total comments: 4
Patch Set 8 : Adds NotifyReleasedWithoutClick callback to views::Button (comments) #
Messages
Total messages: 45 (19 generated)
varkha@chromium.org changed reviewers: + bruthig@chromium.org
bruthig@, please take a look. Do you think this would be a good way to solve the toolbar button release - similar to how https://codereview.chromium.org/1409183003/ implements it for the MenuButton? Thanks!
Patchset #2 (id:20001) has been deleted
PTAL (corrected based on what we discussed this offline). The changes are to handle the case of NOTIFY_ON_PRESS and to use IsTriggerableEvent() instead of explicit check for left / middle button. I am still leaving alone the handling gesture ends in the base class as it needs work in ToolbarButton. We should probably refactor this later so that more of implementation lives in CustomButton base class and is triggered by ButtonState transitions. The descendants can then impact behavior through template methods.
One minor point about documentation but design looks good. Is it possible to add some tests for this new behavior? It should be pretty straight forward to test the new behavior in CustomButton. https://codereview.chromium.org/1411523009/diff/40001/ui/views/controls/butto... File ui/views/controls/button/button.h (right): https://codereview.chromium.org/1411523009/diff/40001/ui/views/controls/butto... ui/views/controls/button/button.h:72: // Called when a button gets released without triggering an action. I think you should be explicit here about this only being called for mouse releases and not gestures. Either in the documentation of the method name. Consider: // Called when a button gets released without triggering an action. // Note: This is only wired up for button releases caused by mouse // button releases AND not gesture events. virtual void NotifyMouseReleasedWithoutClick(const ui::Event& event);
varkha@chromium.org changed reviewers: + sadrul@chromium.org
> Is it possible to add some tests for this new behavior? > It should be pretty straightforward to test the new > behavior in CustomButton. Done. PTAL. +sadrul@ for design oversight. https://codereview.chromium.org/1411523009/diff/40001/ui/views/controls/butto... File ui/views/controls/button/button.h (right): https://codereview.chromium.org/1411523009/diff/40001/ui/views/controls/butto... ui/views/controls/button/button.h:72: // Called when a button gets released without triggering an action. On 2015/10/22 15:18:50, bruthig wrote: > I think you should be explicit here about this only being called for mouse > releases and not gestures. Either in the documentation of the method name. > > Consider: > > // Called when a button gets released without triggering an action. > // Note: This is only wired up for button releases caused by mouse > // button releases AND not gesture events. > virtual void NotifyMouseReleasedWithoutClick(const ui::Event& event); Done.
sadrul@, ping?
Having a callback when a click is canceled sounds good to me. https://codereview.chromium.org/1411523009/diff/60001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1411523009/diff/60001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:143: NotifyMouseReleasedWithoutClick(event); This looks odd. Why should we do this on mouse-press? Maybe call this OnClickCanceled() instead?
https://codereview.chromium.org/1411523009/diff/60001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1411523009/diff/60001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:143: NotifyMouseReleasedWithoutClick(event); On 2015/11/04 19:13:10, sadrul wrote: > This looks odd. Why should we do this on mouse-press? > > Maybe call this OnClickCanceled() instead? I thought we would want this notification to be sent here for symmetry meaning that we are not expecting the click anymore and can unravel whatever pending state we might have. Totally agree with the name, thanks for suggesting it.
Description was changed from ========== Adds NotifyReleasedWithoutClick callback to views::Button Wires an additional callback to ensure that decendants of a CustomButton get notified if a mouse is released without triggering an action. Uses that new callback to hide ripple animation used in toolbar buttons. BUG=544251 ========== to ========== Adds OnClickCanceled callback to views::Button Wires an additional callback to ensure that descendants of a CustomButton get notified if a mouse is released without triggering an action. Uses that new callback to hide ripple animation used in toolbar buttons. BUG=544251 ==========
https://codereview.chromium.org/1411523009/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1411523009/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:113: views::InkDropState::ACTION_PENDING); I am beginning to question if event-handling code is the best place to actually trigger/stop the animations. Would it make sense to trigger the animations specifically when the ButtonState changes? For example, CustomButton::SetState() can call a virtual OnStateChanged(old_state, new_state [, event?]) method, and ToolbarButton here will override that to make the decision of when to start/stop the ripple. If that works, I feel like that would simplify this code.
https://codereview.chromium.org/1411523009/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1411523009/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:113: views::InkDropState::ACTION_PENDING); On 2015/11/06 21:46:59, sadrul wrote: > I am beginning to question if event-handling code is the best place to actually > trigger/stop the animations. Would it make sense to trigger the animations > specifically when the ButtonState changes? For example, CustomButton::SetState() > can call a virtual OnStateChanged(old_state, new_state [, event?]) method, and > ToolbarButton here will override that to make the decision of when to start/stop > the ripple. If that works, I feel like that would simplify this code. This may work as a plan (and would be applicable to other ripple targets) but seems tangential to this CL which is just a bug fix in the existing handling.
lgtm https://codereview.chromium.org/1411523009/diff/80001/ui/views/controls/butto... File ui/views/controls/button/button.h (right): https://codereview.chromium.org/1411523009/diff/80001/ui/views/controls/butto... ui/views/controls/button/button.h:75: // events. *and
https://codereview.chromium.org/1411523009/diff/80001/ui/views/controls/butto... File ui/views/controls/button/button.h (right): https://codereview.chromium.org/1411523009/diff/80001/ui/views/controls/butto... ui/views/controls/button/button.h:75: // events. On 2015/11/25 22:18:42, sadrul wrote: > *and Done.
varkha@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@, can you please take an OWNERS look for chrome/browser/ui/views/? Thanks!
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/1411523009/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411523009/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
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/1411523009/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411523009/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #7 (id:140001) has been deleted
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/1411523009/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411523009/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1411523009/diff/160001/ui/views/controls/butt... File ui/views/controls/button/button.h (right): https://codereview.chromium.org/1411523009/diff/160001/ui/views/controls/butt... ui/views/controls/button/button.h:73: // a NOTIFY_ON_PRESS button is pressed with a wrong mouse button. It's not intuitive why we should fire "canceled" for NOTIFY_ON_PRESS with the wrong mouse button. Either don't do that, fire a separate notification than "canceled", or explain here why this actually makes sense. https://codereview.chromium.org/1411523009/diff/160001/ui/views/controls/butt... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/1411523009/diff/160001/ui/views/controls/butt... ui/views/controls/button/custom_button_unittest.cc:63: bool notified_no_click_ = false; Why not call these |pressed_| and |canceled_|? Those seem like clearer and more direct names.
Patchset #8 (id:180001) has been deleted
pkasting@, PTAL. I have also updated test expectations. https://codereview.chromium.org/1411523009/diff/160001/ui/views/controls/butt... File ui/views/controls/button/button.h (right): https://codereview.chromium.org/1411523009/diff/160001/ui/views/controls/butt... ui/views/controls/button/button.h:73: // a NOTIFY_ON_PRESS button is pressed with a wrong mouse button. On 2015/12/01 02:03:18, Peter Kasting wrote: > It's not intuitive why we should fire "canceled" for NOTIFY_ON_PRESS with the > wrong mouse button. Either don't do that, fire a separate notification than > "canceled", or explain here why this actually makes sense. Done. Thinking a bit more we should not really need that. https://codereview.chromium.org/1411523009/diff/160001/ui/views/controls/butt... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/1411523009/diff/160001/ui/views/controls/butt... ui/views/controls/button/custom_button_unittest.cc:63: bool notified_no_click_ = false; On 2015/12/01 02:03:18, Peter Kasting wrote: > Why not call these |pressed_| and |canceled_|? Those seem like clearer and more > direct names. Done.
LGTM
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/1411523009/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411523009/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1411523009/#ps200001 (title: "Adds NotifyReleasedWithoutClick callback to views::Button (comments)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411523009/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411523009/200001
Message was sent while issue was closed.
Description was changed from ========== Adds OnClickCanceled callback to views::Button Wires an additional callback to ensure that descendants of a CustomButton get notified if a mouse is released without triggering an action. Uses that new callback to hide ripple animation used in toolbar buttons. BUG=544251 ========== to ========== Adds OnClickCanceled callback to views::Button Wires an additional callback to ensure that descendants of a CustomButton get notified if a mouse is released without triggering an action. Uses that new callback to hide ripple animation used in toolbar buttons. BUG=544251 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Adds OnClickCanceled callback to views::Button Wires an additional callback to ensure that descendants of a CustomButton get notified if a mouse is released without triggering an action. Uses that new callback to hide ripple animation used in toolbar buttons. BUG=544251 ========== to ========== Adds OnClickCanceled callback to views::Button Wires an additional callback to ensure that descendants of a CustomButton get notified if a mouse is released without triggering an action. Uses that new callback to hide ripple animation used in toolbar buttons. BUG=544251 Committed: https://crrev.com/9db9996e921e1b90170daa84d2efe74926b17fc2 Cr-Commit-Position: refs/heads/master@{#362568} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9db9996e921e1b90170daa84d2efe74926b17fc2 Cr-Commit-Position: refs/heads/master@{#362568} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
