Update ash shelf/tray focus rects.

Update ash shelf/tray focus rects.

Also improve Canvas::DrawSolidFocusRect and
Painter::CreateSolidFocusPainter.

BUG=658783, 652478, 615892
Committed: https://crrev.com/e69c1ffdb22164631d597a6b44c83ecd515d5281
Cr-Commit-Position: refs/heads/master@{#435309}

[Evan Stade, 2016-11-22 20:15:18]

estade@chromium.org changed reviewers:
+ tdanderson@chromium.org

[Evan Stade, 2016-11-22 20:15:19]

Terry, mind taking a first look?

[tdanderson, 2016-11-23 02:19:07]

LG, thanks! See my note about palette, though.

https://codereview.chromium.org/2527513002/diff/20001/ash/common/ash_constant...
File ash/common/ash_constants.cc (right):

https://codereview.chromium.org/2527513002/diff/20001/ash/common/ash_constant...
ash/common/ash_constants.cc:21: const SkColor kFocusBorderColor =
SkColorSetA(gfx::kGoogleBlue500, 0x99);
I'm /mildly/ concerned about these focus rects looking out of place in non-MD,
in the case where we have to ship non-MD in 57 for some reason. But that's a lot
of 'ifs' and I'm not concerned enough to insist that everything be gated on a
MaterialDesignController check.

https://codereview.chromium.org/2527513002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/palette/palette_tray.cc (right):

https://codereview.chromium.org/2527513002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/palette_tray.cc:115:
IDS_ASH_STATUS_TRAY_SETTINGS);
Actually I would hold off on touching the palette code at least for the time
being; Jacob has a CL in progress
(https://chromiumcodereview.appspot.com/2502153002/) that makes this change plus
fixes some of the fonts, which I'm hoping will land and get merged into 56 for a
consistent look and feel with the system menu.

https://codereview.chromium.org/2527513002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tray_popup_utils.cc (right):

https://codereview.chromium.org/2527513002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_popup_utils.cc:121: kFocusBorderColor,
kFocusBorderThickness, gfx::InsetsF()));
optional: consider adding a GetFocusPainter() to TrayPopupUtils that will return
new views::Painter::CreateSolidFocusPainter(...) and call that instead
throughout this CL.

[Evan Stade, 2016-11-28 19:04:03]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 19:05:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2016-11-28 19:05:53]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[Evan Stade, 2016-11-28 19:05:54]

+sky for OWNERS of everything except ash/common/system/

https://codereview.chromium.org/2527513002/diff/20001/ash/common/ash_constant...
File ash/common/ash_constants.cc (right):

https://codereview.chromium.org/2527513002/diff/20001/ash/common/ash_constant...
ash/common/ash_constants.cc:21: const SkColor kFocusBorderColor =
SkColorSetA(gfx::kGoogleBlue500, 0x99);
On 2016/11/23 02:19:07, tdanderson wrote:
> I'm /mildly/ concerned about these focus rects looking out of place in non-MD,
> in the case where we have to ship non-MD in 57 for some reason. But that's a
lot
> of 'ifs' and I'm not concerned enough to insist that everything be gated on a
> MaterialDesignController check.

Acknowledged. I don't think we should be working to keep non-MD presentable for
m57+.

https://codereview.chromium.org/2527513002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/palette/palette_tray.cc (right):

https://codereview.chromium.org/2527513002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/palette_tray.cc:115:
IDS_ASH_STATUS_TRAY_SETTINGS);
On 2016/11/23 02:19:07, tdanderson wrote:
> Actually I would hold off on touching the palette code at least for the time
> being; Jacob has a CL in progress
> (https://chromiumcodereview.appspot.com/2502153002/) that makes this change
plus
> fixes some of the fonts, which I'm hoping will land and get merged into 56 for
a
> consistent look and feel with the system menu. 

Done.

https://codereview.chromium.org/2527513002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tray_popup_utils.cc (right):

https://codereview.chromium.org/2527513002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_popup_utils.cc:121: kFocusBorderColor,
kFocusBorderThickness, gfx::InsetsF()));
On 2016/11/23 02:19:07, tdanderson wrote:
> optional: consider adding a GetFocusPainter() to TrayPopupUtils that will
return
> new views::Painter::CreateSolidFocusPainter(...) and call that instead
> throughout this CL.

