Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2495213006:
[top-chrome-md] Made extension buttons show the highlight when focused. (Closed)
|
Created:
4 years, 1 month ago by bruthig Modified:
4 years, 1 month ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, varkha Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[top-chrome-md] Made extension buttons show the highlight when focused.
This is fixing a recent regression caused by
https://codereview.chromium.org/2447523002.
BUG=664906
Committed: https://crrev.com/3b5f4f77a0227bc21cd8616b6e24c48c18484b3d
Cr-Commit-Position: refs/heads/master@{#432102}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Merge branch 'master' into keyboard_focus_on_extensions #Patch Set 3 : Removed work to remove ripple from extension buttons shown inside the menu. #Patch Set 4 : Removed unnecessary include. #Messages
Total messages: 22 (13 generated)
The CQ bit was checked by bruthig@chromium.org to run a CQ dry run
bruthig@chromium.org changed reviewers: + pkasting@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pkasting@, can you PTAL?
https://codereview.chromium.org/2495213006/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/2495213006/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:126: gfx::Rect(), gfx::Point(), SK_ColorTRANSPARENT, 0.f); Can we just return null, and have the caller handle that correctly?
https://codereview.chromium.org/2495213006/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/2495213006/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:126: gfx::Rect(), gfx::Point(), SK_ColorTRANSPARENT, 0.f); On 2016/11/15 00:38:39, Peter Kasting wrote: > Can we just return null, and have the caller handle that correctly? Short answer, returning null is a possible long term solution but isn't that straight forward to do since the InkDropImpl can kick off highlight animations based on the starting/ending of ripple animations. This is a quick fix which is strictly better. Issue 665214 will remain open afterwards to track the work required for a better solution.
https://codereview.chromium.org/2495213006/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/2495213006/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:126: gfx::Rect(), gfx::Point(), SK_ColorTRANSPARENT, 0.f); On 2016/11/15 01:25:12, bruthig wrote: > On 2016/11/15 00:38:39, Peter Kasting wrote: > > Can we just return null, and have the caller handle that correctly? > > Short answer, returning null is a possible long term solution but isn't that > straight forward to do since the InkDropImpl can kick off highlight animations > based on the starting/ending of ripple animations. This is a quick fix which is > strictly better. Issue 665214 will remain open afterwards to track the work > required for a better solution. I commented there about the overall route here. I'm concerned that removing the ripple isn't the right behavior change to begin with. How bad is this bug? Can we just leave the ripple for items inside menus for now, until (assuming we want to make this change) we have the right framework to do this better, e.g. some kind of null ripple class that just kicks off subsequent animations, or callers that kick off those animations themselves when there's no ripple, or something? It seems like the addition on line 14 is the most critical one to get in soon?
Description was changed from ========== [top-chrome-md] Made extension buttons show the highlight when focused. This is fixing a recent regression caused by https://codereview.chromium.org/2447523002. This change also makes the ripple effect of the ink drop transparent when animating on extension buttons shown in the Chrome menu. BUG=664906, 665214 ========== to ========== [top-chrome-md] Made extension buttons show the highlight when focused. This is fixing a recent regression caused by https://codereview.chromium.org/2447523002. BUG=664906 ==========
The CQ bit was checked by bruthig@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 bruthig@chromium.org
The CQ bit was checked by bruthig@chromium.org to run a CQ dry run
Peter, can you please take another look? https://codereview.chromium.org/2495213006/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/2495213006/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:126: gfx::Rect(), gfx::Point(), SK_ColorTRANSPARENT, 0.f); On 2016/11/15 01:47:36, Peter Kasting wrote: > On 2016/11/15 01:25:12, bruthig wrote: > > On 2016/11/15 00:38:39, Peter Kasting wrote: > > > Can we just return null, and have the caller handle that correctly? > > > > Short answer, returning null is a possible long term solution but isn't that > > straight forward to do since the InkDropImpl can kick off highlight animations > > based on the starting/ending of ripple animations. This is a quick fix which > is > > strictly better. Issue 665214 will remain open afterwards to track the work > > required for a better solution. > > I commented there about the overall route here. I'm concerned that removing the > ripple isn't the right behavior change to begin with. > > How bad is this bug? Can we just leave the ripple for items inside menus for > now, until (assuming we want to make this change) we have the right framework to > do this better, e.g. some kind of null ripple class that just kicks off > subsequent animations, or callers that kick off those animations themselves when > there's no ripple, or something? It seems like the addition on line 14 is the > most critical one to get in soon? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
The CQ bit was unchecked by bruthig@chromium.org
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [top-chrome-md] Made extension buttons show the highlight when focused. This is fixing a recent regression caused by https://codereview.chromium.org/2447523002. BUG=664906 ========== to ========== [top-chrome-md] Made extension buttons show the highlight when focused. This is fixing a recent regression caused by https://codereview.chromium.org/2447523002. BUG=664906 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [top-chrome-md] Made extension buttons show the highlight when focused. This is fixing a recent regression caused by https://codereview.chromium.org/2447523002. BUG=664906 ========== to ========== [top-chrome-md] Made extension buttons show the highlight when focused. This is fixing a recent regression caused by https://codereview.chromium.org/2447523002. BUG=664906 Committed: https://crrev.com/3b5f4f77a0227bc21cd8616b6e24c48c18484b3d Cr-Commit-Position: refs/heads/master@{#432102} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3b5f4f77a0227bc21cd8616b6e24c48c18484b3d Cr-Commit-Position: refs/heads/master@{#432102} |
