[Chrome OS MD] Implement accessibility detailed view for the MD Ash system menu

ash/common/system/tray_accessibility.cc

Index: ash/common/system/tray_accessibility.cc
diff --git a/ash/common/system/tray_accessibility.cc b/ash/common/system/tray_accessibility.cc
index 273a93937a0aff907e732b51e109e1b83d867279..436d3bf9b44896c1357ecc2eaac09700c9586d05 100644
--- a/ash/common/system/tray_accessibility.cc
+++ b/ash/common/system/tray_accessibility.cc
@@ -23,8 +23,11 @@
 #include "grit/ash_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/color_palette.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/paint_vector_icon.h"
+#include "ui/gfx/vector_icon_types.h"

[tdanderson, 2016/09/19 23:32:27]

I don't think you need this #include (#including paint_vector_icon should be
sufficient?)

[yiyix, 2016/09/21 17:10:48]

you are right!

+#include "ui/gfx/vector_icons_public.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/box_layout.h"
@@ -151,7 +154,10 @@ AccessibilityDetailedView::AccessibilityDetailedView(SystemTrayItem* owner,
   Reset();
 
   AppendAccessibilityList();
-  AppendHelpEntries();
+
+  if (!ash::MaterialDesignController::IsSystemTrayMenuMaterial())

[tdanderson, 2016/09/19 23:32:27]

nit: ash:: not needed (here and elsewhere in this CL). Originally I suggested
adding ash:: before MaterialDesignController to avoid confusing it with the top
chrome MDC, but that is in the process of being killed off anyway.

[yiyix, 2016/09/21 17:10:48]

I know MaterialDesignController exists in UI space and ash space. I thought it
is a good hobbit of adding it, so it can be differentiated from the UI one. Does
it mean that the ui one is enabled by default?

[tdanderson, 2016/09/21 17:59:32]

As discussed in person, top-chrome MDC calls are in the process of being
removed, and in retrospect telling the difference between the two was probably
only an issue for the handful of people who worked on both projects.

+    AppendHelpEntries();
+
   CreateTitleRow(IDS_ASH_STATUS_TRAY_ACCESSIBILITY_TITLE);
 
   Layout();
@@ -161,12 +167,15 @@ void AccessibilityDetailedView::AppendAccessibilityList() {
   CreateScrollableList();
   ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
 
+  // Generate entries in Accessibility detailed view menu for MD and non-MD,
+  // respectively.
   AccessibilityDelegate* delegate = WmShell::Get()->accessibility_delegate();
   spoken_feedback_enabled_ = delegate->IsSpokenFeedbackEnabled();
   spoken_feedback_view_ =
       AddScrollListItem(bundle.GetLocalizedString(
                             IDS_ASH_STATUS_TRAY_ACCESSIBILITY_SPOKEN_FEEDBACK),
-                        spoken_feedback_enabled_, spoken_feedback_enabled_);
+                        spoken_feedback_enabled_, spoken_feedback_enabled_,
+                        kSystemMenuAccessibilityChromevoxIcon);
 
   // Large Cursor item is shown only in Login screen.
   if (login_ == LoginStatus::NOT_LOGGED_IN) {
@@ -174,33 +183,38 @@ void AccessibilityDetailedView::AppendAccessibilityList() {
     large_cursor_view_ =
         AddScrollListItem(bundle.GetLocalizedString(
                               IDS_ASH_STATUS_TRAY_ACCESSIBILITY_LARGE_CURSOR),
-                          large_cursor_enabled_, large_cursor_enabled_);
+                          large_cursor_enabled_, large_cursor_enabled_,
+                          kSystemMenuAccessibilityLargeCursorIcon);
   }
 
   high_contrast_enabled_ = delegate->IsHighContrastEnabled();
   high_contrast_view_ = AddScrollListItem(
       bundle.GetLocalizedString(
           IDS_ASH_STATUS_TRAY_ACCESSIBILITY_HIGH_CONTRAST_MODE),
-      high_contrast_enabled_, high_contrast_enabled_);
+      high_contrast_enabled_, high_contrast_enabled_,
+      kSystemMenuAccessibilityContrastIcon);
   screen_magnifier_enabled_ = delegate->IsMagnifierEnabled();
   screen_magnifier_view_ =
       AddScrollListItem(bundle.GetLocalizedString(
                             IDS_ASH_STATUS_TRAY_ACCESSIBILITY_SCREEN_MAGNIFIER),
-                        screen_magnifier_enabled_, screen_magnifier_enabled_);
+                        screen_magnifier_enabled_, screen_magnifier_enabled_,
+                        kSystemMenuAccessibilityScreenMagnifierIcon);
 
   // Don't show autoclick option at login screen.
   if (login_ != LoginStatus::NOT_LOGGED_IN) {
     autoclick_enabled_ = delegate->IsAutoclickEnabled();
     autoclick_view_ = AddScrollListItem(
         bundle.GetLocalizedString(IDS_ASH_STATUS_TRAY_ACCESSIBILITY_AUTOCLICK),
-        autoclick_enabled_, autoclick_enabled_);
+        autoclick_enabled_, autoclick_enabled_,
+        kSystemMenuAccessibilityAutoClickIcon);
   }
 
   virtual_keyboard_enabled_ = delegate->IsVirtualKeyboardEnabled();
   virtual_keyboard_view_ =
       AddScrollListItem(bundle.GetLocalizedString(
                             IDS_ASH_STATUS_TRAY_ACCESSIBILITY_VIRTUAL_KEYBOARD),
-                        virtual_keyboard_enabled_, virtual_keyboard_enabled_);
+                        virtual_keyboard_enabled_, virtual_keyboard_enabled_,
+                        kSystemMenuKeyboardIcon);
 }
 
 void AccessibilityDetailedView::AppendHelpEntries() {
@@ -237,60 +251,76 @@ void AccessibilityDetailedView::AppendHelpEntries() {
 HoverHighlightView* AccessibilityDetailedView::AddScrollListItem(
     const base::string16& text,
     bool highlight,
-    bool checked) {
+    bool checked,
+    const gfx::VectorIcon& icon) {
   HoverHighlightView* container = new HoverHighlightView(this);
-  container->AddCheckableLabel(text, highlight, checked);
+  if (ash::MaterialDesignController::IsSystemTrayMenuMaterial()) {
+    gfx::ImageSkia image = CreateVectorIcon(icon, kMenuIconColor);
+    const int padding = (kMenuButtonSize - image.width()) / 2;
+    container->AddIconAndLabelCustomSize(image, text, highlight,
+                                         image.width() + kMenuIconMargin * 2,
+                                         padding, padding);
+    gfx::ImageSkia check_mark =
+        CreateVectorIcon(gfx::VectorIconId::CHECK_CIRCLE, gfx::kGoogleGreen700);
+    container->AddRightIcon(check_mark, check_mark.width());
+    container->SetRightIconVisible(checked);
+  } else {
+    container->AddCheckableLabel(text, highlight, checked);
+  }
   scroll_content()->AddChildView(container);
   return container;
 }
 
 void AccessibilityDetailedView::HandleViewClicked(views::View* view) {
   AccessibilityDelegate* delegate = WmShell::Get()->accessibility_delegate();
+  UserMetricsAction user_action;

[tdanderson, 2016/09/19 23:32:27]

Nice.

[yiyix, 2016/09/21 17:10:48]

Thanks :)

   if (view == spoken_feedback_view_) {
-    WmShell::Get()->RecordUserMetricsAction(
-        delegate->IsSpokenFeedbackEnabled()
-            ? ash::UMA_STATUS_AREA_DISABLE_SPOKEN_FEEDBACK
-            : ash::UMA_STATUS_AREA_ENABLE_SPOKEN_FEEDBACK);
+    user_action = delegate->IsSpokenFeedbackEnabled()
+                      ? ash::UMA_STATUS_AREA_DISABLE_SPOKEN_FEEDBACK
+                      : ash::UMA_STATUS_AREA_ENABLE_SPOKEN_FEEDBACK;
     delegate->ToggleSpokenFeedback(A11Y_NOTIFICATION_NONE);
   } else if (view == high_contrast_view_) {
-    WmShell::Get()->RecordUserMetricsAction(
-        delegate->IsHighContrastEnabled()
-            ? ash::UMA_STATUS_AREA_DISABLE_HIGH_CONTRAST
-            : ash::UMA_STATUS_AREA_ENABLE_HIGH_CONTRAST);
+    user_action = delegate->IsHighContrastEnabled()
+                      ? ash::UMA_STATUS_AREA_DISABLE_HIGH_CONTRAST
+                      : ash::UMA_STATUS_AREA_ENABLE_HIGH_CONTRAST;
     delegate->ToggleHighContrast();
   } else if (view == screen_magnifier_view_) {
-    WmShell::Get()->RecordUserMetricsAction(
-        delegate->IsMagnifierEnabled() ? ash::UMA_STATUS_AREA_DISABLE_MAGNIFIER
-                                       : ash::UMA_STATUS_AREA_ENABLE_MAGNIFIER);
+    user_action = delegate->IsMagnifierEnabled()
+                      ? ash::UMA_STATUS_AREA_DISABLE_MAGNIFIER
+                      : ash::UMA_STATUS_AREA_ENABLE_MAGNIFIER;
     delegate->SetMagnifierEnabled(!delegate->IsMagnifierEnabled());
   } else if (large_cursor_view_ && view == large_cursor_view_) {
-    WmShell::Get()->RecordUserMetricsAction(
-        delegate->IsLargeCursorEnabled()
-            ? ash::UMA_STATUS_AREA_DISABLE_LARGE_CURSOR
-            : ash::UMA_STATUS_AREA_ENABLE_LARGE_CURSOR);
+    user_action = delegate->IsLargeCursorEnabled()
+                      ? ash::UMA_STATUS_AREA_DISABLE_LARGE_CURSOR
+                      : ash::UMA_STATUS_AREA_ENABLE_LARGE_CURSOR;
     delegate->SetLargeCursorEnabled(!delegate->IsLargeCursorEnabled());
   } else if (autoclick_view_ && view == autoclick_view_) {
-    WmShell::Get()->RecordUserMetricsAction(
-        delegate->IsAutoclickEnabled()
-            ? ash::UMA_STATUS_AREA_DISABLE_AUTO_CLICK
-            : ash::UMA_STATUS_AREA_ENABLE_AUTO_CLICK);
+    user_action = delegate->IsAutoclickEnabled()
+                      ? ash::UMA_STATUS_AREA_DISABLE_AUTO_CLICK
+                      : ash::UMA_STATUS_AREA_ENABLE_AUTO_CLICK;
     delegate->SetAutoclickEnabled(!delegate->IsAutoclickEnabled());
   } else if (virtual_keyboard_view_ && view == virtual_keyboard_view_) {
-    WmShell::Get()->RecordUserMetricsAction(
-        delegate->IsVirtualKeyboardEnabled()
-            ? ash::UMA_STATUS_AREA_DISABLE_VIRTUAL_KEYBOARD
-            : ash::UMA_STATUS_AREA_ENABLE_VIRTUAL_KEYBOARD);
+    user_action = delegate->IsVirtualKeyboardEnabled()
+                      ? ash::UMA_STATUS_AREA_DISABLE_VIRTUAL_KEYBOARD
+                      : ash::UMA_STATUS_AREA_ENABLE_VIRTUAL_KEYBOARD;
     delegate->SetVirtualKeyboardEnabled(!delegate->IsVirtualKeyboardEnabled());
+  } else {
+    return;

[tdanderson, 2016/09/19 23:32:27]

An early return at the top of the function would be preferable.

   }
+  WmShell::Get()->RecordUserMetricsAction(user_action);
 }
 
 void AccessibilityDetailedView::HandleButtonPressed(views::Button* sender,
                                                     const ui::Event& event) {
-  SystemTrayDelegate* tray_delegate = WmShell::Get()->system_tray_delegate();
-  if (sender == help_view_)
-    tray_delegate->ShowAccessibilityHelp();
-  else if (sender == settings_view_)
-    tray_delegate->ShowAccessibilitySettings();
+  // Since this row is not initialized in MD, this function should be called for

[tdanderson, 2016/09/19 23:32:27]

I suggest removing this comment, since in the very near future you/someone will
be using this for the MD "info" button.

[yiyix, 2016/09/21 17:10:48]

Done.

+  // non-MD only.
+  if (!MaterialDesignController::UseMaterialDesignSystemIcons()) {

[tdanderson, 2016/09/19 23:32:27]

To avoid indenting the whole function, it would be better to use an early return
instead.

[yiyix, 2016/09/21 17:10:48]

Done.

+    SystemTrayDelegate* tray_delegate = WmShell::Get()->system_tray_delegate();
+    if (sender == help_view_)
+      tray_delegate->ShowAccessibilityHelp();
+    else if (sender == settings_view_)
+      tray_delegate->ShowAccessibilitySettings();
+  }
 }
 
 }  // namespace tray