Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2720183002:
[Views] Update ink drop for omnibox icons (Closed)
Description[Views] Update hover/active states for omnibox icons
- Update the ink drop ripple behavior on the omnibox icons so that they use the
flood behavior instead
- Implement hover/active states for the page actions icon and popup blocker
Previously, ink drops were not enabled for IconLabelBubbleView when the the
label was visible since the ink drop would make the text blurry. To fix this, an
InkDropContainerView is now added to contain the ink drop. The separator
needs to fade out when the ink drop fades in, so a new view containing the
separator is now created.
IconLabelBubbleView, ContentSettingImageView and LocationIconView are also
refactored so that IconLabelBubbleView handles all of the logic for the ink drop
and suppressing mouse release events when the bubble is already opened.
BUG=588377
Review-Url: https://codereview.chromium.org/2720183002
Cr-Commit-Position: refs/heads/master@{#473647}
Committed: https://chromium.googlesource.com/chromium/src/+/bf7e0532e34476062226eab6ef1affe78abc8a8d
Patch Set 1 #Patch Set 2 : Fixed the issue I think #Patch Set 3 : Update the behavior #
Total comments: 20
Patch Set 4 : Fix for bruthig #Patch Set 5 : Moved SeparatorView out #Patch Set 6 : Refactored #
Total comments: 21
Patch Set 7 : CustomButton and fixes for bruthig #Patch Set 8 : Rebased and fixed tests #
Total comments: 23
Patch Set 9 : Fixes for bruthig and added a test #Patch Set 10 : Nits #
Total comments: 11
Patch Set 11 : Fixed issues. Also rebased #
Total comments: 1
Patch Set 12 : Gesture fix #Patch Set 13 : Fixed gesture #Patch Set 14 : Removed the mask #
Total comments: 21
Patch Set 15 : Fixes for bruthig #Patch Set 16 : Fixed tests and added comments #
Total comments: 14
Patch Set 17 : Addressed bruthig's comments #Patch Set 18 : Fix for bruthig #
Total comments: 16
Patch Set 19 : Fixes for sky #Patch Set 20 : Addressed sky's comments #
Total comments: 20
Patch Set 21 : Fixed a test, added a test #Patch Set 22 : Rebased Patch 20 #Patch Set 23 : Rebased Patch 21 #Patch Set 24 : Fixed the test #Patch Set 25 : Removed CanProcessEventsWithinSubtree #Messages
Total messages: 308 (261 generated)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:120001) has been deleted
Patchset #8 (id:220001) has been deleted
Patchset #3 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Description was changed from ========== [Views] Hover state for page icon BUG= ========== to ========== [Views] Update hover/active states for omnibox icons - Update the ink drop ripple behavior on the omnibox icons so that they flood instead - Implement hover/active states for the page actions icon BUG=588377 ==========
Description was changed from ========== [Views] Update hover/active states for omnibox icons - Update the ink drop ripple behavior on the omnibox icons so that they flood instead - Implement hover/active states for the page actions icon BUG=588377 ========== to ========== [Views] Update hover/active states for omnibox icons - Update the ink drop ripple behavior on the omnibox icons so that they flood instead - Implement hover/active states for the page actions icon and popup blocker BUG=588377 ==========
Description was changed from ========== [Views] Update hover/active states for omnibox icons - Update the ink drop ripple behavior on the omnibox icons so that they flood instead - Implement hover/active states for the page actions icon and popup blocker BUG=588377 ========== to ========== [Views] Update hover/active states for omnibox icons - Update the ink drop ripple behavior on the omnibox icons so that they use the flood behavior instead - Implement hover/active states for the page actions icon and popup blocker BUG=588377 ==========
Description was changed from ========== [Views] Update hover/active states for omnibox icons - Update the ink drop ripple behavior on the omnibox icons so that they use the flood behavior instead - Implement hover/active states for the page actions icon and popup blocker BUG=588377 ========== to ========== [Views] Update hover/active states for omnibox icons - Update the ink drop ripple behavior on the omnibox icons so that they use the flood behavior instead - Implement hover/active states for the page actions icon and popup blocker Previously, ink drops were not enabled for IconLabelBubbleView when the the label was visible since the ink drop would make the text blurry. To fix this, an InkDropContainerView is now added to contain the ink drop. The separator needs to fade out when the ink drop fades in, so a new view containing the separator is now created. BUG=588377 ==========
Description was changed from ========== [Views] Update hover/active states for omnibox icons - Update the ink drop ripple behavior on the omnibox icons so that they use the flood behavior instead - Implement hover/active states for the page actions icon and popup blocker Previously, ink drops were not enabled for IconLabelBubbleView when the the label was visible since the ink drop would make the text blurry. To fix this, an InkDropContainerView is now added to contain the ink drop. The separator needs to fade out when the ink drop fades in, so a new view containing the separator is now created. BUG=588377 ========== to ========== [Views] Update hover/active states for omnibox icons - Update the ink drop ripple behavior on the omnibox icons so that they use the flood behavior instead - Implement hover/active states for the page actions icon and popup blocker Previously, ink drops were not enabled for IconLabelBubbleView when the the label was visible since the ink drop would make the text blurry. To fix this, an InkDropContainerView is now added to contain the ink drop. The separator needs to fade out when the ink drop fades in, so a new view containing the separator is now created. BUG=588377 ==========
Description was changed from ========== [Views] Update hover/active states for omnibox icons - Update the ink drop ripple behavior on the omnibox icons so that they use the flood behavior instead - Implement hover/active states for the page actions icon and popup blocker Previously, ink drops were not enabled for IconLabelBubbleView when the the label was visible since the ink drop would make the text blurry. To fix this, an InkDropContainerView is now added to contain the ink drop. The separator needs to fade out when the ink drop fades in, so a new view containing the separator is now created. BUG=588377 ========== to ========== [Views] Update hover/active states for omnibox icons - Update the ink drop ripple behavior on the omnibox icons so that they use the flood behavior instead - Implement hover/active states for the page actions icon and popup blocker Previously, ink drops were not enabled for IconLabelBubbleView when the the label was visible since the ink drop would make the text blurry. To fix this, an InkDropContainerView is now added to contain the ink drop. The separator needs to fade out when the ink drop fades in, so a new view containing the separator is now created. BUG=588377 ==========
spqchan@chromium.org changed reviewers: + bruthig@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hello bruthig, can you review this? I'm implementing the new ink drop behaviour on the omnibox icons. Let me know if you want me to explain anything. Thanks!
Hey, I've taken a look from an ink drop usage perspective and have left some higher level comments. Most of them hinge on the discussion about what the SeparatorView really cares about so digest that one first. Overall comments: - Can any IconLabelBubbleView instances change size while an ink drop ripple or highlight is active? If so we may need to do some extra work to make sure we don't get the ink drop bleeding out over neighbouring views. You can check out InkDropHostView::CreateInkDropMask() for more info but if this is the route we need to go, let's discuss offline. - Assuming the activated state of the ink drop should have rounded corners, is this effect currently implemented? I would have expected to see some use of an InkDropMask to do this. Cheers, Ben https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:98: if (!ink_drop->ShouldHighlight() && Should this really care about the logical hover state of the ink drop or should it care about whether the ripple or the highlight is visible? These are not the same thing, see AutoHighlightMode::HIDE_ON_RIPPLE behavior. The tl;dr is that the highlight is hidden for a period of time after a ripple animates to hidden. Put another way, if that mode changed to HIDE_ON_RIPPLE would you get the desired/reasonable behavior for the separator visibility? https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:136: ink_drop_container_->SetPaintToLayer(); Why does the |ink_drop_container_| have to paint to it's own layer when the ink drop is not active? https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:256: SchedulePaint(); Why is this SchedulePaint() necessary? https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:261: SchedulePaint(); Why is this SchedulePaint() necessary? https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:266: ink_drop->SetShowHighlightOnFocus(true); |ink_drop| doesn't appear to be used? https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:18: class SeparatorView; I will let OWNER's confirm/correct me on this but I don't think you should pollute the global namespace like this. I think a better alternative would be to declare it inside of IconLabelBubbleView. https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:76: virtual bool IsInkDropEnabled() const; Would InkDropHostView::SetInkDropMode() be a sufficient replacement for this? https://codereview.chromium.org/2720183002/diff/240001/ui/views/animation/ink... File ui/views/animation/ink_drop.h (right): https://codereview.chromium.org/2720183002/diff/240001/ui/views/animation/ink... ui/views/animation/ink_drop.h:51: virtual bool ShouldHighlight() const = 0; If we keep this function we should come up with a better name. Something like IsHighlighted() with clear documentation that this reports the logical highlight state and not necessarily the visible highlight state. https://codereview.chromium.org/2720183002/diff/240001/ui/views/animation/ink... File ui/views/animation/ink_drop_host.h (right): https://codereview.chromium.org/2720183002/diff/240001/ui/views/animation/ink... ui/views/animation/ink_drop_host.h:43: virtual void InkDropAnimationStarted() {} To enable compos-ability this should be a function on a new InkDropObserver class that is attached to InkDrop instances. The Host should only be responsible for 'hosting' the ink drop layers. (It's currently overloaded with the creation of them too...but that's in the backlog) https://codereview.chromium.org/2720183002/diff/240001/ui/views/animation/ink... File ui/views/animation/ink_drop_impl.h (right): https://codereview.chromium.org/2720183002/diff/240001/ui/views/animation/ink... ui/views/animation/ink_drop_impl.h:75: bool ShouldHighlight() const override; If we decide to keep ShouldHighlight() as a virtual function in InkDrop then it should be marked as final here. Child classes should not be able to control the visibility of the highlight by overriding this function. The InkDropImpl has some dependencies on when the return value of ShouldHighlight() changes. For example see InkDropImpl::SetShowHighlightOnHover(). Let me rephrase this, I think there is more effort required to make the InkDropImpl work well if this function as overridable and I don't think it is worth it.
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Description was changed from ========== [Views] Update hover/active states for omnibox icons - Update the ink drop ripple behavior on the omnibox icons so that they use the flood behavior instead - Implement hover/active states for the page actions icon and popup blocker Previously, ink drops were not enabled for IconLabelBubbleView when the the label was visible since the ink drop would make the text blurry. To fix this, an InkDropContainerView is now added to contain the ink drop. The separator needs to fade out when the ink drop fades in, so a new view containing the separator is now created. BUG=588377 ========== to ========== [Views] Update hover/active states for omnibox icons - Update the ink drop ripple behavior on the omnibox icons so that they use the flood behavior instead - Implement hover/active states for the page actions icon and popup blocker Previously, ink drops were not enabled for IconLabelBubbleView when the the label was visible since the ink drop would make the text blurry. To fix this, an InkDropContainerView is now added to contain the ink drop. The separator needs to fade out when the ink drop fades in, so a new view containing the separator is now created. IconLabelBubbleView, ContentSettingImageView and LocationIconView are also refactored so that IconLabelBubbleView handles all of the logic for the ink drop and suppressing mouse release events when the bubble is already opened. BUG=588377 ==========
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #6 (id:300001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #6 (id:320001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:340001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #6 (id:360001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for looking into this. I addressed your comments and refactored some stuff. https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:98: if (!ink_drop->ShouldHighlight() && On 2017/03/08 18:58:24, bruthig wrote: > Should this really care about the logical hover state of the ink drop or should > it care about whether the ripple or the highlight is visible? > These are not the same thing, see AutoHighlightMode::HIDE_ON_RIPPLE behavior. > The tl;dr is that the highlight is hidden for a period of time after a ripple > animates to hidden. > > Put another way, if that mode changed to HIDE_ON_RIPPLE would you get the > desired/reasonable behavior for the separator visibility? > The separator needs to be invisible when the ripple or highlight is visible. If the mode changed to HIDE_ON_RIPPLE, I would get the desired behaviour. Is there a better a way to detect if either the ripple or highlight is visible? I searched around in the codebase but couldn't find one. https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:136: ink_drop_container_->SetPaintToLayer(); On 2017/03/08 18:58:24, bruthig wrote: > Why does the |ink_drop_container_| have to paint to it's own layer when the ink > drop is not active? That's a good point. I just realized that the container will SetPaintToLayer() when you call AddInkDropLayer(). Removed it https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:256: SchedulePaint(); On 2017/03/08 18:58:24, bruthig wrote: > Why is this SchedulePaint() necessary? Whoops. Thanks for catching it, this should be removed. This was code from my experiments. https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:261: SchedulePaint(); On 2017/03/08 18:58:24, bruthig wrote: > Why is this SchedulePaint() necessary? Whoops. Thanks for catching it, this should be removed. This was code from my experiments. https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:266: ink_drop->SetShowHighlightOnFocus(true); On 2017/03/08 18:58:24, bruthig wrote: > |ink_drop| doesn't appear to be used? Whoops, thanks for the catch! https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:18: class SeparatorView; On 2017/03/08 18:58:24, bruthig wrote: > I will let OWNER's confirm/correct me on this but I don't think you should > pollute the global namespace like this. I think a better alternative would be > to declare it inside of IconLabelBubbleView. Done. https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:76: virtual bool IsInkDropEnabled() const; On 2017/03/08 18:58:24, bruthig wrote: > Would InkDropHostView::SetInkDropMode() be a sufficient replacement for this? Not quite, IsInkDropEnabled() is used in LocationIconView. For most cases, the icon can have an ink drop. However, if the button is disabled from clicking, then the ink drop shouldn't appear. I renamed the method so hopefully it's more clear now https://codereview.chromium.org/2720183002/diff/240001/ui/views/animation/ink... File ui/views/animation/ink_drop.h (right): https://codereview.chromium.org/2720183002/diff/240001/ui/views/animation/ink... ui/views/animation/ink_drop.h:51: virtual bool ShouldHighlight() const = 0; On 2017/03/08 18:58:24, bruthig wrote: > If we keep this function we should come up with a better name. Something like > IsHighlighted() with clear documentation that this reports the logical highlight > state and not necessarily the visible highlight state. I just realized that there's a IsHighlightFadingInOrVisible() method I could use instead. https://codereview.chromium.org/2720183002/diff/240001/ui/views/animation/ink... File ui/views/animation/ink_drop_host.h (right): https://codereview.chromium.org/2720183002/diff/240001/ui/views/animation/ink... ui/views/animation/ink_drop_host.h:43: virtual void InkDropAnimationStarted() {} On 2017/03/08 18:58:24, bruthig wrote: > To enable compos-ability this should be a function on a new InkDropObserver > class that is attached to InkDrop instances. The Host should only be > responsible for 'hosting' the ink drop layers. (It's currently overloaded with > the creation of them too...but that's in the backlog) Done, create a InkDropObserver class https://codereview.chromium.org/2720183002/diff/240001/ui/views/animation/ink... File ui/views/animation/ink_drop_impl.h (right): https://codereview.chromium.org/2720183002/diff/240001/ui/views/animation/ink... ui/views/animation/ink_drop_impl.h:75: bool ShouldHighlight() const override; On 2017/03/08 18:58:24, bruthig wrote: > If we decide to keep ShouldHighlight() as a virtual function in InkDrop then it > should be marked as final here. Child classes should not be able to control the > visibility of the highlight by overriding this function. The InkDropImpl has > some dependencies on when the return value of ShouldHighlight() changes. For > example see InkDropImpl::SetShowHighlightOnHover(). > > Let me rephrase this, I think there is more effort required to make the > InkDropImpl work well if this function as overridable and I don't think it is > worth it. Used IsHighlightFadingInOrVisible() instead. Let me know if there are any concerns!
+sky@ Sorry it took me until the second pass to think of this but, I think we should consider making the BubbleIconView and IconLabelBubbleView own a Button to handle the input events and ink drop animations. Wiring up the ink drop animations to input events was tricky business for the Buttons and I imagine it will be the same here. If we don't have to duplicate this work then I don't think we should. Open questions: - Can the CustomButton class be configured to have the same interaction model that is desired by these two classes? (e.g. trigger action on mouse release) sky@, WDYT? https://codereview.chromium.org/2720183002/diff/380001/ash/shelf/shelf_view_u... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2720183002/diff/380001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:1942: bool IsHighlightFadingInOrVisible() const override { return false; } This should return |ink_drop_|->IsHighlightFadingInOrVisible(). https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:42: SetPaintToLayer(); Can this use a LAYER_SOLID_COLOR LayerType instead of the default LAYER_TEXTURED type? https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:78: // separator should Incomplete sentence? https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:121: ink_drop_container_->SetVisible(false); Is SetVisible(true) ever called on the |ink_drop_container_|, I don't see it anywhere? Assuming it's not, it surprises me that the highlight and ripple are ever visible. https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:226: void IconLabelBubbleView::OnMouseReleased(const ui::MouseEvent& event) { Does the InkDropHostView::InkDropGestureHandler work correctly for touch interactions with these views? https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:233: AnimateInkDrop(views::InkDropState::ACTIVATED, &event); Should it really animate to ACTIVATED if OnActivate() returns false? https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:236: AnimateInkDrop(views::InkDropState::DEACTIVATED, &event); I think you want the HIDDEN state here, this will cause a PENDING ripple to shrink, which is used on other buttons to let the user know the 'click' was canceled. https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:269: if (!CanClick()) I believe the button hierarchy prevents a highlight from being shown by calling View::SetEnabled(false). Would that work here as well? I'm not sure if Tooltips are involved or whether they are a factor to consider. A few other questions: - Alternatively, could you use InkDropImpl::SetShowHighlightOnHover(false) when the view isn't clickable? - Will CanClick() ever dynamically update it's return value while an instance has the mouse over it? Presumably the View should then be showing the highlight but with this implementation I don't think it will. - This begs the question of how the View is supposed to behave when it gets keyboard focus. Should the highlight be shown? https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:273: gfx::Size ink_drop_size = size(); When is it valid for size() to be empty? https://codereview.chromium.org/2720183002/diff/380001/ui/views/animation/ink... File ui/views/animation/ink_drop_observer.h (right): https://codereview.chromium.org/2720183002/diff/380001/ui/views/animation/ink... ui/views/animation/ink_drop_observer.h:21: virtual void AnimationStarted() = 0; nit: AnimationStarted() is very generic and there is a risk of a naming conflict by inheriting classes. InkDropAnimationStarted() would probably be a specific enough name to avoid this.
On 2017/03/14 03:49:47, bruthig wrote: > +sky@ > > Sorry it took me until the second pass to think of this but, I think we should > consider making the BubbleIconView and IconLabelBubbleView own a Button to > handle the input events and ink drop animations. Wiring up the ink drop > animations to input events was tricky business for the Buttons and I imagine it > will be the same here. If we don't have to duplicate this work then I don't > think we should. > > Open questions: > - Can the CustomButton class be configured to have the same interaction model > that is desired by these two classes? (e.g. trigger action on mouse release) > > sky@, WDYT? > > https://codereview.chromium.org/2720183002/diff/380001/ash/shelf/shelf_view_u... > File ash/shelf/shelf_view_unittest.cc (right): > > https://codereview.chromium.org/2720183002/diff/380001/ash/shelf/shelf_view_u... > ash/shelf/shelf_view_unittest.cc:1942: bool IsHighlightFadingInOrVisible() const > override { return false; } > This should return |ink_drop_|->IsHighlightFadingInOrVisible(). > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:42: > SetPaintToLayer(); > Can this use a LAYER_SOLID_COLOR LayerType instead of the default LAYER_TEXTURED > type? > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:78: // separator > should > Incomplete sentence? > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:121: > ink_drop_container_->SetVisible(false); > Is SetVisible(true) ever called on the |ink_drop_container_|, I don't see it > anywhere? Assuming it's not, it surprises me that the highlight and ripple are > ever visible. > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:226: void > IconLabelBubbleView::OnMouseReleased(const ui::MouseEvent& event) { > Does the InkDropHostView::InkDropGestureHandler work correctly for touch > interactions with these views? > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:233: > AnimateInkDrop(views::InkDropState::ACTIVATED, &event); > Should it really animate to ACTIVATED if OnActivate() returns false? > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:236: > AnimateInkDrop(views::InkDropState::DEACTIVATED, &event); > I think you want the HIDDEN state here, this will cause a PENDING ripple to > shrink, which is used on other buttons to let the user know the 'click' was > canceled. > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:269: if > (!CanClick()) > I believe the button hierarchy prevents a highlight from being shown by calling > View::SetEnabled(false). Would that work here as well? I'm not sure if > Tooltips are involved or whether they are a factor to consider. > > A few other questions: > - Alternatively, could you use InkDropImpl::SetShowHighlightOnHover(false) when > the view isn't clickable? > - Will CanClick() ever dynamically update it's return value while an instance > has the mouse over it? Presumably the View should then be showing the highlight > but with this implementation I don't think it will. > - This begs the question of how the View is supposed to behave when it gets > keyboard focus. Should the highlight be shown? > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:273: gfx::Size > ink_drop_size = size(); > When is it valid for size() to be empty? > > https://codereview.chromium.org/2720183002/diff/380001/ui/views/animation/ink... > File ui/views/animation/ink_drop_observer.h (right): > > https://codereview.chromium.org/2720183002/diff/380001/ui/views/animation/ink... > ui/views/animation/ink_drop_observer.h:21: virtual void AnimationStarted() = 0; > nit: AnimationStarted() is very generic and there is a risk of a naming conflict > by inheriting classes. InkDropAnimationStarted() would probably be a specific > enough name to avoid this. Whoops, I didn't mean to publish all of my drafts, just the main point about using Buttons...
I think it would be good to make these classes have a Button as well, but I would have to stare at the code to see what that entails. -Scott On Mon, Mar 13, 2017 at 8:49 PM, <bruthig@chromium.org> wrote: > +sky@ > > Sorry it took me until the second pass to think of this but, I think we > should > consider making the BubbleIconView and IconLabelBubbleView own a Button to > handle the input events and ink drop animations. Wiring up the ink drop > animations to input events was tricky business for the Buttons and I imagine > it > will be the same here. If we don't have to duplicate this work then I don't > think we should. > > Open questions: > - Can the CustomButton class be configured to have the same interaction > model > that is desired by these two classes? (e.g. trigger action on mouse release) > > sky@, WDYT? > > > https://codereview.chromium.org/2720183002/diff/380001/ash/shelf/shelf_view_u... > File ash/shelf/shelf_view_unittest.cc (right): > > https://codereview.chromium.org/2720183002/diff/380001/ash/shelf/shelf_view_u... > ash/shelf/shelf_view_unittest.cc:1942: bool > IsHighlightFadingInOrVisible() const override { return false; } > This should return |ink_drop_|->IsHighlightFadingInOrVisible(). > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc > (right): > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:42: > SetPaintToLayer(); > Can this use a LAYER_SOLID_COLOR LayerType instead of the default > LAYER_TEXTURED type? > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:78: // > separator should > Incomplete sentence? > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:121: > ink_drop_container_->SetVisible(false); > Is SetVisible(true) ever called on the |ink_drop_container_|, I don't > see it anywhere? Assuming it's not, it surprises me that the highlight > and ripple are ever visible. > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:226: void > IconLabelBubbleView::OnMouseReleased(const ui::MouseEvent& event) { > Does the InkDropHostView::InkDropGestureHandler work correctly for touch > interactions with these views? > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:233: > AnimateInkDrop(views::InkDropState::ACTIVATED, &event); > Should it really animate to ACTIVATED if OnActivate() returns false? > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:236: > AnimateInkDrop(views::InkDropState::DEACTIVATED, &event); > I think you want the HIDDEN state here, this will cause a PENDING ripple > to shrink, which is used on other buttons to let the user know the > 'click' was canceled. > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:269: if > (!CanClick()) > I believe the button hierarchy prevents a highlight from being shown by > calling View::SetEnabled(false). Would that work here as well? I'm not > sure if Tooltips are involved or whether they are a factor to consider. > > A few other questions: > - Alternatively, could you use > InkDropImpl::SetShowHighlightOnHover(false) when the view isn't > clickable? > - Will CanClick() ever dynamically update it's return value while an > instance has the mouse over it? Presumably the View should then be > showing the highlight but with this implementation I don't think it > will. > - This begs the question of how the View is supposed to behave when it > gets keyboard focus. Should the highlight be shown? > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:273: > gfx::Size ink_drop_size = size(); > When is it valid for size() to be empty? > > https://codereview.chromium.org/2720183002/diff/380001/ui/views/animation/ink... > File ui/views/animation/ink_drop_observer.h (right): > > https://codereview.chromium.org/2720183002/diff/380001/ui/views/animation/ink... > ui/views/animation/ink_drop_observer.h:21: virtual void > AnimationStarted() = 0; > nit: AnimationStarted() is very generic and there is a risk of a naming > conflict by inheriting classes. InkDropAnimationStarted() would > probably be a specific enough name to avoid this. > > https://codereview.chromium.org/2720183002/ -- 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.
On 2017/03/14 16:53:21, sky wrote: > I think it would be good to make these classes have a Button as well, > but I would have to stare at the code to see what that entails. > > -Scott > > On Mon, Mar 13, 2017 at 8:49 PM, <mailto:bruthig@chromium.org> wrote: > > +sky@ > > > > Sorry it took me until the second pass to think of this but, I think we > > should > > consider making the BubbleIconView and IconLabelBubbleView own a Button to > > handle the input events and ink drop animations. Wiring up the ink drop > > animations to input events was tricky business for the Buttons and I imagine > > it > > will be the same here. If we don't have to duplicate this work then I don't > > think we should. > > > > Open questions: > > - Can the CustomButton class be configured to have the same interaction > > model > > that is desired by these two classes? (e.g. trigger action on mouse release) > > > > sky@, WDYT? > > > > > > > https://codereview.chromium.org/2720183002/diff/380001/ash/shelf/shelf_view_u... > > File ash/shelf/shelf_view_unittest.cc (right): > > > > > https://codereview.chromium.org/2720183002/diff/380001/ash/shelf/shelf_view_u... > > ash/shelf/shelf_view_unittest.cc:1942: bool > > IsHighlightFadingInOrVisible() const override { return false; } > > This should return |ink_drop_|->IsHighlightFadingInOrVisible(). > > > > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > > File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc > > (right): > > > > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:42: > > SetPaintToLayer(); > > Can this use a LAYER_SOLID_COLOR LayerType instead of the default > > LAYER_TEXTURED type? > > > > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:78: // > > separator should > > Incomplete sentence? > > > > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:121: > > ink_drop_container_->SetVisible(false); > > Is SetVisible(true) ever called on the |ink_drop_container_|, I don't > > see it anywhere? Assuming it's not, it surprises me that the highlight > > and ripple are ever visible. > > > > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:226: void > > IconLabelBubbleView::OnMouseReleased(const ui::MouseEvent& event) { > > Does the InkDropHostView::InkDropGestureHandler work correctly for touch > > interactions with these views? > > > > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:233: > > AnimateInkDrop(views::InkDropState::ACTIVATED, &event); > > Should it really animate to ACTIVATED if OnActivate() returns false? > > > > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:236: > > AnimateInkDrop(views::InkDropState::DEACTIVATED, &event); > > I think you want the HIDDEN state here, this will cause a PENDING ripple > > to shrink, which is used on other buttons to let the user know the > > 'click' was canceled. > > > > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:269: if > > (!CanClick()) > > I believe the button hierarchy prevents a highlight from being shown by > > calling View::SetEnabled(false). Would that work here as well? I'm not > > sure if Tooltips are involved or whether they are a factor to consider. > > > > A few other questions: > > - Alternatively, could you use > > InkDropImpl::SetShowHighlightOnHover(false) when the view isn't > > clickable? > > - Will CanClick() ever dynamically update it's return value while an > > instance has the mouse over it? Presumably the View should then be > > showing the highlight but with this implementation I don't think it > > will. > > - This begs the question of how the View is supposed to behave when it > > gets keyboard focus. Should the highlight be shown? > > > > > https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... > > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:273: > > gfx::Size ink_drop_size = size(); > > When is it valid for size() to be empty? > > > > > https://codereview.chromium.org/2720183002/diff/380001/ui/views/animation/ink... > > File ui/views/animation/ink_drop_observer.h (right): > > > > > https://codereview.chromium.org/2720183002/diff/380001/ui/views/animation/ink... > > ui/views/animation/ink_drop_observer.h:21: virtual void > > AnimationStarted() = 0; > > nit: AnimationStarted() is very generic and there is a risk of a naming > > conflict by inheriting classes. InkDropAnimationStarted() would > > probably be a specific enough name to avoid this. > > > > https://codereview.chromium.org/2720183002/ > > -- > 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. > I'll give that a try and will let you know how it goes
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #8 (id:420001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #9 (id:460001) has been deleted
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #9 (id:480001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I switched to CustomButton and added the InkDropMask bruthig mentioned earlier (I accidentally missed his comments on it) One issue I still need to work out: It's possible to hover over the pop-up blocker decoration while it's animating in. When that happens, the ink drop size would be incorrect: https://screenshot.googleplex.com/Oea1HoCseRx I already tried calling the HostSizeChanged() for the current InkDrop when the bounds changed and setting the InkDrop size to the maximum and relying on InkDropMask to clip it. Do you have any other suggestions? Thanks! https://codereview.chromium.org/2720183002/diff/380001/ash/shelf/shelf_view_u... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2720183002/diff/380001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:1942: bool IsHighlightFadingInOrVisible() const override { return false; } On 2017/03/14 03:49:37, bruthig wrote: > This should return |ink_drop_|->IsHighlightFadingInOrVisible(). Done. https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:42: SetPaintToLayer(); On 2017/03/14 03:49:39, bruthig wrote: > Can this use a LAYER_SOLID_COLOR LayerType instead of the default LAYER_TEXTURED > type? I can do that, but I'll have to set the width of the separator to exactly one pixel. Will that have a different effect from calling Draw1pxLine? https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:78: // separator should On 2017/03/14 03:49:41, bruthig wrote: > Incomplete sentence? Done. https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:121: ink_drop_container_->SetVisible(false); On 2017/03/14 03:49:40, bruthig wrote: > Is SetVisible(true) ever called on the |ink_drop_container_|, I don't see it > anywhere? Assuming it's not, it surprises me that the highlight and ripple are > ever visible. Yes: https://cs.chromium.org/chromium/src/ui/views/animation/ink_drop.cc?type=cs&q... https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:226: void IconLabelBubbleView::OnMouseReleased(const ui::MouseEvent& event) { On 2017/03/14 03:49:43, bruthig wrote: > Does the InkDropHostView::InkDropGestureHandler work correctly for touch > interactions with these views? That's a good point. I made changes so that CustomButton now handles the gesture events. https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:233: AnimateInkDrop(views::InkDropState::ACTIVATED, &event); On 2017/03/14 03:49:42, bruthig wrote: > Should it really animate to ACTIVATED if OnActivate() returns false? This is now dealt by CustomButton https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:236: AnimateInkDrop(views::InkDropState::DEACTIVATED, &event); On 2017/03/14 03:49:38, bruthig wrote: > I think you want the HIDDEN state here, this will cause a PENDING ripple to > shrink, which is used on other buttons to let the user know the 'click' was > canceled. Done. https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:269: if (!CanClick()) On 2017/03/14 03:49:43, bruthig wrote: > I believe the button hierarchy prevents a highlight from being shown by calling > View::SetEnabled(false). Would that work here as well? I'm not sure if > Tooltips are involved or whether they are a factor to consider. > > A few other questions: > - Alternatively, could you use InkDropImpl::SetShowHighlightOnHover(false) when > the view isn't clickable? > - Will CanClick() ever dynamically update it's return value while an instance > has the mouse over it? Presumably the View should then be showing the highlight > but with this implementation I don't think it will. > - This begs the question of how the View is supposed to behave when it gets > keyboard focus. Should the highlight be shown? Sounds good. Question: - I can try that although your first solution seems good enough - No, that shouldn't happen - It should be if it's focused https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:273: gfx::Size ink_drop_size = size(); On 2017/03/14 03:49:38, bruthig wrote: > When is it valid for size() to be empty? It happens during tests. Now that you mentioned it, it's makes more sense to change the test instead. https://codereview.chromium.org/2720183002/diff/380001/ui/views/animation/ink... File ui/views/animation/ink_drop_observer.h (right): https://codereview.chromium.org/2720183002/diff/380001/ui/views/animation/ink... ui/views/animation/ink_drop_observer.h:21: virtual void AnimationStarted() = 0; On 2017/03/14 03:49:44, bruthig wrote: > nit: AnimationStarted() is very generic and there is a risk of a naming conflict > by inheriting classes. InkDropAnimationStarted() would probably be a specific > enough name to avoid this. Done.
Thanks for introducing the CustomButton =D On 2017/03/21 18:25:23, spqchan wrote: > I switched to CustomButton and added the InkDropMask bruthig mentioned earlier > (I accidentally missed his comments on it) > > One issue I still need to work out: It's possible to hover over the pop-up > blocker decoration while it's animating in. > > When that happens, the ink drop size would be incorrect: > https://screenshot.googleplex.com/Oea1HoCseRx > > I already tried calling the HostSizeChanged() for the current InkDrop when the > bounds changed and setting the InkDrop size to the maximum and relying on > InkDropMask to clip it. Do you have any other suggestions? Thanks! It seems you have stumbled upon crbug.com/669253. Artificially increasing the InkDropHighlights size and clipping it via the InkDropMask is the quick fix. If you choose to do the quick fix can you add a TODO(bruthig) referencing 669253. If you decide to fix 669253 ;) then please do it in a separate CL. As for testing, you could check out the InkDropSpy and how it is used to verify InkDropStates. If it's easy enough to bring up a test fixture for these buttons then these tests would be valuable. See https://cs.chromium.org/chromium/src/ash/shelf/shelf_view_unittest.cc?l=1945&... https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:42: SetPaintToLayer(); On 2017/03/21 18:25:22, spqchan wrote: > On 2017/03/14 03:49:39, bruthig wrote: > > Can this use a LAYER_SOLID_COLOR LayerType instead of the default > LAYER_TEXTURED > > type? > > I can do that, but I'll have to set the width of the separator to exactly one > pixel. Will that have a different effect from calling Draw1pxLine? I spoke with sadrul@ offline and he wasn't sure how to force a 1px wide line using Layers. So ignore this request. https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right): https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.h:27: class BubbleIconView : public views::InkDropHostView, If you're not going to use a CustomButton for this can you add a TODO? Feel free to use 'TODO(bruthig)'. https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:82: state == views::InkDropState::DEACTIVATED)) { You may want to include ACTION_TRIGGERED. It auto-transitions to HIDDEN as similar to DEACTIVATED. https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:266: if (ShouldShowLabel()) { Would it be easier to position the |ink_drop_container_| based on ShouldShowLabel() and then the ripple and highlight could be positioned relative to the container? i.e. the ripple/highlight could just be centered in the container. The ripple and highlight really should be positioned relative to the |ink_drop_container_|'s coordinates anyway. They just currently happen to be the same as |this|'s coordinates. https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:174: bool suppress_button_action_; Instead of manually tracking this, would it be possible to configure the CustomButton to NOTIFY_ON_PRESS instead? https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... File ui/views/animation/ink_drop.h (right): https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... ui/views/animation/ink_drop.h:16: #include "ui/views/animation/ink_drop_observer.h" Can this be forward declared instead? https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... ui/views/animation/ink_drop.h:55: void SetObserver(InkDropObserver* observer); I'd prefer if this used the Add/RemoveObserver() collection paradigm instead. https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... File ui/views/animation/ink_drop_observer.h (right): https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... ui/views/animation/ink_drop_observer.h:11: #include "ui/views/animation/ink_drop_animation_ended_reason.h" nit: Is this needed? https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... ui/views/animation/ink_drop_observer.h:12: #include "ui/views/animation/ink_drop_state.h" nit: Is this needed? https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... ui/views/animation/ink_drop_observer.h:17: // Observer to attach to an InkDropRipple. 'InkDropRipple' -> just 'InkDrop' https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... ui/views/animation/ink_drop_observer.h:20: // Called when the animation of the current InkDrop has started. Can you be explicit here and mention that this includes a ripple or a highlight animation.
Patchset #8 (id:440001) has been deleted
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #10 (id:540001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
>It seems you have stumbled upon crbug.com/669253. Artificially increasing the InkDropHighlights size and clipping it via the InkDropMask is the quick fix. If you choose to do the quick fix can you add a TODO(bruthig) referencing 669253. If you decide to fix 669253 ;) then please do it in a separate CL. Thanks! I used the quick fix. I wouldn't mind looking into the issue, but I'm not sure if I'll have the time to do it anytime soon :( >As for testing, you could check out the InkDropSpy and how it is used to verify InkDropStates. If it's easy enough to bring up a test fixture for these buttons then these tests would be valuable. See Awesome, I looked into InkDropSpy and stumbled into the InkDropTest API. I added a test in icon_label_bubble_view_unittest based on the tests written for MenuButton. Let me know if there any issues! https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right): https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.h:27: class BubbleIconView : public views::InkDropHostView, On 2017/03/22 17:20:16, bruthig wrote: > If you're not going to use a CustomButton for this can you add a TODO? Feel > free to use 'TODO(bruthig)'. I can do it :) I might be making more changes to it later https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:82: state == views::InkDropState::DEACTIVATED)) { On 2017/03/22 17:20:16, bruthig wrote: > You may want to include ACTION_TRIGGERED. It auto-transitions to HIDDEN as > similar to DEACTIVATED. Done. https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:266: if (ShouldShowLabel()) { On 2017/03/22 17:20:16, bruthig wrote: > Would it be easier to position the |ink_drop_container_| based on > ShouldShowLabel() and then the ripple and highlight could be positioned relative > to the container? i.e. the ripple/highlight could just be centered in the > container. > > The ripple and highlight really should be positioned relative to the > |ink_drop_container_|'s coordinates anyway. They just currently happen to be > the same as |this|'s coordinates. Done. https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:174: bool suppress_button_action_; On 2017/03/22 17:20:16, bruthig wrote: > Instead of manually tracking this, would it be possible to configure the > CustomButton to NOTIFY_ON_PRESS instead? The expected behavior is to notify on release, especially since it's possible to drag and drop the icon. https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... File ui/views/animation/ink_drop.h (right): https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... ui/views/animation/ink_drop.h:16: #include "ui/views/animation/ink_drop_observer.h" On 2017/03/22 17:20:16, bruthig wrote: > Can this be forward declared instead? Done. https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... ui/views/animation/ink_drop.h:55: void SetObserver(InkDropObserver* observer); On 2017/03/22 17:20:16, bruthig wrote: > I'd prefer if this used the Add/RemoveObserver() collection paradigm instead. Done. https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... File ui/views/animation/ink_drop_observer.h (right): https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... ui/views/animation/ink_drop_observer.h:11: #include "ui/views/animation/ink_drop_animation_ended_reason.h" On 2017/03/22 17:20:17, bruthig wrote: > nit: Is this needed? Done. https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... ui/views/animation/ink_drop_observer.h:12: #include "ui/views/animation/ink_drop_state.h" On 2017/03/22 17:20:17, bruthig wrote: > nit: Is this needed? Done. https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... ui/views/animation/ink_drop_observer.h:17: // Observer to attach to an InkDropRipple. On 2017/03/22 17:20:17, bruthig wrote: > 'InkDropRipple' -> just 'InkDrop' Done. https://codereview.chromium.org/2720183002/diff/500001/ui/views/animation/ink... ui/views/animation/ink_drop_observer.h:20: // Called when the animation of the current InkDrop has started. On 2017/03/22 17:20:16, bruthig wrote: > Can you be explicit here and mention that this includes a ripple or a highlight > animation. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:228: return CustomButton::OnMousePressed(event); Can you confirm that this isn't going to cause an ACTION_PENDING animation when |suppress_button_action_| is true? I think it will and I don't think that's what we want here? The best way would be with a test ;) https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:174: bool suppress_button_action_; On 2017/03/24 17:58:03, spqchan wrote: > On 2017/03/22 17:20:16, bruthig wrote: > > Instead of manually tracking this, would it be possible to configure the > > CustomButton to NOTIFY_ON_PRESS instead? > > The expected behavior is to notify on release, especially since it's possible to > drag and drop the icon. Right, I guess it just seems odd that this needs to be manually tracked. Why isn't it sufficient to check IsBubbleShown() instead of |suppress_button_action_|? And why is ButtonPressed() even getting called if IsTriggerable() overrides should be returning false? And this makes me wonder how the bubbles are being dismissed and closed? https://codereview.chromium.org/2720183002/diff/560001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/2720183002/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:200: // TODO(bruthig): See crbug.com/669253. The ink drop highlight is artifially nit: sp 'artifially' -> 'artificially' https://codereview.chromium.org/2720183002/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:201: // inflated to accomodate for that bug. This override needs to be removed once nit: 'inflated' -> 'larger' https://codereview.chromium.org/2720183002/diff/560001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:220: ink_drop_bounds.set_width(ink_drop_bounds.width() - Is this the correct layout for RTL layouts? https://codereview.chromium.org/2720183002/diff/560001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc (right): https://codereview.chromium.org/2720183002/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc:72: void OnWidgetVisibilityChanged(views::Widget* widget, bool visible) override { Is this needed? https://codereview.chromium.org/2720183002/diff/560001/ui/views/animation/ink... File ui/views/animation/ink_drop.h (right): https://codereview.chromium.org/2720183002/diff/560001/ui/views/animation/ink... ui/views/animation/ink_drop.h:62: base::ObserverList<InkDropObserver> observers_; Can you prevent subclasses from modifying this? Probably easiest done by making this private and providing a const accessor.
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:174: bool suppress_button_action_; On 2017/03/24 20:48:02, bruthig wrote: > On 2017/03/24 17:58:03, spqchan wrote: > > On 2017/03/22 17:20:16, bruthig wrote: > > > Instead of manually tracking this, would it be possible to configure the > > > CustomButton to NOTIFY_ON_PRESS instead? > > > > The expected behavior is to notify on release, especially since it's possible > to > > drag and drop the icon. > > Right, I guess it just seems odd that this needs to be manually tracked. Why > isn't it sufficient to check IsBubbleShown() instead of > |suppress_button_action_|? And why is ButtonPressed() even getting called if > IsTriggerable() overrides should be returning false? > And this makes me wonder how the bubbles are being dismissed and closed? I added a comment that explains this and modified the code so it's more clear. We need this flag because the bubble gets dismissed on MouseReleased before IsTriggerable() for the Mouse Released event got called. I changed it so that instead of suppress_button_action, we'll have suppress_button_release. IsTriggerable() will return !IsBubbleShown() && !suppress_button_release. https://codereview.chromium.org/2720183002/diff/560001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/2720183002/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:200: // TODO(bruthig): See crbug.com/669253. The ink drop highlight is artifially On 2017/03/24 20:48:02, bruthig wrote: > nit: sp 'artifially' -> 'artificially' Done. https://codereview.chromium.org/2720183002/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:201: // inflated to accomodate for that bug. This override needs to be removed once On 2017/03/24 20:48:02, bruthig wrote: > nit: 'inflated' -> 'larger' Done. https://codereview.chromium.org/2720183002/diff/560001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:220: ink_drop_bounds.set_width(ink_drop_bounds.width() - On 2017/03/24 20:48:03, bruthig wrote: > Is this the correct layout for RTL layouts? Yep https://codereview.chromium.org/2720183002/diff/560001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc (right): https://codereview.chromium.org/2720183002/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc:72: void OnWidgetVisibilityChanged(views::Widget* widget, bool visible) override { On 2017/03/24 20:48:03, bruthig wrote: > Is this needed? Removed https://codereview.chromium.org/2720183002/diff/560001/ui/views/animation/ink... File ui/views/animation/ink_drop.h (right): https://codereview.chromium.org/2720183002/diff/560001/ui/views/animation/ink... ui/views/animation/ink_drop.h:62: base::ObserverList<InkDropObserver> observers_; On 2017/03/24 20:48:03, bruthig wrote: > Can you prevent subclasses from modifying this? Probably easiest done by making > this private and providing a const accessor. Done.
Patchset #11 (id:580001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2720183002/diff/560001/ui/views/animation/ink... File ui/views/animation/ink_drop.h (right): https://codereview.chromium.org/2720183002/diff/560001/ui/views/animation/ink... ui/views/animation/ink_drop.h:62: base::ObserverList<InkDropObserver> observers_; On 2017/03/27 23:41:51, spqchan wrote: > On 2017/03/24 20:48:03, bruthig wrote: > > Can you prevent subclasses from modifying this? Probably easiest done by > making > > this private and providing a const accessor. > > Done. Thx :) https://codereview.chromium.org/2720183002/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:234: suppress_button_release_ = IsBubbleShown(); Correct me if I'm wrong but I think the IconLabelBubbleView will ignore touch events if the menu is dismissed via a MousePress/MouseRelease sequence? Ex: 1 - Press left mouse button 2 - Release left mouse button => Bubble will be shown 3 - Press left mouse button => |suppress_button_release_| will be set to true 4 - Release left mouse button a. => bubble will be hidden b. CustomButton::OnMouseReleased() will check IsTriggerableEvent() and avoid the call to ButtonPressed(). 5 - User taps (ui::ET_GESTURE_TAP event) => CustomButton::OnGestureEvent() will ignore it because |suppress_button_release_| is still true and IsTriggerableEvent() will return false. If possible this sequence is probably worth a test. I don't understand why/how/from where the bubble is being dismissed between steps 4a and 4b AND the events are being dispatched to |this|. I guess setting |suppress_button_release_| to false after 4b might work but it's tough to know for sure :(
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/03/28 21:33:34, bruthig (OOO Apr 2-9) wrote: > https://codereview.chromium.org/2720183002/diff/560001/ui/views/animation/ink... > File ui/views/animation/ink_drop.h (right): > > https://codereview.chromium.org/2720183002/diff/560001/ui/views/animation/ink... > ui/views/animation/ink_drop.h:62: base::ObserverList<InkDropObserver> > observers_; > On 2017/03/27 23:41:51, spqchan wrote: > > On 2017/03/24 20:48:03, bruthig wrote: > > > Can you prevent subclasses from modifying this? Probably easiest done by > > making > > > this private and providing a const accessor. > > > > Done. > > Thx :) > > https://codereview.chromium.org/2720183002/diff/600001/chrome/browser/ui/view... > File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): > > https://codereview.chromium.org/2720183002/diff/600001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:234: > suppress_button_release_ = IsBubbleShown(); > Correct me if I'm wrong but I think the IconLabelBubbleView will ignore touch > events if the menu is dismissed via a MousePress/MouseRelease sequence? > > Ex: > > 1 - Press left mouse button > 2 - Release left mouse button > => Bubble will be shown > 3 - Press left mouse button > => |suppress_button_release_| will be set to true > 4 - Release left mouse button > a. => bubble will be hidden > b. CustomButton::OnMouseReleased() will check IsTriggerableEvent() and avoid > the call to ButtonPressed(). > 5 - User taps (ui::ET_GESTURE_TAP event) > => CustomButton::OnGestureEvent() will ignore it because > |suppress_button_release_| is still true and IsTriggerableEvent() will return > false. > > If possible this sequence is probably worth a test. > > I don't understand why/how/from where the bubble is being dismissed between > steps 4a and 4b AND the events are being dispatched to |this|. > > I guess setting |suppress_button_release_| to false after 4b might work but it's > tough to know for sure :( FYI, my Windows machine died sometime this week :( My replacement is coming in on Monday so I'll continue to work on this CL once I get chromium set up on it
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #13 (id:640001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #13 (id:660001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/07 22:52:12, spqchan wrote: > On 2017/03/28 21:33:34, bruthig (OOO Apr 2-9) wrote: > > > https://codereview.chromium.org/2720183002/diff/560001/ui/views/animation/ink... > > File ui/views/animation/ink_drop.h (right): > > > > > https://codereview.chromium.org/2720183002/diff/560001/ui/views/animation/ink... > > ui/views/animation/ink_drop.h:62: base::ObserverList<InkDropObserver> > > observers_; > > On 2017/03/27 23:41:51, spqchan wrote: > > > On 2017/03/24 20:48:03, bruthig wrote: > > > > Can you prevent subclasses from modifying this? Probably easiest done by > > > making > > > > this private and providing a const accessor. > > > > > > Done. > > > > Thx :) > > > > > https://codereview.chromium.org/2720183002/diff/600001/chrome/browser/ui/view... > > File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): > > > > > https://codereview.chromium.org/2720183002/diff/600001/chrome/browser/ui/view... > > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:234: > > suppress_button_release_ = IsBubbleShown(); > > Correct me if I'm wrong but I think the IconLabelBubbleView will ignore touch > > events if the menu is dismissed via a MousePress/MouseRelease sequence? > > > > Ex: > > > > 1 - Press left mouse button > > 2 - Release left mouse button > > => Bubble will be shown > > 3 - Press left mouse button > > => |suppress_button_release_| will be set to true > > 4 - Release left mouse button > > a. => bubble will be hidden > > b. CustomButton::OnMouseReleased() will check IsTriggerableEvent() and avoid > > the call to ButtonPressed(). > > 5 - User taps (ui::ET_GESTURE_TAP event) > > => CustomButton::OnGestureEvent() will ignore it because > > |suppress_button_release_| is still true and IsTriggerableEvent() will return > > false. > > > > If possible this sequence is probably worth a test. > > > > I don't understand why/how/from where the bubble is being dismissed between > > steps 4a and 4b AND the events are being dispatched to |this|. > > > > I guess setting |suppress_button_release_| to false after 4b might work but > it's > > tough to know for sure :( > > FYI, my Windows machine died sometime this week :( > My replacement is coming in on Monday so I'll continue to work on this CL once I > get chromium set up on it Hey, It looks like CustomButton would handle gestures, but it won’t animate the ink drop. I made a change so that IconLabelBubbleView animates the ink drop to the ACTIVATED state, similar to what BubbleIconView does. One thing I noticed on Windows is that for some reason, adding a InkDropMask messes up the InkDropHighlight: https://screenshot.googleplex.com/4OkFYUckoYg. I suspect it’s messing up the highlight’s opacity. I’m not sure why it happens, InkDropRipple is fine with the mask. I tried fixing this issue but I didn’t have much luck. Do you have any idea what might be causing it?
On 2017/04/07 22:52:12, spqchan wrote: > On 2017/03/28 21:33:34, bruthig (OOO Apr 2-9) wrote: > > > https://codereview.chromium.org/2720183002/diff/560001/ui/views/animation/ink... > > File ui/views/animation/ink_drop.h (right): > > > > > https://codereview.chromium.org/2720183002/diff/560001/ui/views/animation/ink... > > ui/views/animation/ink_drop.h:62: base::ObserverList<InkDropObserver> > > observers_; > > On 2017/03/27 23:41:51, spqchan wrote: > > > On 2017/03/24 20:48:03, bruthig wrote: > > > > Can you prevent subclasses from modifying this? Probably easiest done by > > > making > > > > this private and providing a const accessor. > > > > > > Done. > > > > Thx :) > > > > > https://codereview.chromium.org/2720183002/diff/600001/chrome/browser/ui/view... > > File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): > > > > > https://codereview.chromium.org/2720183002/diff/600001/chrome/browser/ui/view... > > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:234: > > suppress_button_release_ = IsBubbleShown(); > > Correct me if I'm wrong but I think the IconLabelBubbleView will ignore touch > > events if the menu is dismissed via a MousePress/MouseRelease sequence? > > > > Ex: > > > > 1 - Press left mouse button > > 2 - Release left mouse button > > => Bubble will be shown > > 3 - Press left mouse button > > => |suppress_button_release_| will be set to true > > 4 - Release left mouse button > > a. => bubble will be hidden > > b. CustomButton::OnMouseReleased() will check IsTriggerableEvent() and avoid > > the call to ButtonPressed(). > > 5 - User taps (ui::ET_GESTURE_TAP event) > > => CustomButton::OnGestureEvent() will ignore it because > > |suppress_button_release_| is still true and IsTriggerableEvent() will return > > false. > > > > If possible this sequence is probably worth a test. > > > > I don't understand why/how/from where the bubble is being dismissed between > > steps 4a and 4b AND the events are being dispatched to |this|. > > > > I guess setting |suppress_button_release_| to false after 4b might work but > it's > > tough to know for sure :( > > FYI, my Windows machine died sometime this week :( > My replacement is coming in on Monday so I'll continue to work on this CL once I > get chromium set up on it Hey, It looks like CustomButton would handle gestures, but it won’t animate the ink drop. I made a change so that IconLabelBubbleView animates the ink drop to the ACTIVATED state, similar to what BubbleIconView does. One thing I noticed on Windows is that for some reason, adding a InkDropMask messes up the InkDropHighlight: https://screenshot.googleplex.com/4OkFYUckoYg. I suspect it’s messing up the highlight’s opacity. I’m not sure why it happens, InkDropRipple is fine with the mask. I tried fixing this issue but I didn’t have much luck. Do you have any idea what might be causing it?
> Hey, > > It looks like CustomButton would handle gestures, but it won’t animate the ink > drop. > I made a change so that IconLabelBubbleView animates the ink drop to the > ACTIVATED state, similar to what BubbleIconView does. > > One thing I noticed on Windows is that for some reason, adding a InkDropMask > messes up the InkDropHighlight: > https://screenshot.googleplex.com/4OkFYUckoYg. I suspect it’s messing up the > highlight’s opacity. > > I’m not sure why it happens, InkDropRipple is fine with the mask. I tried fixing > this issue but I didn’t have much luck. > Do you have any idea what might be causing it? Mohsen, any ideas why the mask would be messing with the highlights appearance?
On 2017/04/18 at 21:10:44, bruthig wrote: > > Hey, > > > > It looks like CustomButton would handle gestures, but it won’t animate the ink > > drop. > > I made a change so that IconLabelBubbleView animates the ink drop to the > > ACTIVATED state, similar to what BubbleIconView does. > > > > One thing I noticed on Windows is that for some reason, adding a InkDropMask > > messes up the InkDropHighlight: > > https://screenshot.googleplex.com/4OkFYUckoYg. I suspect it’s messing up the > > highlight’s opacity. > > > > I’m not sure why it happens, InkDropRipple is fine with the mask. I tried fixing > > this issue but I didn’t have much luck. > > Do you have any idea what might be causing it? > > Mohsen, any ideas why the mask would be messing with the highlights appearance? I think I saw a similar issue in buttons on download shelf when I was testing custom masking. I didn't dig into the issue though and I'm not sure what the problem is. (those buttons are not using masks, yet.)
On 2017/04/18 21:29:14, mohsen wrote: > On 2017/04/18 at 21:10:44, bruthig wrote: > > > Hey, > > > > > > It looks like CustomButton would handle gestures, but it won’t animate the > ink > > > drop. > > > I made a change so that IconLabelBubbleView animates the ink drop to the > > > ACTIVATED state, similar to what BubbleIconView does. > > > > > > One thing I noticed on Windows is that for some reason, adding a InkDropMask > > > messes up the InkDropHighlight: > > > https://screenshot.googleplex.com/4OkFYUckoYg. I suspect it’s messing up the > > > highlight’s opacity. > > > > > > I’m not sure why it happens, InkDropRipple is fine with the mask. I tried > fixing > > > this issue but I didn’t have much luck. > > > Do you have any idea what might be causing it? > > > > Mohsen, any ideas why the mask would be messing with the highlights > appearance? > > I think I saw a similar issue in buttons on download shelf when I was testing > custom masking. I didn't dig into the issue though and I'm not sure what the > problem is. (those buttons are not using masks, yet.) One thing I tried was modifying InkDropHighlight to copy what FloodFillInkDropRipple was doing, since the ripple looks fine. It didn't work out though, and it doesn't look like the InkDropMask is applied to the highlight different. I can try to investigate this again. If it doesn't work out, I can leave out the mask for now and not have an highlight appear for the popup button (and main reason why we're using the mask)
On 2017/04/18 21:35:17, spqchan wrote: > On 2017/04/18 21:29:14, mohsen wrote: > > On 2017/04/18 at 21:10:44, bruthig wrote: > > > > Hey, > > > > > > > > It looks like CustomButton would handle gestures, but it won’t animate the > > ink > > > > drop. > > > > I made a change so that IconLabelBubbleView animates the ink drop to the > > > > ACTIVATED state, similar to what BubbleIconView does. > > > > > > > > One thing I noticed on Windows is that for some reason, adding a > InkDropMask > > > > messes up the InkDropHighlight: > > > > https://screenshot.googleplex.com/4OkFYUckoYg. I suspect it’s messing up > the > > > > highlight’s opacity. > > > > > > > > I’m not sure why it happens, InkDropRipple is fine with the mask. I tried > > fixing > > > > this issue but I didn’t have much luck. > > > > Do you have any idea what might be causing it? > > > > > > Mohsen, any ideas why the mask would be messing with the highlights > > appearance? > > > > I think I saw a similar issue in buttons on download shelf when I was testing > > custom masking. I didn't dig into the issue though and I'm not sure what the > > problem is. (those buttons are not using masks, yet.) > > One thing I tried was modifying InkDropHighlight to copy what > FloodFillInkDropRipple was doing, since the ripple looks fine. > It didn't work out though, and it doesn't look like the InkDropMask is applied > to the highlight different. > > I can try to investigate this again. If it doesn't work out, I can leave out the > mask for now and not have an highlight appear for the popup button (and main > reason why we're using the mask) I think we're hitting this TODO: // TODO(miket): This does not work on Windows. Implement layer masking or // alternate solutions if the TrayBubbleView is needed there in the future. found here: https://cs.chromium.org/chromium/src/ui/views/bubble/tray_bubble_view.cc?rcl=... Stay tuned for offline email. I suspect including a mask in this CL will be too much. I'm surprised the ripple isn't affected in weird and wonderful ways. Can you double check that the mask is actually affecting the ripple? e.g. make it's size small like (5, 5) and ensure the ripple is clipped.
On 2017/04/19 16:05:20, bruthig wrote: > On 2017/04/18 21:35:17, spqchan wrote: > > On 2017/04/18 21:29:14, mohsen wrote: > > > On 2017/04/18 at 21:10:44, bruthig wrote: > > > > > Hey, > > > > > > > > > > It looks like CustomButton would handle gestures, but it won’t animate > the > > > ink > > > > > drop. > > > > > I made a change so that IconLabelBubbleView animates the ink drop to the > > > > > ACTIVATED state, similar to what BubbleIconView does. > > > > > > > > > > One thing I noticed on Windows is that for some reason, adding a > > InkDropMask > > > > > messes up the InkDropHighlight: > > > > > https://screenshot.googleplex.com/4OkFYUckoYg. I suspect it’s messing up > > the > > > > > highlight’s opacity. > > > > > > > > > > I’m not sure why it happens, InkDropRipple is fine with the mask. I > tried > > > fixing > > > > > this issue but I didn’t have much luck. > > > > > Do you have any idea what might be causing it? > > > > > > > > Mohsen, any ideas why the mask would be messing with the highlights > > > appearance? > > > > > > I think I saw a similar issue in buttons on download shelf when I was > testing > > > custom masking. I didn't dig into the issue though and I'm not sure what the > > > problem is. (those buttons are not using masks, yet.) > > > > One thing I tried was modifying InkDropHighlight to copy what > > FloodFillInkDropRipple was doing, since the ripple looks fine. > > It didn't work out though, and it doesn't look like the InkDropMask is applied > > to the highlight different. > > > > I can try to investigate this again. If it doesn't work out, I can leave out > the > > mask for now and not have an highlight appear for the popup button (and main > > reason why we're using the mask) > > I think we're hitting this TODO: > > // TODO(miket): This does not work on Windows. Implement layer masking or > // alternate solutions if the TrayBubbleView is needed there in the future. > > found here: > https://cs.chromium.org/chromium/src/ui/views/bubble/tray_bubble_view.cc?rcl=... > > Stay tuned for offline email. > > I suspect including a mask in this CL will be too much. > > I'm surprised the ripple isn't affected in weird and wonderful ways. Can you > double check that the mask is actually affecting the ripple? e.g. make it's size > small like (5, 5) and ensure the ripple is clipped. Can confirm, here's a (oddly) clipped ripple: https://screenshot.googleplex.com/dtyctJwM4Q9. The ripple's opacity it looks okay. Anyway, sounds good. I'll remove the InkDropMask and prevent the ink drop highlight from showing up when the popup decoration animates in. Want me to leave a comment about using InkDropMask in InkDropHostView?
On 2017/04/19 16:05:20, bruthig wrote: > On 2017/04/18 21:35:17, spqchan wrote: > > On 2017/04/18 21:29:14, mohsen wrote: > > > On 2017/04/18 at 21:10:44, bruthig wrote: > > > > > Hey, > > > > > > > > > > It looks like CustomButton would handle gestures, but it won’t animate > the > > > ink > > > > > drop. > > > > > I made a change so that IconLabelBubbleView animates the ink drop to the > > > > > ACTIVATED state, similar to what BubbleIconView does. > > > > > > > > > > One thing I noticed on Windows is that for some reason, adding a > > InkDropMask > > > > > messes up the InkDropHighlight: > > > > > https://screenshot.googleplex.com/4OkFYUckoYg. I suspect it’s messing up > > the > > > > > highlight’s opacity. > > > > > > > > > > I’m not sure why it happens, InkDropRipple is fine with the mask. I > tried > > > fixing > > > > > this issue but I didn’t have much luck. > > > > > Do you have any idea what might be causing it? > > > > > > > > Mohsen, any ideas why the mask would be messing with the highlights > > > appearance? > > > > > > I think I saw a similar issue in buttons on download shelf when I was > testing > > > custom masking. I didn't dig into the issue though and I'm not sure what the > > > problem is. (those buttons are not using masks, yet.) > > > > One thing I tried was modifying InkDropHighlight to copy what > > FloodFillInkDropRipple was doing, since the ripple looks fine. > > It didn't work out though, and it doesn't look like the InkDropMask is applied > > to the highlight different. > > > > I can try to investigate this again. If it doesn't work out, I can leave out > the > > mask for now and not have an highlight appear for the popup button (and main > > reason why we're using the mask) > > I think we're hitting this TODO: > > // TODO(miket): This does not work on Windows. Implement layer masking or > // alternate solutions if the TrayBubbleView is needed there in the future. > > found here: > https://cs.chromium.org/chromium/src/ui/views/bubble/tray_bubble_view.cc?rcl=... > > Stay tuned for offline email. > > I suspect including a mask in this CL will be too much. > > I'm surprised the ripple isn't affected in weird and wonderful ways. Can you > double check that the mask is actually affecting the ripple? e.g. make it's size > small like (5, 5) and ensure the ripple is clipped. Can confirm, here's a (oddly) clipped ripple: https://screenshot.googleplex.com/dtyctJwM4Q9. The ripple's opacity it looks okay. Anyway, sounds good. I'll remove the InkDropMask and prevent the ink drop highlight from showing up when the popup decoration animates in. Want me to leave a comment about using InkDropMask in InkDropHostView?
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL, I removed the InkDropMask and added the TODO
Hi Sarah, Thanks for sticking with this!! I've left a few more thoughts. Ben https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right): https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.cc:171: GetLocalBounds().CenterPoint(), GetInkDropBaseColor(), Was it an intentional decision to center the ripple on the view as opposed to using InkDropHostView::GetInkDropCenterBasedOnLastEvent() ? https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:206: ? nullptr CreateInkDropHighlight() overrides shouldn't return nullptr. (Ya, I know it currently works but there are some old TODO's to remove support for this) Anyway, would it be possible to configure the InkDropImpl as SetShowHighlightOnHover(false) and SetShowHighlightOnFocus(false) while the slide_animator_ is not null? https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:229: AnimateInkDrop(views::InkDropState::ACTIVATED, event); This animation seems a little distant from the spot that actually shows the bubble, i.e. OnActivate(). This might cause a ripple to persist in the ACTIVATED state when there is no bubble showing. Would it be possible to couple this together with the triggered action somehow? We could possibly remove the animation from BubbleIconView::OnGestureEvent() then too. Example template pattern: // Change return value and to be non-virtual. void IconLabelBubbleView::OnActivate(const ui::Event& e) { if (TriggerAction()) AnimateInkDrop(ACTIVATED, e); else AnimateInkDrop(HIDDEN, e); } // Returns true if a 'bubble' was shown. // May need to replace bool return value with enum if possible actions is more diverse. virtual bool IconLabelBubbleView::TriggerAction() {} https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:272: ink_drop_container_->size(), ink_drop_container_->bounds().CenterPoint(), Was it an intentional decision to center the ripple on the view as opposed to using InkDropHostView::GetInkDropCenterBasedOnLastEvent() ? https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:289: // Overriden to prevent CustomButton from hiding the ink drop when the click I believe this override may allow for an ACTION_PENDING ripple to incorrectly persist in the following situation: 1. Navigate to http://popuptest.com/popuptest1.html 2. Click and hold on the 'P' of the expanded 'Pop-up blocked' button 3. Maintain the click down and allow the 'Pop-up blocked' button to shrink 4. Release the mouse (This should be outside the bounds of the button since the mouse down happened near the 'P') Expected: The ripple should not persist on the button. Actual: The ripple persists. Can you see if there's an easy fix for this? If not we may have to capture it as a issue/TODO. https://codereview.chromium.org/2720183002/diff/700001/ui/views/animation/ink... File ui/views/animation/ink_drop.h (right): https://codereview.chromium.org/2720183002/diff/700001/ui/views/animation/ink... ui/views/animation/ink_drop.h:24: // Pure virtual base class that manages the lifetime and state of an ink drop Can you update the docs? This is no longer a pure-virtual class. https://codereview.chromium.org/2720183002/diff/700001/ui/views/animation/ink... File ui/views/animation/ink_drop_host_view.h (right): https://codereview.chromium.org/2720183002/diff/700001/ui/views/animation/ink... ui/views/animation/ink_drop_host_view.h:110: // TODO(brutig): InkDropMasks do not currently work on Windows. See sp nit: brutig -> bruthig https://codereview.chromium.org/2720183002/diff/700001/ui/views/animation/ink... File ui/views/animation/ink_drop_impl.cc (right): https://codereview.chromium.org/2720183002/diff/700001/ui/views/animation/ink... ui/views/animation/ink_drop_impl.cc:746: for (InkDropObserver& observer : *observers()) nit: Consider adding an InkDrop::NotifyInkDropAnimationStarted() method and simply calling it from here and below.
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #17 (id:760001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #18 (id:800001) has been deleted
Patchset #16 (id:740001) has been deleted
Patchset #16 (id:780001) has been deleted
Patchset #16 (id:820001) has been deleted
PTAL https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right): https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/bubble_icon_view.cc:171: GetLocalBounds().CenterPoint(), GetInkDropBaseColor(), On 2017/04/21 15:05:09, bruthig wrote: > Was it an intentional decision to center the ripple on the view as opposed to > using InkDropHostView::GetInkDropCenterBasedOnLastEvent() ? Nope, this method slipped past my radar https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:206: ? nullptr On 2017/04/21 15:05:09, bruthig wrote: > CreateInkDropHighlight() overrides shouldn't return nullptr. (Ya, I know it > currently works but there are some old TODO's to remove support for this) > > Anyway, would it be possible to configure the InkDropImpl as > SetShowHighlightOnHover(false) and SetShowHighlightOnFocus(false) while the > slide_animator_ is not null? Done. https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:229: AnimateInkDrop(views::InkDropState::ACTIVATED, event); On 2017/04/21 15:05:09, bruthig wrote: > This animation seems a little distant from the spot that actually shows the > bubble, i.e. OnActivate(). This might cause a ripple to persist in the > ACTIVATED state when there is no bubble showing. > Would it be possible to couple this together with the triggered action somehow? > > We could possibly remove the animation from BubbleIconView::OnGestureEvent() > then too. > > Example template pattern: > > // Change return value and to be non-virtual. > void IconLabelBubbleView::OnActivate(const ui::Event& e) { > if (TriggerAction()) > AnimateInkDrop(ACTIVATED, e); > else > AnimateInkDrop(HIDDEN, e); > } > > // Returns true if a 'bubble' was shown. > // May need to replace bool return value with enum if possible actions is more > diverse. > virtual bool IconLabelBubbleView::TriggerAction() {} Done. https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:272: ink_drop_container_->size(), ink_drop_container_->bounds().CenterPoint(), On 2017/04/21 15:05:09, bruthig wrote: > Was it an intentional decision to center the ripple on the view as opposed to > using InkDropHostView::GetInkDropCenterBasedOnLastEvent() ? Nope, I didn't notice this method. I can't use it here though, since the ink drop isn't actually centered for the bubble view (the ink drop container has different bounds) https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:289: // Overriden to prevent CustomButton from hiding the ink drop when the click On 2017/04/21 15:05:09, bruthig wrote: > I believe this override may allow for an ACTION_PENDING ripple to incorrectly > persist in the following situation: > > 1. Navigate to http://popuptest.com/popuptest1.html > 2. Click and hold on the 'P' of the expanded 'Pop-up blocked' button > 3. Maintain the click down and allow the 'Pop-up blocked' button to shrink > 4. Release the mouse (This should be outside the bounds of the button since the > mouse down happened near the 'P') > > Expected: > The ripple should not persist on the button. > > Actual: > The ripple persists. > > Can you see if there's an easy fix for this? If not we may have to capture it > as a issue/TODO. I added a fix. In OnBoundsChanged, if the view is no longer hover, the InkDrop will be animated to HIDDEN and the hover state will be set to false. There's still an issue where if the click is at the right of the button, the ripple will persist (like it's supposed to) but the ink drop size will be incorrect. This will be fixed by crbug.com/669253, I think I'll give that a shot after this CL lands :) https://codereview.chromium.org/2720183002/diff/700001/ui/views/animation/ink... File ui/views/animation/ink_drop.h (right): https://codereview.chromium.org/2720183002/diff/700001/ui/views/animation/ink... ui/views/animation/ink_drop.h:24: // Pure virtual base class that manages the lifetime and state of an ink drop On 2017/04/21 15:05:10, bruthig wrote: > Can you update the docs? This is no longer a pure-virtual class. Done. https://codereview.chromium.org/2720183002/diff/700001/ui/views/animation/ink... File ui/views/animation/ink_drop_host_view.h (right): https://codereview.chromium.org/2720183002/diff/700001/ui/views/animation/ink... ui/views/animation/ink_drop_host_view.h:110: // TODO(brutig): InkDropMasks do not currently work on Windows. See On 2017/04/21 15:05:10, bruthig wrote: > sp nit: brutig -> bruthig whoops, sorry https://codereview.chromium.org/2720183002/diff/700001/ui/views/animation/ink... File ui/views/animation/ink_drop_impl.cc (right): https://codereview.chromium.org/2720183002/diff/700001/ui/views/animation/ink... ui/views/animation/ink_drop_impl.cc:746: for (InkDropObserver& observer : *observers()) On 2017/04/21 15:05:10, bruthig wrote: > nit: Consider adding an InkDrop::NotifyInkDropAnimationStarted() method and > simply calling it from here and below. Done.
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've left a few comments but I need to ponder on the InkDropObservers a little bit more. My concern is that other InkDrop sub-classes don't notify them, and this is mostly concerning in tests that swap out InkDrop instances for test doubles. Perhaps we need to add an InkDropHost::OnInkDropAttached() and add the observers in there.... PS I think the diff between patch set 16 and 15 also includes a rebase/merge with master. It's easier as a reviewer if authors separates those out into distinct patch sets ;) https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:272: ink_drop_container_->size(), ink_drop_container_->bounds().CenterPoint(), On 2017/04/27 21:40:06, spqchan wrote: > On 2017/04/21 15:05:09, bruthig wrote: > > Was it an intentional decision to center the ripple on the view as opposed to > > using InkDropHostView::GetInkDropCenterBasedOnLastEvent() ? > > Nope, I didn't notice this method. I can't use it here though, since the ink > drop isn't actually centered for the bubble view > (the ink drop container has different bounds) Would it be possible to use View::ConvertPointToTarget() to convert the point to the |ink_drop_container_| coordinates? https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:289: // Overriden to prevent CustomButton from hiding the ink drop when the click On 2017/04/27 21:40:06, spqchan wrote: > On 2017/04/21 15:05:09, bruthig wrote: > > I believe this override may allow for an ACTION_PENDING ripple to incorrectly > > persist in the following situation: > > > > 1. Navigate to http://popuptest.com/popuptest1.html > > 2. Click and hold on the 'P' of the expanded 'Pop-up blocked' button > > 3. Maintain the click down and allow the 'Pop-up blocked' button to shrink > > 4. Release the mouse (This should be outside the bounds of the button since > the > > mouse down happened near the 'P') > > > > Expected: > > The ripple should not persist on the button. > > > > Actual: > > The ripple persists. > > > > Can you see if there's an easy fix for this? If not we may have to capture it > > as a issue/TODO. > > I added a fix. In OnBoundsChanged, if the view is no longer hover, the InkDrop > will be animated to HIDDEN and the hover state will be set to false. There's > still an issue where if the click is at the right of the button, the ripple will > persist (like it's supposed to) but the ink drop size will be incorrect. This > will be fixed by crbug.com/669253, I think I'll give that a shot after this CL > lands :) Acknowledged. https://codereview.chromium.org/2720183002/diff/840001/ash/shelf/shelf_view_u... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2720183002/diff/840001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:1893: // TODO(spqchan): Setting this for ink_drop_ causes the ink drop tests to Which tests fail? I tried running ash_unittests, on Linux, with the change to call ink_drop_->SetShowHighlightOnHover(show_highlight_on_hover) here. https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:67: padding = next_element_interior_padding_; Should this be reverted? https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc (right): https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc:56: void SetInkDrop(std::unique_ptr<views::InkDrop> ink_drop) { Is this used? https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc:306: view()->set_is_bubble_shown(true); Can the |ink_drop_| be put in the ACTIVATED state here to be more accurate? https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc:311: generator()->GestureTapAt(gfx::Point()); I think this '#if defined' block would be better as an independent test. e.g. IconLabelBubbleViewTest.InkDropStateForGestures. It's not clear to me whether the previous steps in this test are actually prerequisite setup.... e.g. is it important that the left mouse button is currently pressed?! https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_icon_view.cc (right): https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_icon_view.cc:120: const ui::MouseEvent& mouseev = static_cast<const ui::MouseEvent&>(event); nit: I think event.AsMouseEvent() is preferred. https://codereview.chromium.org/2720183002/diff/840001/ui/views/animation/ink... File ui/views/animation/ink_drop_host_view.h (right): https://codereview.chromium.org/2720183002/diff/840001/ui/views/animation/ink... ui/views/animation/ink_drop_host_view.h:110: // TODO(bruthig): InkDropMasks do not currently work on Windows. See Thx for adding this!
On 2017/05/01 22:42:18, bruthig wrote: > I've left a few comments but I need to ponder on the InkDropObservers a little > bit more. My concern is that other InkDrop sub-classes don't notify them, and > this is mostly concerning in tests that swap out InkDrop instances for test > doubles. Perhaps we need to add an InkDropHost::OnInkDropAttached() and add the > observers in there.... I can't think of a good suggestion here as the use-case is new and not really flushed out completely. I think we will need to move forward as-is and update it as needed as the use-cases mature. Can you add a doc to the InkDrop::(Add|Remove)Observer() methods that the observers are not guaranteed to be notified and it is dependent on an implementation/subclass basis.
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #17 (id:860001) has been deleted
Whoops, I'll be sure to add a rebase patch in the future to make things easier. Thanks for letting me know! Anyway, I addressed your comments in this patch. PTAL https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:272: ink_drop_container_->size(), ink_drop_container_->bounds().CenterPoint(), On 2017/05/01 22:42:18, bruthig wrote: > On 2017/04/27 21:40:06, spqchan wrote: > > On 2017/04/21 15:05:09, bruthig wrote: > > > Was it an intentional decision to center the ripple on the view as opposed > to > > > using InkDropHostView::GetInkDropCenterBasedOnLastEvent() ? > > > > Nope, I didn't notice this method. I can't use it here though, since the ink > > drop isn't actually centered for the bubble view > > (the ink drop container has different bounds) > > Would it be possible to use View::ConvertPointToTarget() to convert the point to > the |ink_drop_container_| coordinates? I'm a bit confused with what you mean. How would I use that method to get the center point? https://codereview.chromium.org/2720183002/diff/840001/ash/shelf/shelf_view_u... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2720183002/diff/840001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:1893: // TODO(spqchan): Setting this for ink_drop_ causes the ink drop tests to On 2017/05/01 22:42:18, bruthig wrote: > Which tests fail? I tried running ash_unittests, on Linux, with the change to > call ink_drop_->SetShowHighlightOnHover(show_highlight_on_hover) here. Oh hey, looks like this fixed it: https://chromium.googlesource.com/chromium/src/+/8c6e6113a0f57941108e5267f699... The changes that they made in the CL were the tests that was failing. Awesome, one less TODO for me. Thanks for checking it out! https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:67: padding = next_element_interior_padding_; On 2017/05/01 22:42:18, bruthig wrote: > Should this be reverted? Done. https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc (right): https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc:56: void SetInkDrop(std::unique_ptr<views::InkDrop> ink_drop) { On 2017/05/01 22:42:18, bruthig wrote: > Is this used? Removed https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc:306: view()->set_is_bubble_shown(true); On 2017/05/01 22:42:18, bruthig wrote: > Can the |ink_drop_| be put in the ACTIVATED state here to be more accurate? Done. https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc:311: generator()->GestureTapAt(gfx::Point()); On 2017/05/01 22:42:18, bruthig wrote: > I think this '#if defined' block would be better as an independent test. e.g. > IconLabelBubbleViewTest.InkDropStateForGestures. It's not clear to me whether > the previous steps in this test are actually prerequisite setup.... e.g. is it > important that the left mouse button is currently pressed?! Done. https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_icon_view.cc (right): https://codereview.chromium.org/2720183002/diff/840001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_icon_view.cc:120: const ui::MouseEvent& mouseev = static_cast<const ui::MouseEvent&>(event); On 2017/05/01 22:42:18, bruthig wrote: > nit: I think event.AsMouseEvent() is preferred. Done. https://codereview.chromium.org/2720183002/diff/840001/ui/views/animation/ink... File ui/views/animation/ink_drop_host_view.h (right): https://codereview.chromium.org/2720183002/diff/840001/ui/views/animation/ink... ui/views/animation/ink_drop_host_view.h:110: // TODO(bruthig): InkDropMasks do not currently work on Windows. See On 2017/05/01 22:42:18, bruthig wrote: > Thx for adding this! No prob~
lgtm https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:272: ink_drop_container_->size(), ink_drop_container_->bounds().CenterPoint(), On 2017/05/02 22:50:05, spqchan wrote: > On 2017/05/01 22:42:18, bruthig wrote: > > On 2017/04/27 21:40:06, spqchan wrote: > > > On 2017/04/21 15:05:09, bruthig wrote: > > > > Was it an intentional decision to center the ripple on the view as opposed > > to > > > > using InkDropHostView::GetInkDropCenterBasedOnLastEvent() ? > > > > > > Nope, I didn't notice this method. I can't use it here though, since the ink > > > drop isn't actually centered for the bubble view > > > (the ink drop container has different bounds) > > > > Would it be possible to use View::ConvertPointToTarget() to convert the point > to > > the |ink_drop_container_| coordinates? > > I'm a bit confused with what you mean. How would I use that method to get the > center point? I guess you'd also have to snap the point to the, possibly smaller, |ink_drop_container_| bounds. i.e. gfx::Point center_point = GetInkDropCenterBasedOnLastEvent().; View::ConvertPointToTarget(this, ink_drop_container_, center_point); center_point.SetToMax(ink_drop_container_.origin()); center_point.SetToMin(ink_drop_container.bottom_right()); return base::MakeUnique<view::FloodFillInkDropRipple>( ink_drop_container_->size(), center_point, GetInkDropBNaseColor(), ink_drop_visible_opacity());
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
spqchan@chromium.org changed reviewers: + sky@chromium.org
Thanks! +sky for ownership Please review everything that's not in ui/views/animation https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:272: ink_drop_container_->size(), ink_drop_container_->bounds().CenterPoint(), On 2017/05/02 23:49:56, bruthig wrote: > On 2017/05/02 22:50:05, spqchan wrote: > > On 2017/05/01 22:42:18, bruthig wrote: > > > On 2017/04/27 21:40:06, spqchan wrote: > > > > On 2017/04/21 15:05:09, bruthig wrote: > > > > > Was it an intentional decision to center the ripple on the view as > opposed > > > to > > > > > using InkDropHostView::GetInkDropCenterBasedOnLastEvent() ? > > > > > > > > Nope, I didn't notice this method. I can't use it here though, since the > ink > > > > drop isn't actually centered for the bubble view > > > > (the ink drop container has different bounds) > > > > > > Would it be possible to use View::ConvertPointToTarget() to convert the > point > > to > > > the |ink_drop_container_| coordinates? > > > > I'm a bit confused with what you mean. How would I use that method to get the > > center point? > > I guess you'd also have to snap the point to the, possibly smaller, > |ink_drop_container_| bounds. i.e. > > gfx::Point center_point = GetInkDropCenterBasedOnLastEvent().; > View::ConvertPointToTarget(this, ink_drop_container_, center_point); > center_point.SetToMax(ink_drop_container_.origin()); > center_point.SetToMin(ink_drop_container.bottom_right()); > > return base::MakeUnique<view::FloodFillInkDropRipple>( > ink_drop_container_->size(), center_point, > GetInkDropBNaseColor(), ink_drop_visible_opacity()); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/03 00:43:51, spqchan wrote: > Thanks! > > +sky for ownership > Please review everything that's not in ui/views/animation > > https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... > File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): > > https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:272: > ink_drop_container_->size(), ink_drop_container_->bounds().CenterPoint(), > On 2017/05/02 23:49:56, bruthig wrote: > > On 2017/05/02 22:50:05, spqchan wrote: > > > On 2017/05/01 22:42:18, bruthig wrote: > > > > On 2017/04/27 21:40:06, spqchan wrote: > > > > > On 2017/04/21 15:05:09, bruthig wrote: > > > > > > Was it an intentional decision to center the ripple on the view as > > opposed > > > > to > > > > > > using InkDropHostView::GetInkDropCenterBasedOnLastEvent() ? > > > > > > > > > > Nope, I didn't notice this method. I can't use it here though, since the > > ink > > > > > drop isn't actually centered for the bubble view > > > > > (the ink drop container has different bounds) > > > > > > > > Would it be possible to use View::ConvertPointToTarget() to convert the > > point > > > to > > > > the |ink_drop_container_| coordinates? > > > > > > I'm a bit confused with what you mean. How would I use that method to get > the > > > center point? > > > > I guess you'd also have to snap the point to the, possibly smaller, > > |ink_drop_container_| bounds. i.e. > > > > gfx::Point center_point = GetInkDropCenterBasedOnLastEvent().; > > View::ConvertPointToTarget(this, ink_drop_container_, center_point); > > center_point.SetToMax(ink_drop_container_.origin()); > > center_point.SetToMin(ink_drop_container.bottom_right()); > > > > return base::MakeUnique<view::FloodFillInkDropRipple>( > > ink_drop_container_->size(), center_point, > > GetInkDropBNaseColor(), ink_drop_visible_opacity()); > > Done. Ping on this, let me know if you have any questions!
Here's some initial comments. I'll look more deeply later. https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:31: constexpr int kFadeInDurationMs = 250; Where do these numbers come from? Shouldn't we use consistent numbers for animations? https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:42: SetPaintToLayer(); It would be nice if you only enabled the layer when necessary (animating). https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:57: bounds.Inset(0, (bounds.height() - kSeparatorHeight) / 2); Why don't you position the SeparatorView rather than making it take the whole bounds and then only drawing a small portion of that? https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:64: return false; Do you actually need this? AFAICT you don't add children to SeparateView. https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:68: views::InkDrop* ink_drop = owner_->GetInkDrop(); Is it worth running the animation if owner_->ShouldShowLabel() is false? https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:294: // Overriden to prevent CustomButton from hiding the ink drop when the click This makes me mildly nervous. If CustomButton ever did anything else in OnClickCanceled this wouldn't work right. It's safter to have CustomButton::OnClickCanceled() call a virtual function that updates the ink drop state that you can override. Or perhaps a ShouldUpdateInkDropOnClickCanceled(). https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:47: void ButtonPressed(Button* sender, const ui::Event& event) override; It's mildly weird that this is a subclass of CustomButton *and* the ButtonListener for itself. How about overriding NotifyClick() and not extending ButtonListener? https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:142: }; DISALLOW...
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
Patchset #19 (id:920001) has been deleted
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
Patchset #19 (id:940001) has been deleted
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:31: constexpr int kFadeInDurationMs = 250; On 2017/05/05 23:40:20, sky wrote: > Where do these numbers come from? Shouldn't we use consistent numbers for > animations? These numbers are based from observation on what looks good. It looks slightly better with the difference. I added a comment. https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:42: SetPaintToLayer(); On 2017/05/05 23:40:20, sky wrote: > It would be nice if you only enabled the layer when necessary (animating). Done. https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:57: bounds.Inset(0, (bounds.height() - kSeparatorHeight) / 2); On 2017/05/05 23:40:20, sky wrote: > Why don't you position the SeparatorView rather than making it take the whole > bounds and then only drawing a small portion of that? Done. https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:64: return false; On 2017/05/05 23:40:20, sky wrote: > Do you actually need this? AFAICT you don't add children to SeparateView. Yes, otherwise the SeparatorView will consume the events before the parents could. https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:68: views::InkDrop* ink_drop = owner_->GetInkDrop(); On 2017/05/05 23:40:20, sky wrote: > Is it worth running the animation if owner_->ShouldShowLabel() is false? Done. https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:294: // Overriden to prevent CustomButton from hiding the ink drop when the click On 2017/05/05 23:40:20, sky wrote: > This makes me mildly nervous. If CustomButton ever did anything else in > OnClickCanceled this wouldn't work right. It's safter to have > CustomButton::OnClickCanceled() call a virtual function that updates the ink > drop state that you can override. Or perhaps a > ShouldUpdateInkDropOnClickCanceled(). Done. https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:47: void ButtonPressed(Button* sender, const ui::Event& event) override; On 2017/05/05 23:40:20, sky wrote: > It's mildly weird that this is a subclass of CustomButton *and* the > ButtonListener for itself. How about overriding NotifyClick() and not extending > ButtonListener? Done. https://codereview.chromium.org/2720183002/diff/900001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:142: }; On 2017/05/05 23:40:20, sky wrote: > DISALLOW... Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:55: return false; Why do you need to override this? It doesn't seem like add child views. https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:176: bool IconLabelBubbleView::OnKeyPressed(const ui::KeyEvent& event) { Now that IconLabelBubbleView subclasses CustomButton is the override here necessary? CustomButton has slightly different handling, but I suspect it's more correct than what is here. https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:214: const int kSeparatorHeight = 16; Did you make sure all this looks right when RTL? https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:86: virtual bool IsBubbleShown() const; IsBubbleShowing() https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:172: std::unique_ptr<SeparatorView> separator_view_; Why do you use a unique_ptr for SeparatorView but not ink_drop_container_? Generally in views we don't use unique_ptr as the views are owned by the containment hierarchy (e.g. image_ and label_ are not in unique_ptr). https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:180: // event. If this is true then the IsTriggerableEvent() will return false to Remove 'the' or add 'function' before 'will'. https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:183: bool suppress_button_release_; Is there test coverage of this? By that I mean a test that generates a press makes sure IsBubbleShowing() is true and then generates another press to make sure the bubble closes? If not, please add one. https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc (right): https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc:111: }; no ; https://codereview.chromium.org/2720183002/diff/980001/ui/views/controls/butt... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2720183002/diff/980001/ui/views/controls/butt... ui/views/controls/button/custom_button.cc:494: if (ShouldUpdateInkDropOnClickCanceled()) { If this returns false, how is the ink drop updated?
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #22 (id:1020001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #21 (id:1000001) has been deleted
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #23 (id:1080001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #23 (id:1100001) has been deleted
PTAL. I addressed your comments and added a new test. I also rebased the CL https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:55: return false; On 2017/05/12 13:11:47, sky wrote: > Why do you need to override this? It doesn't seem like add child views. Whoops, sorry I missed your earlier comment. While separator_view doesn't add any subviews, it can still and will process events before it reaches the parent view. https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:176: bool IconLabelBubbleView::OnKeyPressed(const ui::KeyEvent& event) { On 2017/05/12 13:11:47, sky wrote: > Now that IconLabelBubbleView subclasses CustomButton is the override here > necessary? CustomButton has slightly different handling, but I suspect it's more > correct than what is here. Done. https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:214: const int kSeparatorHeight = 16; On 2017/05/12 13:11:47, sky wrote: > Did you make sure all this looks right when RTL? Good point, I changed the separator's position a bit. It now looks right in RTL https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:86: virtual bool IsBubbleShown() const; On 2017/05/12 13:11:48, sky wrote: > IsBubbleShowing() Done. https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:172: std::unique_ptr<SeparatorView> separator_view_; On 2017/05/12 13:11:48, sky wrote: > Why do you use a unique_ptr for SeparatorView but not ink_drop_container_? > Generally in views we don't use unique_ptr as the views are owned by the > containment hierarchy (e.g. image_ and label_ are not in unique_ptr). Removed unique_ptr https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:180: // event. If this is true then the IsTriggerableEvent() will return false to On 2017/05/12 13:11:48, sky wrote: > Remove 'the' or add 'function' before 'will'. Done. https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:183: bool suppress_button_release_; On 2017/05/12 13:11:48, sky wrote: > Is there test coverage of this? By that I mean a test that generates a press > makes sure IsBubbleShowing() is true and then generates another press to make > sure the bubble closes? If not, please add one. Done. Added in the unit test https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc (right): https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc:111: }; On 2017/05/12 13:11:48, sky wrote: > no ; Done. https://codereview.chromium.org/2720183002/diff/980001/ui/views/controls/butt... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2720183002/diff/980001/ui/views/controls/butt... ui/views/controls/button/custom_button.cc:494: if (ShouldUpdateInkDropOnClickCanceled()) { On 2017/05/12 13:11:48, sky wrote: > If this returns false, how is the ink drop updated? The ink drop gets updated by the subclass. I added a comment on it
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/05/18 01:13:05, spqchan wrote: > PTAL. I addressed your comments and added a new test. > I also rebased the CL > > https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... > File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): > > https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:55: return false; > On 2017/05/12 13:11:47, sky wrote: > > Why do you need to override this? It doesn't seem like add child views. > > Whoops, sorry I missed your earlier comment. While separator_view doesn't add > any subviews, it can still and will process events before it reaches the parent > view. > > https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:176: bool > IconLabelBubbleView::OnKeyPressed(const ui::KeyEvent& event) { > On 2017/05/12 13:11:47, sky wrote: > > Now that IconLabelBubbleView subclasses CustomButton is the override here > > necessary? CustomButton has slightly different handling, but I suspect it's > more > > correct than what is here. > > Done. > > https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:214: const int > kSeparatorHeight = 16; > On 2017/05/12 13:11:47, sky wrote: > > Did you make sure all this looks right when RTL? > > Good point, I changed the separator's position a bit. It now looks right in RTL > > https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... > File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): > > https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:86: virtual bool > IsBubbleShown() const; > On 2017/05/12 13:11:48, sky wrote: > > IsBubbleShowing() > > Done. > > https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:172: > std::unique_ptr<SeparatorView> separator_view_; > On 2017/05/12 13:11:48, sky wrote: > > Why do you use a unique_ptr for SeparatorView but not ink_drop_container_? > > Generally in views we don't use unique_ptr as the views are owned by the > > containment hierarchy (e.g. image_ and label_ are not in unique_ptr). > > Removed unique_ptr > > https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:180: // event. If > this is true then the IsTriggerableEvent() will return false to > On 2017/05/12 13:11:48, sky wrote: > > Remove 'the' or add 'function' before 'will'. > > Done. > > https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:183: bool > suppress_button_release_; > On 2017/05/12 13:11:48, sky wrote: > > Is there test coverage of this? By that I mean a test that generates a press > > makes sure IsBubbleShowing() is true and then generates another press to make > > sure the bubble closes? If not, please add one. > > Done. Added in the unit test > > https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... > File chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc > (right): > > https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc:111: }; > On 2017/05/12 13:11:48, sky wrote: > > no ; > > Done. > > https://codereview.chromium.org/2720183002/diff/980001/ui/views/controls/butt... > File ui/views/controls/button/custom_button.cc (right): > > https://codereview.chromium.org/2720183002/diff/980001/ui/views/controls/butt... > ui/views/controls/button/custom_button.cc:494: if > (ShouldUpdateInkDropOnClickCanceled()) { > On 2017/05/12 13:11:48, sky wrote: > > If this returns false, how is the ink drop updated? > > The ink drop gets updated by the subclass. I added a comment on it Sorry, please hold the CL. It looks like one of the tests is failing
Patchset #24 (id:1130001) has been deleted
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Test is fixed. PTAL, thanks!
https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:55: return false; On 2017/05/18 01:13:04, spqchan wrote: > On 2017/05/12 13:11:47, sky wrote: > > Why do you need to override this? It doesn't seem like add child views. > > Whoops, sorry I missed your earlier comment. While separator_view doesn't add > any subviews, it can still and will process events before it reaches the parent > view. That should only matter if child views are added and you don't want them to process events. Who is adding child views?
https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/2720183002/diff/980001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:55: return false; On 2017/05/18 23:28:38, sky wrote: > On 2017/05/18 01:13:04, spqchan wrote: > > On 2017/05/12 13:11:47, sky wrote: > > > Why do you need to override this? It doesn't seem like add child views. > > > > Whoops, sorry I missed your earlier comment. While separator_view doesn't add > > any subviews, it can still and will process events before it reaches the > parent > > view. > > That should only matter if child views are added and you don't want them to > process events. Who is adding child views? IconLabelBubbleView adds the child views (including the SeparatorView) to itself. No child view is added to SeparatorView. However, to prevent the SeparatorView from becoming a target of an event, I overrode this method since it "returns true if this view or any of its descendants are permitted to be the target of an event". Are there any alternatives that I should use instead?
What happens if you don't override said function? On Thu, May 18, 2017 at 4:58 PM, <spqchan@chromium.org> wrote: > > https://codereview.chromium.org/2720183002/diff/980001/ > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc > File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc > (right): > > https://codereview.chromium.org/2720183002/diff/980001/ > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc#newcode55 > chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:55: > return false; > On 2017/05/18 23:28:38, sky wrote: > > On 2017/05/18 01:13:04, spqchan wrote: > > > On 2017/05/12 13:11:47, sky wrote: > > > > Why do you need to override this? It doesn't seem like add child > views. > > > > > > Whoops, sorry I missed your earlier comment. While separator_view > doesn't add > > > any subviews, it can still and will process events before it reaches > the > > parent > > > view. > > > > That should only matter if child views are added and you don't want > them to > > process events. Who is adding child views? > > IconLabelBubbleView adds the child views (including the SeparatorView) > to itself. No child view is added to SeparatorView. > > However, to prevent the SeparatorView from becoming a target of an > event, I overrode this method since it "returns true if this view or any > of its descendants are permitted to be the target of an event". Are > there any alternatives that I should use instead? > > https://codereview.chromium.org/2720183002/ > -- 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.
The InkDrop doesn't appear when you hover over the separator
On 2017/05/19 17:15:24, spqchan wrote: > The InkDrop doesn't appear when you hover over the separator What happens if call set_notify_enter_exit_on_child(true) on the parent?
On 2017/05/19 17:46:26, sky wrote: > On 2017/05/19 17:15:24, spqchan wrote: > > The InkDrop doesn't appear when you hover over the separator > > What happens if call set_notify_enter_exit_on_child(true) on the parent? Tried it and it works :) I updated the CL
LGTM
On 2017/05/22 16:27:08, sky wrote: > LGTM Thanks all!
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org Link to the patchset: https://codereview.chromium.org/2720183002/#ps1170001 (title: "Removed CanProcessEventsWithinSubtree")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1170001, "attempt_start_ts": 1495470556752630,
"parent_rev": "1a25f28b9fbf2ec51a542202b9c0576723f3932b", "commit_rev":
"bf7e0532e34476062226eab6ef1affe78abc8a8d"}
Message was sent while issue was closed.
Description was changed from ========== [Views] Update hover/active states for omnibox icons - Update the ink drop ripple behavior on the omnibox icons so that they use the flood behavior instead - Implement hover/active states for the page actions icon and popup blocker Previously, ink drops were not enabled for IconLabelBubbleView when the the label was visible since the ink drop would make the text blurry. To fix this, an InkDropContainerView is now added to contain the ink drop. The separator needs to fade out when the ink drop fades in, so a new view containing the separator is now created. IconLabelBubbleView, ContentSettingImageView and LocationIconView are also refactored so that IconLabelBubbleView handles all of the logic for the ink drop and suppressing mouse release events when the bubble is already opened. BUG=588377 ========== to ========== [Views] Update hover/active states for omnibox icons - Update the ink drop ripple behavior on the omnibox icons so that they use the flood behavior instead - Implement hover/active states for the page actions icon and popup blocker Previously, ink drops were not enabled for IconLabelBubbleView when the the label was visible since the ink drop would make the text blurry. To fix this, an InkDropContainerView is now added to contain the ink drop. The separator needs to fade out when the ink drop fades in, so a new view containing the separator is now created. IconLabelBubbleView, ContentSettingImageView and LocationIconView are also refactored so that IconLabelBubbleView handles all of the logic for the ink drop and suppressing mouse release events when the bubble is already opened. BUG=588377 Review-Url: https://codereview.chromium.org/2720183002 Cr-Commit-Position: refs/heads/master@{#473647} Committed: https://chromium.googlesource.com/chromium/src/+/bf7e0532e34476062226eab6ef1a... ==========
Message was sent while issue was closed.
Committed patchset #25 (id:1170001) as https://chromium.googlesource.com/chromium/src/+/bf7e0532e34476062226eab6ef1a... |
