Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(12)

Issue 2720183002: [Views] Update ink drop for omnibox icons (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 3 weeks ago by spqchan
Modified:
1 day, 10 hours ago
Reviewers:
sky, bruthig
CC:
chromium-reviews, tfarina, bruthig+ink_drop_chromium.org, dcheng
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+648 lines, -215 lines) Patch
M ash/shelf/shelf_view_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +12 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/browser_dialogs_views_mac.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/frame/browser_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/views/location_bar/bubble_icon_view.h View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/location_bar/bubble_icon_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +11 lines, -1 line 0 comments Download
M chrome/browser/ui/views/location_bar/content_setting_image_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 22 5 chunks +9 lines, -14 lines 0 comments Download
M chrome/browser/ui/views/location_bar/content_setting_image_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 22 9 chunks +21 lines, -47 lines 0 comments Download
M chrome/browser/ui/views/location_bar/icon_label_bubble_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 4 chunks +71 lines, -9 lines 0 comments Download
M chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 9 chunks +204 lines, -42 lines 0 comments Download
M chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 10 chunks +127 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/location_bar/location_bar_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/location_bar/location_icon_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 22 4 chunks +7 lines, -20 lines 0 comments Download
M chrome/browser/ui/views/location_bar/location_icon_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 22 7 chunks +23 lines, -40 lines 0 comments Download
M chrome/browser/ui/views/page_info/page_info_bubble_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/page_info/page_info_bubble_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 4 chunks +6 lines, -0 lines 0 comments Download
M ui/views/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -0 lines 0 comments Download
M ui/views/animation/ink_drop.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +25 lines, -4 lines 0 comments Download
M ui/views/animation/ink_drop.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +21 lines, -0 lines 0 comments Download
M ui/views/animation/ink_drop_host_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +2 lines, -0 lines 0 comments Download
M ui/views/animation/ink_drop_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +3 lines, -8 lines 0 comments Download
M ui/views/animation/ink_drop_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +18 lines, -15 lines 0 comments Download
A ui/views/animation/ink_drop_observer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +34 lines, -0 lines 0 comments Download
M ui/views/animation/ink_drop_stub.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +3 lines, -0 lines 0 comments Download
M ui/views/animation/ink_drop_stub.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +8 lines, -0 lines 0 comments Download
M ui/views/animation/test/test_ink_drop.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +3 lines, -0 lines 0 comments Download
M ui/views/animation/test/test_ink_drop.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +8 lines, -0 lines 0 comments Download
M ui/views/controls/button/custom_button.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 22 1 chunk +6 lines, -0 lines 0 comments Download
M ui/views/controls/button/custom_button.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +12 lines, -6 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 308 (261 generated)
spqchan
Hello bruthig, can you review this? I'm implementing the new ink drop behaviour on the ...
2 months, 2 weeks ago (2017-03-08 02:58:31 UTC) #50
bruthig
Hey, I've taken a look from an ink drop usage perspective and have left some ...
2 months, 2 weeks ago (2017-03-08 18:58:24 UTC) #51
spqchan
Thanks for looking into this. I addressed your comments and refactored some stuff. https://codereview.chromium.org/2720183002/diff/240001/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc File ...
2 months, 1 week ago (2017-03-14 00:07:52 UTC) #81
bruthig
+sky@ Sorry it took me until the second pass to think of this but, I ...
2 months, 1 week ago (2017-03-14 03:49:47 UTC) #82
bruthig
On 2017/03/14 03:49:47, bruthig wrote: > +sky@ > > Sorry it took me until the ...
2 months, 1 week ago (2017-03-14 03:50:54 UTC) #83
sky
I think it would be good to make these classes have a Button as well, ...
2 months, 1 week ago (2017-03-14 16:53:21 UTC) #84
spqchan
On 2017/03/14 16:53:21, sky wrote: > I think it would be good to make these ...
2 months, 1 week ago (2017-03-14 19:29:34 UTC) #85
spqchan
I switched to CustomButton and added the InkDropMask bruthig mentioned earlier (I accidentally missed his ...
2 months ago (2017-03-21 18:25:23 UTC) #113
bruthig
Thanks for introducing the CustomButton =D On 2017/03/21 18:25:23, spqchan wrote: > I switched to ...
2 months ago (2017-03-22 17:20:17 UTC) #114
spqchan
>It seems you have stumbled upon crbug.com/669253. Artificially increasing the InkDropHighlights size and clipping it ...
2 months ago (2017-03-24 17:58:04 UTC) #125
bruthig
https://codereview.chromium.org/2720183002/diff/500001/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/500001/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc#newcode228 chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:228: return CustomButton::OnMousePressed(event); Can you confirm that this isn't going ...
2 months ago (2017-03-24 20:48:03 UTC) #128
spqchan
PTAL https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/views/location_bar/icon_label_bubble_view.h File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/2720183002/diff/500001/chrome/browser/ui/views/location_bar/icon_label_bubble_view.h#newcode174 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: > ...
1 month, 3 weeks ago (2017-03-27 23:41:51 UTC) #137
bruthig
https://codereview.chromium.org/2720183002/diff/560001/ui/views/animation/ink_drop.h File ui/views/animation/ink_drop.h (right): https://codereview.chromium.org/2720183002/diff/560001/ui/views/animation/ink_drop.h#newcode62 ui/views/animation/ink_drop.h:62: base::ObserverList<InkDropObserver> observers_; On 2017/03/27 23:41:51, spqchan wrote: > On ...
1 month, 3 weeks ago (2017-03-28 21:33:34 UTC) #141
spqchan
On 2017/03/28 21:33:34, bruthig (OOO Apr 2-9) wrote: > https://codereview.chromium.org/2720183002/diff/560001/ui/views/animation/ink_drop.h > File ui/views/animation/ink_drop.h (right): > ...
1 month, 2 weeks ago (2017-04-07 22:52:12 UTC) #146
spqchan
On 2017/04/07 22:52:12, spqchan wrote: > On 2017/03/28 21:33:34, bruthig (OOO Apr 2-9) wrote: > ...
1 month ago (2017-04-18 20:55:56 UTC) #155
spqchan
On 2017/04/07 22:52:12, spqchan wrote: > On 2017/03/28 21:33:34, bruthig (OOO Apr 2-9) wrote: > ...
1 month ago (2017-04-18 20:56:02 UTC) #156
bruthig
> Hey, > > It looks like CustomButton would handle gestures, but it won’t animate ...
1 month ago (2017-04-18 21:10:44 UTC) #157
mohsen
On 2017/04/18 at 21:10:44, bruthig wrote: > > Hey, > > > > It looks ...
1 month ago (2017-04-18 21:29:14 UTC) #158
spqchan
On 2017/04/18 21:29:14, mohsen wrote: > On 2017/04/18 at 21:10:44, bruthig wrote: > > > ...
1 month ago (2017-04-18 21:35:17 UTC) #159
bruthig
On 2017/04/18 21:35:17, spqchan wrote: > On 2017/04/18 21:29:14, mohsen wrote: > > On 2017/04/18 ...
1 month ago (2017-04-19 16:05:20 UTC) #160
spqchan
On 2017/04/19 16:05:20, bruthig wrote: > On 2017/04/18 21:35:17, spqchan wrote: > > On 2017/04/18 ...
1 month ago (2017-04-19 17:45:19 UTC) #161
spqchan
On 2017/04/19 16:05:20, bruthig wrote: > On 2017/04/18 21:35:17, spqchan wrote: > > On 2017/04/18 ...
1 month ago (2017-04-19 17:45:24 UTC) #162
spqchan
PTAL, I removed the InkDropMask and added the TODO
1 month ago (2017-04-19 22:32:19 UTC) #167
bruthig
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/views/location_bar/bubble_icon_view.cc ...
1 month ago (2017-04-21 15:05:10 UTC) #168
spqchan
PTAL https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/views/location_bar/bubble_icon_view.cc File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right): https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/views/location_bar/bubble_icon_view.cc#newcode171 chrome/browser/ui/views/location_bar/bubble_icon_view.cc:171: GetLocalBounds().CenterPoint(), GetInkDropBaseColor(), On 2017/04/21 15:05:09, bruthig wrote: > ...
3 weeks, 5 days ago (2017-04-27 21:40:06 UTC) #214
bruthig
I've left a few comments but I need to ponder on the InkDropObservers a little ...
3 weeks, 1 day ago (2017-05-01 22:42:18 UTC) #219
bruthig
On 2017/05/01 22:42:18, bruthig wrote: > I've left a few comments but I need to ...
3 weeks ago (2017-05-02 16:05:52 UTC) #220
spqchan
Whoops, I'll be sure to add a rebase patch in the future to make things ...
3 weeks ago (2017-05-02 22:50:05 UTC) #226
bruthig
lgtm https://codereview.chromium.org/2720183002/diff/700001/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/700001/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc#newcode272 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: > ...
3 weeks ago (2017-05-02 23:49:57 UTC) #227
spqchan
Thanks! +sky for ownership Please review everything that's not in ui/views/animation https://codereview.chromium.org/2720183002/diff/700001/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): ...
3 weeks ago (2017-05-03 00:43:51 UTC) #231
spqchan
On 2017/05/03 00:43:51, spqchan wrote: > Thanks! > > +sky for ownership > Please review ...
2 weeks, 4 days ago (2017-05-05 22:55:53 UTC) #234
sky
Here's some initial comments. I'll look more deeply later. https://codereview.chromium.org/2720183002/diff/900001/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/900001/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc#newcode31 chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:31: ...
2 weeks, 4 days ago (2017-05-05 23:40:20 UTC) #235
spqchan
PTAL https://codereview.chromium.org/2720183002/diff/900001/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/900001/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc#newcode31 chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:31: constexpr int kFadeInDurationMs = 250; On 2017/05/05 23:40:20, ...
1 week, 6 days ago (2017-05-10 19:22:12 UTC) #254
sky
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; Why do you need to override this? ...
1 week, 4 days ago (2017-05-12 13:11:48 UTC) #257
spqchan
PTAL. I addressed your comments and added a new test. I also rebased the CL ...
6 days, 4 hours ago (2017-05-18 01:13:05 UTC) #284
spqchan
On 2017/05/18 01:13:05, spqchan wrote: > PTAL. I addressed your comments and added a new ...
6 days ago (2017-05-18 05:17:33 UTC) #288
spqchan
Test is fixed. PTAL, thanks!
5 days, 9 hours ago (2017-05-18 20:22:51 UTC) #294
sky
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 01:13:04, spqchan wrote: > On ...
5 days, 6 hours ago (2017-05-18 23:28:38 UTC) #295
spqchan
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 ...
5 days, 5 hours ago (2017-05-18 23:58:09 UTC) #296
sky
What happens if you don't override said function? On Thu, May 18, 2017 at 4:58 ...
4 days, 15 hours ago (2017-05-19 14:11:26 UTC) #297
spqchan
The InkDrop doesn't appear when you hover over the separator
4 days, 12 hours ago (2017-05-19 17:15:24 UTC) #298
sky
On 2017/05/19 17:15:24, spqchan wrote: > The InkDrop doesn't appear when you hover over the ...
4 days, 12 hours ago (2017-05-19 17:46:26 UTC) #299
spqchan
On 2017/05/19 17:46:26, sky wrote: > On 2017/05/19 17:15:24, spqchan wrote: > > The InkDrop ...
4 days, 7 hours ago (2017-05-19 22:34:00 UTC) #300
sky
LGTM
1 day, 13 hours ago (2017-05-22 16:27:08 UTC) #301
spqchan
On 2017/05/22 16:27:08, sky wrote: > LGTM Thanks all!
1 day, 13 hours ago (2017-05-22 16:29:04 UTC) #302
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2720183002/1170001
1 day, 13 hours ago (2017-05-22 16:29:43 UTC) #305
commit-bot: I haz the power
1 day, 10 hours ago (2017-05-22 19:11:08 UTC) #308
Message was sent while issue was closed.
Committed patchset #25 (id:1170001) as
https://chromium.googlesource.com/chromium/src/+/bf7e0532e34476062226eab6ef1a...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06