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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 4 weeks ago by spqchan
Modified:
1 month ago
Reviewers:
sky, bruthig
CC:
chromium-reviews, tfarina, bruthig+ink_drop_chromium.org, dcheng (OOO Jun 25 - Jul 1)
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 ...
3 months, 3 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 ...
3 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 ...
3 months, 2 weeks 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 ...
3 months, 2 weeks 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 ...
3 months, 2 weeks 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, ...
3 months, 2 weeks 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 ...
3 months, 2 weeks 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 ...
3 months, 1 week 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 ...
3 months, 1 week 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 ...
3 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 ...
3 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: > ...
3 months 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 ...
3 months 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): > ...
2 months, 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: > ...
2 months, 1 week 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: > ...
2 months, 1 week ago (2017-04-18 20:56:02 UTC) #156
bruthig
> Hey, > > It looks like CustomButton would handle gestures, but it won’t animate ...
2 months, 1 week ago (2017-04-18 21:10:44 UTC) #157
mohsen
On 2017/04/18 at 21:10:44, bruthig wrote: > > Hey, > > > > It looks ...
2 months, 1 week 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: > > > ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week ago (2017-04-19 17:45:24 UTC) #162
spqchan
PTAL, I removed the InkDropMask and added the TODO
2 months, 1 week 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 ...
2 months, 1 week 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: > ...
2 months 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 ...
1 month, 3 weeks 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 ...
1 month, 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 ...
1 month, 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: > ...
1 month, 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): ...
1 month, 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 ...
1 month, 3 weeks 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: ...
1 month, 3 weeks 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 month, 2 weeks 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 month, 2 weeks 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 ...
1 month, 1 week 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 ...
1 month, 1 week ago (2017-05-18 05:17:33 UTC) #288
spqchan
Test is fixed. PTAL, thanks!
1 month, 1 week 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 ...
1 month, 1 week 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 ...
1 month, 1 week 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 ...
1 month, 1 week ago (2017-05-19 14:11:26 UTC) #297
spqchan
The InkDrop doesn't appear when you hover over the separator
1 month, 1 week 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 ...
1 month, 1 week 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 ...
1 month, 1 week ago (2017-05-19 22:34:00 UTC) #300
sky
LGTM
1 month ago (2017-05-22 16:27:08 UTC) #301
spqchan
On 2017/05/22 16:27:08, sky wrote: > LGTM Thanks all!
1 month 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 month ago (2017-05-22 16:29:43 UTC) #305
commit-bot: I haz the power
1 month 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 cb946e318