Done.

[commit-bot: I haz the power, 2016-11-28 19:48:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 19:48:28]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[sky, 2016-11-28 21:27:31]

https://codereview.chromium.org/2527513002/diff/40001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/2527513002/diff/40001/ui/gfx/canvas.cc#newcod...
ui/gfx/canvas.cc:348: float thickness = 1.f) {
Isn't the default value only specified in the declaration?

https://codereview.chromium.org/2527513002/diff/40001/ui/views/painter.cc
File ui/views/painter.cc (right):

https://codereview.chromium.org/2527513002/diff/40001/ui/views/painter.cc#new...
ui/views/painter.cc:366: const gfx::Insets corrected_insets = insets -
gfx::Insets(0, 0, 1, 1);
Ick. But I assume this is going to be nuked.

[Evan Stade, 2016-11-29 02:04:13]

https://codereview.chromium.org/2527513002/diff/40001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/2527513002/diff/40001/ui/gfx/canvas.cc#newcod...
ui/gfx/canvas.cc:348: float thickness = 1.f) {
On 2016/11/28 21:27:31, sky wrote:
> Isn't the default value only specified in the declaration?

hmm, yes, not sure why I thought this was useful but I doubt it would have
compiled if any call site tried to leave off the argument. I've removed the
default value.

https://codereview.chromium.org/2527513002/diff/40001/ui/views/painter.cc
File ui/views/painter.cc (right):

https://codereview.chromium.org/2527513002/diff/40001/ui/views/painter.cc#new...
ui/views/painter.cc:366: const gfx::Insets corrected_insets = insets -
gfx::Insets(0, 0, 1, 1);
On 2016/11/28 21:27:31, sky wrote:
> Ick. But I assume this is going to be nuked.

yea. It's only used by the message center at this point and hopefully someone
will turn their attention to updating that at some point.

[sky, 2016-11-29 17:49:16]

LGTM

[Evan Stade, 2016-11-29 17:52:55]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 17:53:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 18:53:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 18:53:55]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2016-11-29 18:58:34]

ash/common/system LGTM

https://codereview.chromium.org/2527513002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/palette/palette_tray.cc (right):

https://codereview.chromium.org/2527513002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/palette_tray.cc:115:
IDS_ASH_STATUS_TRAY_SETTINGS);
On 2016/11/28 19:05:53, Evan Stade wrote:
> On 2016/11/23 02:19:07, tdanderson wrote:
> > Actually I would hold off on touching the palette code at least for the time
> > being; Jacob has a CL in progress
> > (https://chromiumcodereview.appspot.com/2502153002/) that makes this change
> plus
> > fixes some of the fonts, which I'm hoping will land and get merged into 56
for
> a
> > consistent look and feel with the system menu. 
> 
> Done.

Just double checked and the palette should indeed have all of the correct focus
rects for free once this change lands. No further modifications needed to the
palette code.

[Evan Stade, 2016-11-29 23:41:35]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-11-29 23:43:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2016-11-30 01:35:43]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-11-30 01:35:51]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-11-30 01:36:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 01:56:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 01:56:39]

Failed to apply patch for ash/common/system/tray/tray_popup_utils.cc:
While running git apply --index -p1;
  error: patch failed: ash/common/system/tray/tray_popup_utils.cc:117
  error: ash/common/system/tray/tray_popup_utils.cc: patch does not apply

