Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2067893002:
Don't use dashed focus painter on back button in MD mode. (Closed)
|
Created:
4 years, 6 months ago by Evan Stade Modified:
4 years, 6 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't use dashed focus painter on back button in MD mode.
BUG=619831
Committed: https://crrev.com/17947a83c6d3edc2802033376a0690517aa190df
Cr-Commit-Position: refs/heads/master@{#399940}
Patch Set 1 #
Total comments: 3
Patch Set 2 : switch on md flag instead #
Total comments: 1
Messages
Total messages: 23 (8 generated)
estade@chromium.org changed reviewers: + pkasting@chromium.org
LGTM https://codereview.chromium.org/2067893002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/back_button.cc (right): https://codereview.chromium.org/2067893002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/back_button.cc:24: if (focus_painter()) { Who would be setting this? It reads here a bit like "if there's a focus painter, set a focus painter" which sounds circular. Should this have a comment on how the focus painter would get set here?
https://codereview.chromium.org/2067893002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/back_button.cc (right): https://codereview.chromium.org/2067893002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/back_button.cc:24: if (focus_painter()) { On 2016/06/14 17:51:59, Peter Kasting wrote: > Who would be setting this? It reads here a bit like "if there's a focus > painter, set a focus painter" which sounds circular. Should this have a comment > on how the focus painter would get set here? ToolbarButton (the parent class). In my head it reads as, "If there's supposed to be a focus painter, give it special insets. If there's no focus painter, don't add one."
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067893002/1
https://codereview.chromium.org/2067893002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/back_button.cc (right): https://codereview.chromium.org/2067893002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/back_button.cc:24: if (focus_painter()) { On 2016/06/14 17:57:34, Evan Stade wrote: > On 2016/06/14 17:51:59, Peter Kasting wrote: > > Who would be setting this? It reads here a bit like "if there's a focus > > painter, set a focus painter" which sounds circular. Should this have a > comment > > on how the focus painter would get set here? > > ToolbarButton (the parent class). In my head it reads as, "If there's supposed > to be a focus painter, give it special insets. If there's no focus painter, > don't add one." Does ToolbarButton ever add such a focus painter? It seems like giving accessibility focus to the button doesn't do this in MD, so I'm wondering if we should just nuke the focus painter code here entirely, or maybe make it pre-MD only.
On 2016/06/14 18:00:21, Peter Kasting wrote: > https://codereview.chromium.org/2067893002/diff/1/chrome/browser/ui/views/too... > File chrome/browser/ui/views/toolbar/back_button.cc (right): > > https://codereview.chromium.org/2067893002/diff/1/chrome/browser/ui/views/too... > chrome/browser/ui/views/toolbar/back_button.cc:24: if (focus_painter()) { > On 2016/06/14 17:57:34, Evan Stade wrote: > > On 2016/06/14 17:51:59, Peter Kasting wrote: > > > Who would be setting this? It reads here a bit like "if there's a focus > > > painter, set a focus painter" which sounds circular. Should this have a > > comment > > > on how the focus painter would get set here? > > > > ToolbarButton (the parent class). In my head it reads as, "If there's supposed > > to be a focus painter, give it special insets. If there's no focus painter, > > don't add one." > > Does ToolbarButton ever add such a focus painter? It seems like giving > accessibility focus to the button doesn't do this in MD, so I'm wondering if we > should just nuke the focus painter code here entirely, or maybe make it pre-MD > only. yes, it is pre-md only. https://cs.chromium.org/chromium/src/chrome/browser/ui/views/toolbar/toolbar_...
On 2016/06/14 18:14:34, Evan Stade wrote: > On 2016/06/14 18:00:21, Peter Kasting wrote: > > > https://codereview.chromium.org/2067893002/diff/1/chrome/browser/ui/views/too... > > File chrome/browser/ui/views/toolbar/back_button.cc (right): > > > > > https://codereview.chromium.org/2067893002/diff/1/chrome/browser/ui/views/too... > > chrome/browser/ui/views/toolbar/back_button.cc:24: if (focus_painter()) { > > On 2016/06/14 17:57:34, Evan Stade wrote: > > > On 2016/06/14 17:51:59, Peter Kasting wrote: > > > > Who would be setting this? It reads here a bit like "if there's a focus > > > > painter, set a focus painter" which sounds circular. Should this have a > > > comment > > > > on how the focus painter would get set here? > > > > > > ToolbarButton (the parent class). In my head it reads as, "If there's > supposed > > > to be a focus painter, give it special insets. If there's no focus painter, > > > don't add one." > > > > Does ToolbarButton ever add such a focus painter? It seems like giving > > accessibility focus to the button doesn't do this in MD, so I'm wondering if > we > > should just nuke the focus painter code here entirely, or maybe make it pre-MD > > only. > > yes, it is pre-md only. > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/toolbar/toolbar_... I'd stick an MD condition on this code then so we know we can rip it out when pre-MD is removed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/14 18:39:06, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. ok, done
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067893002/20001
SLGTM https://codereview.chromium.org/2067893002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/back_button.cc (right): https://codereview.chromium.org/2067893002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.cc:25: if (!ui::MaterialDesignController::IsModeMaterial()) { Heh, I'd been thinking to have both conditions here, but since the other condition should always be true pre-MD, your way is better.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) ios-device-gn on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2067893002/#ps20001 (title: "switch on md flag instead")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067893002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Don't use dashed focus painter on back button in MD mode. BUG=619831 ========== to ========== Don't use dashed focus painter on back button in MD mode. BUG=619831 Committed: https://crrev.com/17947a83c6d3edc2802033376a0690517aa190df Cr-Commit-Position: refs/heads/master@{#399940} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/17947a83c6d3edc2802033376a0690517aa190df Cr-Commit-Position: refs/heads/master@{#399940} |
