Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1517003002:
Fixes stale hover or ink ripple visuals when dragging over extension buttons (Closed)
DescriptionFixes stale hover / ink ripple when dragging over extension buttons
BUG=568436
TEST=
Install at least 2 extensions
Move their icons to overflow Chrome menu
Open the chrome menu,
Position the mouse cursor to the left of the leftmost extension button
Hold down the left mouse button
Move the cursor over the extension buttons
Only a single extension icon should have ink ripple
BUG=564941
TEST=
Drag an extension button.
ink drop should disappear when drag starts.
Committed: https://crrev.com/a431b315b4805f8d87df83e79ca8e59a0123b030
Cr-Commit-Position: refs/heads/master@{#368105}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixes stale hover or ink ripple visuals when dragging over extension buttons (for all buttons) #Patch Set 3 : Fixes stale hover or ink ripple visuals when dragging over extension buttons (adds test) #
Total comments: 2
Patch Set 4 : Fixes stale hover or ink ripple visuals when dragging over extension buttons (moved the test to cus… #
Total comments: 7
Patch Set 5 : Fixes stale hover or ink ripple visuals when dragging over extension buttons (nits) #
Messages
Total messages: 34 (12 generated)
varkha@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
rdevlin.cronin@, can you please take a look? Thanks! This solution should work because capture lost is sent to the previously active view when dragging in a menu. See: https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
https://codereview.chromium.org/1517003002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1517003002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:235: ink_drop_delegate()->OnAction(views::InkDropState::HIDDEN); Why are these in ToolbarActionView, instead of CustomButton? Won't every button behave the same?
Description was changed from ========== Fixes stale hover / ink ripple when dragging over extension buttons BUG=568436 TEST= Install at least 2 extensions Move their icons to overflow Chrome menu Open the chrome menu, Position the mouse cursor to the left of the leftmost extension button Hold down the left mouse button Move the cursor over the extension buttons Only a single extension icon should have hover (without MD) or ink ripple (with MD) ========== to ========== Fixes stale hover / ink ripple when dragging over extension buttons BUG=568436 TEST= Install at least 2 extensions Move their icons to overflow Chrome menu Open the chrome menu, Position the mouse cursor to the left of the leftmost extension button Hold down the left mouse button Move the cursor over the extension buttons Only a single extension icon should have ink ripple BUG=564941 TEST= Drag an extension button. ink drop should disappear when drag starts. ==========
Yes, I think we can make this apply to all buttons, I just didn't know if we have an example that would apply to any other button class, especially since the ink_drop_delegate so far was only used for extension buttons (this will change with CL https://codereview.chromium.org/1478303003/). It also has an added benefit of fixing bug 564941 without requiring CL https://codereview.chromium.org/1518463002/. Thanks for the tip! https://codereview.chromium.org/1517003002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1517003002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:235: ink_drop_delegate()->OnAction(views::InkDropState::HIDDEN); On 2015/12/10 22:18:22, Devlin wrote: > Why are these in ToolbarActionView, instead of CustomButton? Won't every button > behave the same? Done.
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/1517003002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1517003002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rdevlin.cronin@chromium.org changed reviewers: + sky@chromium.org
I'm not an OWNER in ui/views, so passing it over to Sky. That said, I would like to see a test for this. :)
I agree, please add test coverage. Are there other views that need to be updated like this as well?
varkha@chromium.org changed reviewers: + bruthig@chromium.org
bruthig@, can you please take a first look at a unit test for basic ripple show / hide? Thanks!
As discussed offline these tests probably should live in custom_button_unittest.cc. https://codereview.chromium.org/1517003002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_delegate_unittest.cc (right): https://codereview.chromium.org/1517003002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate_unittest.cc:107: button_ = new TestButton(&ink_shown_, &ink_hidden_); It might be worth having a DCHECK(!button_) after 106, just incase the check on line 114 disappears.
> these tests probably should live in custom_button_unittest.cc. Done, PTAL. https://codereview.chromium.org/1517003002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_delegate_unittest.cc (right): https://codereview.chromium.org/1517003002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate_unittest.cc:107: button_ = new TestButton(&ink_shown_, &ink_hidden_); On 2016/01/06 22:46:00, bruthig wrote: > It might be worth having a DCHECK(!button_) after 106, just incase the check on > line 114 disappears. Acknowledged (the code has been reworked so that CreateButtonWithInkDrop could be called more than once).
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/1517003002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1517003002/60001
https://codereview.chromium.org/1517003002/diff/60001/ui/views/controls/butto... File ui/views/controls/button/custom_button_unittest.cc (left): https://codereview.chromium.org/1517003002/diff/60001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:109: widget_->SetBounds(widget_bounds); This code made sense in the original first submission but I don't think it is necessary when used as it is now in SetUp. It causes the widget to be recreated in a wrong spot if a mouse cursor is moved during the test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/12/11 18:38:51, sky wrote: > I agree, please add test coverage. Are there other views that need to be updated > like this as well? You never answered my question. Are there other views that directly use ink drop and need to override OnMouseCaptureLost?
On 2016/01/07 16:10:50, sky wrote: > On 2015/12/11 18:38:51, sky wrote: > > I agree, please add test coverage. Are there other views that need to be > updated > > like this as well? > > You never answered my question. Are there other views that directly use ink drop > and need to override OnMouseCaptureLost? Sorry, no at the moment the highest class that is still aware of ink drop is the CustomButton.
Ok, LGTM
Some minor considerations, otherwise lgtm https://codereview.chromium.org/1517003002/diff/60001/ui/views/controls/butto... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/1517003002/diff/60001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:90: // An InkDropDelegate that keeps track of order of deletions. Is this comment still accurate? https://codereview.chromium.org/1517003002/diff/60001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:119: case InkDropState::DEACTIVATED: The DEACTIVATED, SLOW_ACTION and QUICK_ACTION states will auto transition to HIDDEN but this logic is captured in the InkDropAnimationImpl. It seems odd that you group the HIDDEN and DEACTIVATED here. I would think it should only be the HIDDEN state. Likewise above I don't think the QUICK_ACTION or SLOW_ACTION should set |*ink_shown_| to true. At the very least I think these 3 states should be handled in the same way. https://codereview.chromium.org/1517003002/diff/60001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:183: delete button_; Consider resetting |ink_shown_| and |ink_hidden_| here to false so values aren't carried over if CreateButtonWithInkDrop() is called multiple times.
https://codereview.chromium.org/1517003002/diff/60001/ui/views/controls/butto... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/1517003002/diff/60001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:90: // An InkDropDelegate that keeps track of order of deletions. On 2016/01/07 16:55:09, bruthig wrote: > Is this comment still accurate? Done. https://codereview.chromium.org/1517003002/diff/60001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:119: case InkDropState::DEACTIVATED: On 2016/01/07 16:55:09, bruthig wrote: > The DEACTIVATED, SLOW_ACTION and QUICK_ACTION states will auto transition to > HIDDEN but this logic is captured in the InkDropAnimationImpl. It seems odd > that you group the HIDDEN and DEACTIVATED here. I would think it should only be > the HIDDEN state. Likewise above I don't think the QUICK_ACTION or SLOW_ACTION > should set |*ink_shown_| to true. > At the very least I think these 3 states should be handled in the same way. Done. https://codereview.chromium.org/1517003002/diff/60001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:183: delete button_; On 2016/01/07 16:55:09, bruthig wrote: > Consider resetting |ink_shown_| and |ink_hidden_| here to false so values aren't > carried over if CreateButtonWithInkDrop() is called multiple times. Done.
Awesome, thx. Still LGTM
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1517003002/#ps80001 (title: "Fixes stale hover or ink ripple visuals when dragging over extension buttons (nits)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1517003002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1517003002/80001
Message was sent while issue was closed.
Description was changed from ========== Fixes stale hover / ink ripple when dragging over extension buttons BUG=568436 TEST= Install at least 2 extensions Move their icons to overflow Chrome menu Open the chrome menu, Position the mouse cursor to the left of the leftmost extension button Hold down the left mouse button Move the cursor over the extension buttons Only a single extension icon should have ink ripple BUG=564941 TEST= Drag an extension button. ink drop should disappear when drag starts. ========== to ========== Fixes stale hover / ink ripple when dragging over extension buttons BUG=568436 TEST= Install at least 2 extensions Move their icons to overflow Chrome menu Open the chrome menu, Position the mouse cursor to the left of the leftmost extension button Hold down the left mouse button Move the cursor over the extension buttons Only a single extension icon should have ink ripple BUG=564941 TEST= Drag an extension button. ink drop should disappear when drag starts. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fixes stale hover / ink ripple when dragging over extension buttons BUG=568436 TEST= Install at least 2 extensions Move their icons to overflow Chrome menu Open the chrome menu, Position the mouse cursor to the left of the leftmost extension button Hold down the left mouse button Move the cursor over the extension buttons Only a single extension icon should have ink ripple BUG=564941 TEST= Drag an extension button. ink drop should disappear when drag starts. ========== to ========== Fixes stale hover / ink ripple when dragging over extension buttons BUG=568436 TEST= Install at least 2 extensions Move their icons to overflow Chrome menu Open the chrome menu, Position the mouse cursor to the left of the leftmost extension button Hold down the left mouse button Move the cursor over the extension buttons Only a single extension icon should have ink ripple BUG=564941 TEST= Drag an extension button. ink drop should disappear when drag starts. Committed: https://crrev.com/a431b315b4805f8d87df83e79ca8e59a0123b030 Cr-Commit-Position: refs/heads/master@{#368105} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a431b315b4805f8d87df83e79ca8e59a0123b030 Cr-Commit-Position: refs/heads/master@{#368105} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
