Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2028303004:
Hide hover effect on hidden ink drop host views. (Closed)
|
Created:
4 years, 6 months ago by Evan Stade Modified:
4 years, 6 months ago CC:
chromium-reviews, tfarina, bruthig+ink_drop_chromium.org, dcheng Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHide hover effect on hidden ink drop host views.
BUG=584685
Committed: https://crrev.com/75d70ffe50126500835f0895c3e98b795cb1c6ef
Cr-Commit-Position: refs/heads/master@{#399239}
Patch Set 1 #
Total comments: 10
Patch Set 2 : also do find bar #Patch Set 3 : fix tests #
Total comments: 5
Patch Set 4 : rebase #Patch Set 5 : rebase #
Total comments: 5
Patch Set 6 : not indrag #Patch Set 7 : null widget #Patch Set 8 : . #
Total comments: 2
Patch Set 9 : here's the beef #Patch Set 10 : rebase #
Messages
Total messages: 42 (12 generated)
estade@chromium.org changed reviewers: + bruthig@chromium.org, sky@chromium.org
https://codereview.chromium.org/2028303004/diff/1/ui/views/animation/ink_drop... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2028303004/diff/1/ui/views/animation/ink_drop... ui/views/animation/ink_drop_host_view.cc:78: ink_drop_delegate()->GetInkDrop()->SetHovered(false); analogous to CustomButton::VisibilityChanged https://codereview.chromium.org/2028303004/diff/1/ui/views/controls/button/cu... File ui/views/controls/button/custom_button.cc (left): https://codereview.chromium.org/2028303004/diff/1/ui/views/controls/button/cu... ui/views/controls/button/custom_button.cc:139: ink_drop_delegate()->SetHovered(ShouldShowInkDropHover()); this change/fix is tangential
The CQ bit was checked by estade@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/2028303004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028303004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2028303004/diff/1/ui/views/animation/ink_drop... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2028303004/diff/1/ui/views/animation/ink_drop... ui/views/animation/ink_drop_host_view.cc:77: if (ink_drop_delegate() && GetWidget() && !is_visible) Why do you care if there is a widget here? https://codereview.chromium.org/2028303004/diff/1/ui/views/controls/button/cu... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2028303004/diff/1/ui/views/controls/button/cu... ui/views/controls/button/custom_button.cc:136: ink_drop_delegate()->SetHovered(InDrag() && IsMouseHovered()); Why do you check the InDrag here? I have to admit I'm confused as to why this doesn't call ShouldShowInkDropHover(). What is the intention of that method if you can't call it here?
Would it make more sense to dispatch a MouseExit() event to the Views when the visibility goes to false?
I could see that mildly confusing and think we should avoid it. On Thu, Jun 2, 2016 at 10:47 AM, <bruthig@chromium.org> wrote: > Would it make more sense to dispatch a MouseExit() event to the Views when > the > visibility goes to false? > > https://codereview.chromium.org/2028303004/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Evan, do you plan to address the same issue with the ripple persisting when an InkDropHotView is hidden in a follow-up? On 2016/06/02 18:05:57, sky wrote: > I could see that mildly confusing and think we should avoid it. Acknowledged. > > On Thu, Jun 2, 2016 at 10:47 AM, <mailto:bruthig@chromium.org> wrote: > > Would it make more sense to dispatch a MouseExit() event to the Views when > > the > > visibility goes to false? > > > > https://codereview.chromium.org/2028303004/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > https://codereview.chromium.org/2028303004/diff/1/ui/views/animation/ink_drop... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2028303004/diff/1/ui/views/animation/ink_drop... ui/views/animation/ink_drop_host_view.cc:78: ink_drop_delegate()->GetInkDrop()->SetHovered(false); On 2016/06/01 22:46:20, Evan Stade wrote: > analogous to CustomButton::VisibilityChanged Is OnBlur() and more importantly SetFocused(false) going to be called when InkDropHostViews become invisible? If not we may want to add that here as well.
> Evan, do you plan to address the same issue with the ripple persisting when an > InkDropHotView is hidden in a follow-up? do you have repro steps that demonstrate this bug? https://codereview.chromium.org/2028303004/diff/1/ui/views/animation/ink_drop... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2028303004/diff/1/ui/views/animation/ink_drop... ui/views/animation/ink_drop_host_view.cc:77: if (ink_drop_delegate() && GetWidget() && !is_visible) On 2016/06/02 03:07:26, sky wrote: > Why do you care if there is a widget here? Without this check it crashes in GetInkDropBaseColor (which tries to use GetNativeTheme, which doesn't exist if not yet added to a widget hierarchy). https://codereview.chromium.org/2028303004/diff/1/ui/views/animation/ink_drop... ui/views/animation/ink_drop_host_view.cc:78: ink_drop_delegate()->GetInkDrop()->SetHovered(false); On 2016/06/02 19:02:59, bruthig wrote: > On 2016/06/01 22:46:20, Evan Stade wrote: > > analogous to CustomButton::VisibilityChanged > > Is OnBlur() and more importantly SetFocused(false) going to be called when > InkDropHostViews become invisible? If not we may want to add that here as well. I didn't investigate with extreme thoroughness but I found that that call was not necessary to fix the given bug. I would imagine it's because of this line where focus is advanced when visibility is set to false: https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&r... https://codereview.chromium.org/2028303004/diff/1/ui/views/controls/button/cu... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2028303004/diff/1/ui/views/controls/button/cu... ui/views/controls/button/custom_button.cc:136: ink_drop_delegate()->SetHovered(InDrag() && IsMouseHovered()); On 2016/06/02 03:07:26, sky wrote: > Why do you check the InDrag here? because I'm copying ShouldShowInkDropHover (minus the part about focus). > I have to admit I'm confused as to why this > doesn't call ShouldShowInkDropHover(). What is the intention of that method if > you can't call it here? Currently we use "hover" sometimes when we really mean "highlight" (other times we use hover when we do mean hover). This is one of the former times. I have a TODO to fix the naming here and elsewhere but Ben is working on changes that collide with it, so I haven't done it yet.
On 2016/06/02 19:34:07, Evan Stade wrote: > > Evan, do you plan to address the same issue with the ripple persisting when an > > InkDropHotView is hidden in a follow-up? > > do you have repro steps that demonstrate this bug? 1. Open find-in-page bar (ctrl+f). 2. Press and hold left moust button on the find-in-page bar's close button. 3. Press Esc key to hide the bar. 4. Move mouse to middle of page. 5. Open find-in-page bar again. Expected: The ripple is not visible. Actual The ripple is visible. https://codereview.chromium.org/2028303004/diff/1/ui/views/animation/ink_drop... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2028303004/diff/1/ui/views/animation/ink_drop... ui/views/animation/ink_drop_host_view.cc:78: ink_drop_delegate()->GetInkDrop()->SetHovered(false); On 2016/06/02 19:34:07, Evan Stade wrote: > On 2016/06/02 19:02:59, bruthig wrote: > > On 2016/06/01 22:46:20, Evan Stade wrote: > > > analogous to CustomButton::VisibilityChanged > > > > Is OnBlur() and more importantly SetFocused(false) going to be called when > > InkDropHostViews become invisible? If not we may want to add that here as > well. > > I didn't investigate with extreme thoroughness but I found that that call was > not necessary to fix the given bug. I would imagine it's because of this line > where focus is advanced when visibility is set to false: > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&r... Awesome, might be worth a test by a good Samaritan at some point. https://codereview.chromium.org/2028303004/diff/1/ui/views/controls/button/cu... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2028303004/diff/1/ui/views/controls/button/cu... ui/views/controls/button/custom_button.cc:136: ink_drop_delegate()->SetHovered(InDrag() && IsMouseHovered()); On 2016/06/02 19:34:07, Evan Stade wrote: > On 2016/06/02 03:07:26, sky wrote: > > Why do you check the InDrag here? > > because I'm copying ShouldShowInkDropHover (minus the part about focus). > > > I have to admit I'm confused as to why this > > doesn't call ShouldShowInkDropHover(). What is the intention of that method if > > you can't call it here? > > Currently we use "hover" sometimes when we really mean "highlight" (other times > we use hover when we do mean hover). This is one of the former times. I have a > TODO to fix the naming here and elsewhere but Ben is working on changes that > collide with it, so I haven't done it yet. Feel free to make this name change, I will deal with the conflicts as I haven't been able to push my current changes along as quickly as I'd like to.
On 2016/06/02 19:46:37, bruthig wrote: > On 2016/06/02 19:34:07, Evan Stade wrote: > > > Evan, do you plan to address the same issue with the ripple persisting when > an > > > InkDropHotView is hidden in a follow-up? > > > > do you have repro steps that demonstrate this bug? > > 1. Open find-in-page bar (ctrl+f). > 2. Press and hold left moust button on the find-in-page bar's close button. > 3. Press Esc key to hide the bar. > 4. Move mouse to middle of page. > 5. Open find-in-page bar again. > > Expected: > The ripple is not visible. > > Actual > The ripple is visible. > thanks. fixed.
The CQ bit was checked by estade@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/2028303004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028303004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
FYI another TestInkDrop was added here: https://codereview.chromium.org/2017833003/ . It was added to the test_ink_drop_delegate.cc file because it wasn't consistent with the TestInkDropDelegate having it's own version of |state_|. Your implementation is perfect tho so just replace the other one with yours. https://codereview.chromium.org/2028303004/diff/40001/ui/views/animation/test... File ui/views/animation/test/test_ink_drop.cc (right): https://codereview.chromium.org/2028303004/diff/40001/ui/views/animation/test... ui/views/animation/test/test_ink_drop.cc:17: return true; Shouldn't this be 'return state_ != InkDropState::HIDDEN' ? https://codereview.chromium.org/2028303004/diff/40001/ui/views/animation/test... File ui/views/animation/test/test_ink_drop.h (right): https://codereview.chromium.org/2028303004/diff/40001/ui/views/animation/test... ui/views/animation/test/test_ink_drop.h:28: bool is_hovered_; Can you add 'bool is_focused_' too?
https://codereview.chromium.org/2028303004/diff/40001/ui/views/animation/test... File ui/views/animation/test/test_ink_drop.cc (right): https://codereview.chromium.org/2028303004/diff/40001/ui/views/animation/test... ui/views/animation/test/test_ink_drop.cc:17: return true; On 2016/06/03 02:59:24, bruthig wrote: > Shouldn't this be 'return state_ != InkDropState::HIDDEN' ? this function (on the InkDrop interface) is unused and I have removed it. https://codereview.chromium.org/2028303004/diff/40001/ui/views/animation/test... File ui/views/animation/test/test_ink_drop.h (right): https://codereview.chromium.org/2028303004/diff/40001/ui/views/animation/test... ui/views/animation/test/test_ink_drop.h:28: bool is_hovered_; On 2016/06/03 02:59:24, bruthig wrote: > Can you add 'bool is_focused_' too? I'd not like to do so until there's a use for it
https://codereview.chromium.org/2028303004/diff/40001/ui/views/animation/test... File ui/views/animation/test/test_ink_drop.cc (right): https://codereview.chromium.org/2028303004/diff/40001/ui/views/animation/test... ui/views/animation/test/test_ink_drop.cc:17: return true; On 2016/06/03 03:25:23, Evan Stade wrote: > On 2016/06/03 02:59:24, bruthig wrote: > > Shouldn't this be 'return state_ != InkDropState::HIDDEN' ? > > this function (on the InkDrop interface) is unused and I have removed it. Acknowledged. https://codereview.chromium.org/2028303004/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2028303004/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:136: ink_drop_delegate()->SetHovered(InDrag() && IsMouseHovered()); Shouldn't this be !InDrag() ? IMO it would be easier to maintain if you didn't duplicate the "is hovered" logic here and in ShouldShowInkDropHighlight(). i.e. bool ShouldShowInkDropHighlight() const { return IsHovered() || IsFocused(); }
https://codereview.chromium.org/2028303004/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2028303004/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:136: ink_drop_delegate()->SetHovered(InDrag() && IsMouseHovered()); On 2016/06/03 14:30:50, bruthig wrote: > Shouldn't this be !InDrag() ? fixed > > IMO it would be easier to maintain if you didn't duplicate the "is hovered" > logic here and in ShouldShowInkDropHighlight(). > i.e. > bool ShouldShowInkDropHighlight() const { > return IsHovered() || IsFocused(); > } I don't really think that makes sense. How would you write IsHovered? bool IsHovered() { return IsMouseHovered() && !InDrag(); } Question: why do we care about InDrag when ShouldEnterHoveredState doesn't seem to?
The CQ bit was checked by estade@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/2028303004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2028303004/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2028303004/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:136: ink_drop_delegate()->SetHovered(InDrag() && IsMouseHovered()); On 2016/06/03 17:55:59, Evan Stade wrote: > On 2016/06/03 14:30:50, bruthig wrote: > > Shouldn't this be !InDrag() ? > > fixed > > > > > IMO it would be easier to maintain if you didn't duplicate the "is hovered" > > logic here and in ShouldShowInkDropHighlight(). > > i.e. > > bool ShouldShowInkDropHighlight() const { > > return IsHovered() || IsFocused(); > > } > > I don't really think that makes sense. How would you write IsHovered? > > bool IsHovered() { > return IsMouseHovered() && !InDrag(); > } Yeah. > > Question: why do we care about InDrag when ShouldEnterHoveredState doesn't seem > to? ShouldEnterHoveredState() probably doesn't need to check the drag state because ButtonState::HOVERED and ButtonState::PRESSED are mutually exclusive and drag operations, AFAIK, require a button to be in a PRESSED state. i.e. a hover state never needs to be cleared/prevented while in a drag. Whereas, when an InkDrop animates to HIDDEN at the start of a drag operation, we don't want the hover to fade back in during the drag, which may happen when InkDropImpl::HoverAfterRippleTimerFired() -> ... -> InkDropHost::CreateInkDropHover().
https://codereview.chromium.org/2028303004/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2028303004/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:136: ink_drop_delegate()->SetHovered(InDrag() && IsMouseHovered()); On 2016/06/03 19:42:36, bruthig wrote: > On 2016/06/03 17:55:59, Evan Stade wrote: > > On 2016/06/03 14:30:50, bruthig wrote: > > > Shouldn't this be !InDrag() ? > > > > fixed > > > > > > > > IMO it would be easier to maintain if you didn't duplicate the "is hovered" > > > logic here and in ShouldShowInkDropHighlight(). > > > i.e. > > > bool ShouldShowInkDropHighlight() const { > > > return IsHovered() || IsFocused(); > > > } > > > > I don't really think that makes sense. How would you write IsHovered? > > > > bool IsHovered() { > > return IsMouseHovered() && !InDrag(); > > } > > Yeah. But then IsHovered is different from IsMouseHovered which is unintuitive. > > > > > Question: why do we care about InDrag when ShouldEnterHoveredState doesn't > seem > > to? > > ShouldEnterHoveredState() probably doesn't need to check the drag state because > ButtonState::HOVERED and ButtonState::PRESSED are mutually exclusive and drag > operations, AFAIK, require a button to be in a PRESSED state. i.e. a hover state > never needs to be cleared/prevented while in a drag. > Whereas, when an InkDrop animates to HIDDEN at the start of a drag operation, we > don't want the hover to fade back in during the drag, which may happen when > InkDropImpl::HoverAfterRippleTimerFired() -> ... -> > InkDropHost::CreateInkDropHover(). Actually I think we should be reusing ShouldEnterHoveredState() here (updated). If the test of whether this works is dragging a bookmark bar button or folder and seeing if the hover ever fades back in, this seems to work.
https://codereview.chromium.org/2028303004/diff/80001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2028303004/diff/80001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:136: ink_drop_delegate()->SetHovered(InDrag() && IsMouseHovered()); On 2016/06/03 21:53:23, Evan Stade wrote: > On 2016/06/03 19:42:36, bruthig wrote: > > On 2016/06/03 17:55:59, Evan Stade wrote: > > > On 2016/06/03 14:30:50, bruthig wrote: > > > > Shouldn't this be !InDrag() ? > > > > > > fixed > > > > > > > > > > > IMO it would be easier to maintain if you didn't duplicate the "is > hovered" > > > > logic here and in ShouldShowInkDropHighlight(). > > > > i.e. > > > > bool ShouldShowInkDropHighlight() const { > > > > return IsHovered() || IsFocused(); > > > > } > > > > > > I don't really think that makes sense. How would you write IsHovered? > > > > > > bool IsHovered() { > > > return IsMouseHovered() && !InDrag(); > > > } > > > > Yeah. > > But then IsHovered is different from IsMouseHovered which is unintuitive. I'd be more concerned if it were the same as it would be completely duplicated code ;) > > > > > > > > > Question: why do we care about InDrag when ShouldEnterHoveredState doesn't > > seem > > > to? > > > > ShouldEnterHoveredState() probably doesn't need to check the drag state > because > > ButtonState::HOVERED and ButtonState::PRESSED are mutually exclusive and drag > > operations, AFAIK, require a button to be in a PRESSED state. i.e. a hover > state > > never needs to be cleared/prevented while in a drag. > > Whereas, when an InkDrop animates to HIDDEN at the start of a drag operation, > we > > don't want the hover to fade back in during the drag, which may happen when > > InkDropImpl::HoverAfterRippleTimerFired() -> ... -> > > InkDropHost::CreateInkDropHover(). > > Actually I think we should be reusing ShouldEnterHoveredState() here (updated). > If the test of whether this works is dragging a bookmark bar button or folder > and seeing if the hover ever fades back in, this seems to work. Ok, so would it be accurate to say ShouldEnterHoveredState() would track the logical hovered state and ShouldShowInkDropHighlight() would track the visual hover/focus state? I could buy in to that. https://codereview.chromium.org/2028303004/diff/140001/ui/views/controls/butt... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/2028303004/diff/140001/ui/views/controls/butt... ui/views/controls/button/menu_button.cc:403: // The widget may be null during shutdown. nit: Can you beef up this comment a little. It is unclear to me why the call to OnAction() requires a Widget...
> I could buy in to that. me too https://codereview.chromium.org/2028303004/diff/140001/ui/views/controls/butt... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/2028303004/diff/140001/ui/views/controls/butt... ui/views/controls/button/menu_button.cc:403: // The widget may be null during shutdown. On 2016/06/03 22:35:25, bruthig wrote: > nit: Can you beef up this comment a little. It is unclear to me why the call to > OnAction() requires a Widget... Done. Creating an ink drop requires a native theme which we won't have during shutdown.
lgtm
LGTM
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028303004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, bruthig@chromium.org Link to the patchset: https://codereview.chromium.org/2028303004/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028303004/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Hide hover effect on hidden ink drop host views. BUG=584685 ========== to ========== Hide hover effect on hidden ink drop host views. BUG=584685 Committed: https://crrev.com/75d70ffe50126500835f0895c3e98b795cb1c6ef Cr-Commit-Position: refs/heads/master@{#399239} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/75d70ffe50126500835f0895c3e98b795cb1c6ef Cr-Commit-Position: refs/heads/master@{#399239} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
