Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2017833003:
Fixes ink drop being "stuck" on when losing focus. (Closed)
DescriptionFixes losing focus while space bar pressed.
BUG=615520
Committed: https://crrev.com/934405233ea138ad8c27528779b6f9071957b8bc
Cr-Commit-Position: refs/heads/master@{#397492}
Patch Set 1 #Patch Set 2 : Removed code intended for another bug fix. #
Total comments: 2
Patch Set 3 : Added unit test #Patch Set 4 : Removed some left-over crufty code. #
Total comments: 3
Patch Set 5 : Removal of the nits... #
Total comments: 5
Patch Set 6 : Moved TestInkDrop to test_ink_drop_delegate.cc. Added code comments. #
Total comments: 2
Patch Set 7 : Formatting changes and added NOTREACHED() macro #
Messages
Total messages: 23 (6 generated)
Description was changed from ========== Fixes losing focus while spacebar pressed and now shows/hides ink drop when mouse moved in and out of the button bounds while button is pressed BUG= ========== to ========== Fixes losing focus while space bar pressed. BUG=615520 ==========
kylixrd@chromium.org changed reviewers: + bruthig@chromium.org, sky@chromium.org
https://codereview.chromium.org/2017833003/diff/20001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2017833003/diff/20001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:374: if (IsHotTracked() || state_ == STATE_PRESSED) { This opens up the following edge case: 1. Press and hold left mouse button 2. Obtain keyboard focus (still holding left mouse down) 3. Lose keyboard focus (i.e. OnBlur() is called) 4. Release left mouse button (which presumably triggers the button action) I believe at step 4 there will be no visual indication that the button is armed and firing an action due to the mouse release may catch a user off guard. Although it appears as though this was also a problem pre-MD however so I'm not sure which is the lesser of two evils. Would it be possible to 'unarm' the button for mouse and keyboard when OnBlur() is called? And would this make sense? And if we go that far, we should also be considering gesture events too. sky@, what are your thoughts on this?
https://codereview.chromium.org/2017833003/diff/20001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2017833003/diff/20001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:374: if (IsHotTracked() || state_ == STATE_PRESSED) { On 2016/05/31 14:20:40, bruthig wrote: > This opens up the following edge case: > > 1. Press and hold left mouse button > 2. Obtain keyboard focus (still holding left mouse down) > 3. Lose keyboard focus (i.e. OnBlur() is called) > 4. Release left mouse button (which presumably triggers the button action) > > I believe at step 4 there will be no visual indication that the button is armed > and firing an action due to the mouse release may catch a user off guard. > Although it appears as though this was also a problem pre-MD however so I'm not > sure which is the lesser of two evils. > > Would it be possible to 'unarm' the button for mouse and keyboard when OnBlur() > is called? And would this make sense? And if we go that far, we should also be > considering gesture events too. > > sky@, what are your thoughts on this? At the very least I do believe this change is an improvement as it prevents the ripple from getting stuck on. But we should at the very least create an issue or a TODO to make buttons work more robustly between the three different input methods.
Please add test coverage for these cases.
On 2016/05/31 14:20:40, bruthig wrote: > https://codereview.chromium.org/2017833003/diff/20001/ui/views/controls/butto... > File ui/views/controls/button/custom_button.cc (right): > > https://codereview.chromium.org/2017833003/diff/20001/ui/views/controls/butto... > ui/views/controls/button/custom_button.cc:374: if (IsHotTracked() || state_ == > STATE_PRESSED) { > This opens up the following edge case: > > 1. Press and hold left mouse button > 2. Obtain keyboard focus (still holding left mouse down) > 3. Lose keyboard focus (i.e. OnBlur() is called) > 4. Release left mouse button (which presumably triggers the button action) > > I believe at step 4 there will be no visual indication that the button is armed > and firing an action due to the mouse release may catch a user off guard. > Although it appears as though this was also a problem pre-MD however so I'm not > sure which is the lesser of two evils. > > Would it be possible to 'unarm' the button for mouse and keyboard when OnBlur() > is called? And would this make sense? And if we go that far, we should also be > considering gesture events too. It appears that the button is too focused on single event actions, rather that acknowledging and ensuring that there is a sequence of events which lead to those actions. For instance, NOTIFY_ON_RELEASE is coded (and unit-tested) such that any mouse release even will trigger the action. From a simplistic view of things, this will work. However, this flies in the face of generally accepted UI behaviors (going all the way back to some of the early mainstream GUIs, such as Windows and eventually MacOS). A button should never fire its action event without the preceding "arming" event. In this case, it is the button pressed event. A "click" action will usually consist of both a mouse button down event, followed by 0 or more mouse move events, and finally a mouse button released event. If, and only if, the mouse cursor is within the bounds of the control when the mouse button released event is received will the action be triggered.
Added test case.
LGTM with the following, but wait for Ben https://codereview.chromium.org/2017833003/diff/60001/ui/views/animation/test... File ui/views/animation/test/test_ink_drop.h (right): https://codereview.chromium.org/2017833003/diff/60001/ui/views/animation/test... ui/views/animation/test/test_ink_drop.h:16: TestInkDrop() : InkDrop() { }; nit: no ';' and no space between { }. If you run git cl format I suspect it'll remote the space. https://codereview.chromium.org/2017833003/diff/60001/ui/views/animation/test... ui/views/animation/test/test_ink_drop.h:17: ~TestInkDrop() override { }; no ';' here either (and the rest of this file). https://codereview.chromium.org/2017833003/diff/60001/ui/views/animation/test... File ui/views/animation/test/test_ink_drop_delegate.cc (right): https://codereview.chromium.org/2017833003/diff/60001/ui/views/animation/test... ui/views/animation/test/test_ink_drop_delegate.cc:34: ink_drop_ = base::WrapUnique(new TestInkDrop()); nit: ink_drop.reset(new TestInkDrop); is shorter and more concise. Also, I'm not sure if this should be added. Wait for Ben to review this.
Nit removal. bruthig@, how is this looking?
https://codereview.chromium.org/2017833003/diff/80001/ui/views/animation/test... File ui/views/animation/test/test_ink_drop_delegate.cc (right): https://codereview.chromium.org/2017833003/diff/80001/ui/views/animation/test... ui/views/animation/test/test_ink_drop_delegate.cc:33: if (!ink_drop_.get()) Was this change necessary or are you just back filling the simple 'return nullptr'? As it stands now, I don't like how TestInkDropDelegate::GetTargetInkDropState() and TestInkDropDelegate::GetInkDrop()::GetTargetState() can return different values. This is inconsistent with the TestInkDropDelegate being somewhat of a test fake. FTR I am in the middle of removing the bulk of the (Button)InkDropDelegates, if not the entire class. So, it might make more sense to not add this, assuming it's not currently required...? https://codereview.chromium.org/2017833003/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2017833003/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:379: ink_drop_delegate()->OnAction(views::InkDropState::HIDDEN); Can you add a TODO here along the following lines: TODO(bruthig): Fix CustomButtons to work well when multiple input methods are interacting with a button. e.g. By animating to HIDDEN here it is possible for a Mouse Release to trigger an action however there would be no visual cue to the user that this will occur. https://codereview.chromium.org/2017833003/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/2017833003/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:81: void OnFocus() override { nit: a comment here would be useful. I assume it's to change the visibility from private to public?
Moved TestInkDrop class to test_ink_drop_delegate.cc & and other updates. https://codereview.chromium.org/2017833003/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2017833003/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:379: ink_drop_delegate()->OnAction(views::InkDropState::HIDDEN); On 2016/06/02 15:21:35, bruthig wrote: > Can you add a TODO here along the following lines: > > TODO(bruthig): Fix CustomButtons to work well when multiple input methods are > interacting with a button. e.g. By animating to HIDDEN here it is possible for > a Mouse Release to trigger an action however there would be no visual cue to the > user that this will occur. Done. https://codereview.chromium.org/2017833003/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button_unittest.cc (right): https://codereview.chromium.org/2017833003/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button_unittest.cc:81: void OnFocus() override { On 2016/06/02 15:21:35, bruthig wrote: > nit: a comment here would be useful. I assume it's to change the visibility > from private to public? Done.
https://codereview.chromium.org/2017833003/diff/100001/ui/views/animation/tes... File ui/views/animation/test/test_ink_drop_delegate.cc (right): https://codereview.chromium.org/2017833003/diff/100001/ui/views/animation/tes... ui/views/animation/test/test_ink_drop_delegate.cc:16: InkDropState GetTargetInkDropState() const override { Please add NOTREACHED() for all methods except SetFocused() as they do not play consistently with the TestInkDropDelegate. https://codereview.chromium.org/2017833003/diff/100001/ui/views/controls/butt... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2017833003/diff/100001/ui/views/controls/butt... ui/views/controls/button/custom_button.cc:380: //TODO(bruthig) : Fix CustomButtons to work well when multiple input methods are nit: formatting/indent. I find running 'git cl format' before uploading a patch is a good routine to get in to :)
On 2016/06/02 16:49:55, bruthig wrote: > https://codereview.chromium.org/2017833003/diff/100001/ui/views/animation/tes... > File ui/views/animation/test/test_ink_drop_delegate.cc (right): > > https://codereview.chromium.org/2017833003/diff/100001/ui/views/animation/tes... > ui/views/animation/test/test_ink_drop_delegate.cc:16: InkDropState > GetTargetInkDropState() const override { > Please add NOTREACHED() for all methods except SetFocused() as they do not play > consistently with the TestInkDropDelegate. Doh! > https://codereview.chromium.org/2017833003/diff/100001/ui/views/controls/butt... > File ui/views/controls/button/custom_button.cc (right): > > https://codereview.chromium.org/2017833003/diff/100001/ui/views/controls/butt... > ui/views/controls/button/custom_button.cc:380: //TODO(bruthig) : Fix > CustomButtons to work well when multiple input methods are > nit: formatting/indent. I find running 'git cl format' before uploading a patch > is a good routine to get in to :) Understood. However, I've resisted that while I'm trying to get a handle on the actual formatting/style preferences. I want to be able to write code in the right format/style and then be able to easily spot code which violates the given style.
Latest updates.
lgtm
The CQ bit was checked by kylixrd@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/2017833003/#ps120001 (title: "Formatting changes and added NOTREACHED() macro")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2017833003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2017833003/120001
Message was sent while issue was closed.
Description was changed from ========== Fixes losing focus while space bar pressed. BUG=615520 ========== to ========== Fixes losing focus while space bar pressed. BUG=615520 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Fixes losing focus while space bar pressed. BUG=615520 ========== to ========== Fixes losing focus while space bar pressed. BUG=615520 Committed: https://crrev.com/934405233ea138ad8c27528779b6f9071957b8bc Cr-Commit-Position: refs/heads/master@{#397492} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/934405233ea138ad8c27528779b6f9071957b8bc Cr-Commit-Position: refs/heads/master@{#397492} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