Patch:       ash/common/system/tray/tray_popup_utils.cc
Index: ash/common/system/tray/tray_popup_utils.cc
diff --git a/ash/common/system/tray/tray_popup_utils.cc
b/ash/common/system/tray/tray_popup_utils.cc
index
b5e3deb294380a84caf165b59617c1bbc0a1994d..93ddd18df2d28c13a868a0c82b82956c09fb3bd8
100644
--- a/ash/common/system/tray/tray_popup_utils.cc
+++ b/ash/common/system/tray/tray_popup_utils.cc
@@ -117,13 +117,7 @@ class BorderlessLabelButton : public views::LabelButton {
       SetBorder(views::CreateEmptyBorder(gfx::Insets(0, kHorizontalPadding)));
       TrayPopupItemStyle style(nullptr, TrayPopupItemStyle::FontStyle::BUTTON);
       style.SetupLabel(label());
-      // TODO(tdanderson): Update focus rect for material design. See
-      // crbug.com/615892
-      // Hack alert: CreateSolidFocusPainter should add 0.5f to all insets to
-      // make the lines align to pixel centers, but for now it doesn't. We can
-      // get around this by relying on Skia rounding up integer coordinates.
-      SetFocusPainter(views::Painter::CreateSolidFocusPainter(
-          kFocusBorderColor, gfx::Insets(0, 0, 1, 1)));
+      SetFocusPainter(TrayPopupUtils::CreateFocusPainter());
     } else {
       SetBorder(std::unique_ptr<views::Border>(new
TrayPopupLabelButtonBorder));
       SetFocusPainter(views::Painter::CreateSolidFocusPainter(
@@ -243,7 +237,6 @@ views::ImageView* TrayPopupUtils::CreateMoreImageView() {
 views::Slider* TrayPopupUtils::CreateSlider(views::SliderListener* listener) {
   const bool is_material =
MaterialDesignController::IsSystemTrayMenuMaterial();
   views::Slider* slider = views::Slider::CreateSlider(is_material, listener);
-  slider->set_focus_border_color(kFocusBorderColor);
   if (is_material) {
     slider->SetBorder(
         views::CreateEmptyBorder(gfx::Insets(0, kTrayPopupSliderPaddingMD)));
@@ -264,14 +257,18 @@ views::ToggleButton* TrayPopupUtils::CreateToggleButton(
       (kTrayToggleButtonWidth - toggle_size.width()) / 2;
   toggle->SetBorder(views::CreateEmptyBorder(
       gfx::Insets(vertical_padding, horizontal_padding)));
-  // TODO(tdanderson): Update the focus rect color, border thickness, and
-  // location for material design.
-  toggle->SetFocusPainter(views::Painter::CreateSolidFocusPainter(
-      kFocusBorderColor, gfx::Insets(1)));
+  // TODO(varkha): remove this and implement toggle-button specific focus. See
+  // crbug.com/669124
+  toggle->SetFocusPainter(CreateFocusPainter());
   toggle->SetAccessibleName(l10n_util::GetStringUTF16(accessible_name_id));
   return toggle;
 }
 
+std::unique_ptr<views::Painter> TrayPopupUtils::CreateFocusPainter() {
+  return views::Painter::CreateSolidFocusPainter(
+      kFocusBorderColor, kFocusBorderThickness, gfx::InsetsF());
+}
+
 void TrayPopupUtils::ConfigureAsStickyHeader(views::View* view) {
   view->set_id(VIEW_ID_STICKY_HEADER);
   view->set_background(

[Evan Stade, 2016-11-30 03:44:19]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-11-30 03:44:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2527513002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-30 03:45:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 05:46:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 05:46:07]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Evan Stade, 2016-11-30 16:26:42]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-11-30 16:27:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 17:23:57]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480523202143580,
"parent_rev": "b00c66db69ce5c778ee2b29d0a0166180f8699d4", "commit_rev":
"97f0b715d05fe18cdcb15a12a1bc4fe6b76a271d"}

[commit-bot: I haz the power, 2016-11-30 17:24:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-30 17:28:49]

Description was changed from

==========
Update ash shelf/tray focus rects.

Also improve Canvas::DrawSolidFocusRect and
Painter::CreateSolidFocusPainter.

BUG=658783,652478,615892
==========

to

==========
Update ash shelf/tray focus rects.

Also improve Canvas::DrawSolidFocusRect and
Painter::CreateSolidFocusPainter.

BUG=658783,652478,615892

Committed: https://crrev.com/e69c1ffdb22164631d597a6b44c83ecd515d5281
Cr-Commit-Position: refs/heads/master@{#435309}
==========

[commit-bot: I haz the power, 2016-11-30 17:28:50]

Patchset 5 (id:??) landed as
https://crrev.com/e69c1ffdb22164631d597a6b44c83ecd515d5281
Cr-Commit-Position: refs/heads/master@{#435309}