cros: Add an icon to separate bubble views. KSV - part 6.

This cl adds a '+' icon between the bubble views of modifiers and key in
the Keyboard Shortcut Viewer (KSV).

Changes:
1. ui::VKEY_UNKNOWN is used as a separator indicator and will be shown
as a highlighted "+" icon.
2. Insert the ui::VKEY_UNKNOWN into |shortcut_key_codes| as the separator.
3. Replace the separator with '+' icon when creating
KeyboardShortcutItemView.

Bug: 768932
Test: Manually test that the separator icon is correctly created.
Change-Id: I2d01beb3f4c4b69a18cbb4820afc024f92679dc3
Reviewed-on: https://chromium-review.googlesource.com/912135Reviewed-by: Mitsuru Oshima (In Tokyo) <oshima@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536949}

chrome/browser/ui/ash/ksv/keyboard_shortcut_viewer_metadata_unittest.cc

@@ -97,17 +97,6 @@ TEST_F(KeyboardShortcutViewerMetadataTest, CheckAcceleratorIdsNoDuplicates) {
   }
 }
 
-// Test that |shortcut_key_codes| and |accelerator_ids| cannot be both empty.
-TEST_F(KeyboardShortcutViewerMetadataTest,
-       CheckShortcutKeyCodesAndAcceleratorIdsNotBothEmpty) {
-  for (const auto& shortcut_item :
-       keyboard_shortcut_viewer::GetKeyboardShortcutItemList()) {
-    EXPECT_TRUE(!shortcut_item.shortcut_key_codes.empty() ||
-                !shortcut_item.accelerator_ids.empty())
-        << "shortcut_key_codes and accelerator_ids cannot be both empty.";
-  }
-}
-
 // Test that metadata with empty |shortcut_key_codes| and grouped AcceleratorIds
 // should have the same modifiers.
 TEST_F(KeyboardShortcutViewerMetadataTest,

ui/chromeos/keyboard_shortcut_viewer_strings.grdp

@@ -52,7 +52,7 @@
 
   <!-- Generic shortcut strings -->
   <message name="IDS_KSV_SHORTCUT_ONE_MODIFIER_ONE_KEY" desc="Human readable version of the keyboard shortcut. This is for all single modifier shortcuts.">
-    <ph name="modifier">$1<ex>CTRL</ex></ph> + <ph name="key">$2<ex>V</ex></ph>
+    <ph name="modifier">$1<ex>Ctrl</ex></ph><ph name="separator">$2<ex>+</ex></ph><ph name="key">$3<ex>V</ex></ph>
   </message>
 
   <message name="IDS_KSV_DESCRIPTION_LOCK_SCREEN" desc="Description of the command in keyboard shortcut viewer.">
@@ -63,6 +63,21 @@
     Change screen resolution
   </message>
   <message name="IDS_KSV_SHORTCUT_CHANGE_SCREEN_RESOLUTION" desc="Human readable version of the keyboard shortcut.">
-    <ph name="ctrl">$1</ph> + <ph name="shift">$2</ph> and + or -
+    <ph name="ctrl">$1</ph><ph name="separator">$2</ph><ph name="shift">$3</ph> and + or -
   </message>
+
+  <message name="IDS_KSV_DESCRIPTION_DRAG_LINK_IN_SAME_TAB" desc="Description of the command in keyboard shortcut viewer.">
+    Open the link in the tab
+  </message>
+  <message name="IDS_KSV_SHORTCUT_DRAG_LINK_IN_SAME_TAB" desc="Human readable version of the keyboard shortcut.">
+    Drag the link to the tab's address bar
+  </message>
+
+  <message name="IDS_KSV_DESCRIPTION_HIGHLIGHT_NEXT_ITEM" desc="Description of the command in keyboard shortcut viewer.">
+    Highlight the next item on your shelf
+  </message>
+  <message name="IDS_KSV_SHORTCUT_HIGHLIGHT_NEXT_ITEM" desc="Human readable version of the keyboard shortcut.">
+    <ph name="shift">$1</ph><ph name="separator1">$2</ph><ph name="alt">$3</ph><ph name="separator2">$4</ph><ph name="l">$5</ph>, then <ph name="tab">$6</ph> or <ph name="right">$7</ph>
+  </message>
+
 </grit-part>

ui/chromeos/ksv/keyboard_shortcut_item.cc

@@ -27,7 +27,6 @@ KeyboardShortcutItem::KeyboardShortcutItem(
       accelerator_ids(accelerator_ids),
       shortcut_key_codes(shortcut_key_codes) {
   DCHECK(!categories.empty());
-  DCHECK(!accelerator_ids.empty() || !shortcut_key_codes.empty());
 }
 
 KeyboardShortcutItem::KeyboardShortcutItem(const KeyboardShortcutItem& other) =

ui/chromeos/ksv/keyboard_shortcut_item.h

@@ -57,7 +57,7 @@ struct KSV_EXPORT KeyboardShortcutItem {
       const std::vector<ShortcutCategory>& categories,
       int description_message_id,
       int shortcut_message_id,
-      const std::vector<AcceleratorId>& accelerator_ids,
+      const std::vector<AcceleratorId>& accelerator_ids = {},
       const std::vector<ui::KeyboardCode>& shortcut_key_codes = {});
   explicit KeyboardShortcutItem(const KeyboardShortcutItem& other);
   ~KeyboardShortcutItem();

ui/chromeos/ksv/keyboard_shortcut_viewer_metadata.cc

@@ -62,6 +62,12 @@ base::Optional<base::string16> GetSpecialStringForKeyboardCode(
       msg_id = ui::DeviceUsesKeyboardLayout2() ? IDS_KSV_MODIFIER_LAUNCHER
                                                : IDS_KSV_MODIFIER_SEARCH;
       break;
+    case ui::VKEY_UNKNOWN:
+      // If this is VKEY_UNKNOWN, it indicates to insert a "+" separator. Use
+      // one space to replace the string resourece's placeholder so that the
+      // separator is not searchable. This does not conflict with the
+      // replacement string for "VKEY_SPACE", which is "Space".
+      return base::ASCIIToUTF16(" ");
     default:
       return base::nullopt;
   }
@@ -124,24 +130,43 @@ const std::vector<KeyboardShortcutItem>& GetKeyboardShortcutItemList() {
       std::vector<KeyboardShortcutItem>, item_list,
       // TODO(crbug.com/793997): These are two examples how we organize the
       // metadata. Full metadata will be provided.
-      ({{// |categories|
-         {ShortcutCategory::kPopular, ShortcutCategory::kSystemAndDisplay},
-         IDS_KSV_DESCRIPTION_LOCK_SCREEN,
-         IDS_KSV_SHORTCUT_ONE_MODIFIER_ONE_KEY,
-         // |accelerator_ids|
-         {{ui::VKEY_L, ui::EF_COMMAND_DOWN}}},
-
-        // Some accelerators are grouped into one metadata for the
-        // KeyboardShortcutViewer due to the similarity. E.g. the shortcut for
-        // "Change screen resolution" is "Ctrl + Shift and + or -", which groups
-        // two accelerators.
-        {// |categories|
-         {ShortcutCategory::kSystemAndDisplay},
-         IDS_KSV_DESCRIPTION_CHANGE_SCREEN_RESOLUTION,
-         IDS_KSV_SHORTCUT_CHANGE_SCREEN_RESOLUTION,
-         // |accelerator_ids|
-         {{ui::VKEY_OEM_MINUS, ui::EF_CONTROL_DOWN | ui::EF_SHIFT_DOWN},
-          {ui::VKEY_OEM_PLUS, ui::EF_CONTROL_DOWN | ui::EF_SHIFT_DOWN}}}}));
+      ({
+          {// |categories|
+           {ShortcutCategory::kPopular, ShortcutCategory::kSystemAndDisplay},
+           IDS_KSV_DESCRIPTION_LOCK_SCREEN,
+           IDS_KSV_SHORTCUT_ONE_MODIFIER_ONE_KEY,
+           // |accelerator_ids|
+           {{ui::VKEY_L, ui::EF_COMMAND_DOWN}}},
+
+          // Some accelerators are grouped into one metadata for the
+          // KeyboardShortcutViewer due to the similarity. E.g. the shortcut for
+          // "Change screen resolution" is "Ctrl + Shift and + or -", which
+          // groups
+          // two accelerators.
+          {// |categories|
+           {ShortcutCategory::kSystemAndDisplay},
+           IDS_KSV_DESCRIPTION_CHANGE_SCREEN_RESOLUTION,
+           IDS_KSV_SHORTCUT_CHANGE_SCREEN_RESOLUTION,
+           // |accelerator_ids|
+           {{ui::VKEY_OEM_MINUS, ui::EF_CONTROL_DOWN | ui::EF_SHIFT_DOWN},
+            {ui::VKEY_OEM_PLUS, ui::EF_CONTROL_DOWN | ui::EF_SHIFT_DOWN}}},
+
+          {// |categories|
+           {ShortcutCategory::kTabAndWindow},
+           IDS_KSV_DESCRIPTION_DRAG_LINK_IN_SAME_TAB,
+           IDS_KSV_SHORTCUT_DRAG_LINK_IN_SAME_TAB},
+
+          {// |categories|
+           {ShortcutCategory::kAccessibility},
+           IDS_KSV_DESCRIPTION_HIGHLIGHT_NEXT_ITEM,
+           IDS_KSV_SHORTCUT_HIGHLIGHT_NEXT_ITEM,
+           // |accelerator_ids|
+           {},
+           // |shortcut_key_codes|
+           {ui::VKEY_SHIFT, ui::VKEY_UNKNOWN, ui::VKEY_LMENU, ui::VKEY_UNKNOWN,
+            ui::VKEY_L, ui::VKEY_TAB, ui::VKEY_RIGHT}},
+
+      }));
 
   CR_DEFINE_STATIC_LOCAL(bool, is_initialized, (false));
   // If the item's |shortcut_key_codes| is empty, we need to dynamically
@@ -150,8 +175,7 @@ const std::vector<KeyboardShortcutItem>& GetKeyboardShortcutItemList() {
   if (!is_initialized) {
     is_initialized = true;
     for (auto& item : item_list) {
-      if (item.shortcut_key_codes.empty()) {
-        DCHECK(!item.accelerator_ids.empty());
+      if (item.shortcut_key_codes.empty() && !item.accelerator_ids.empty()) {
         // Only use the first |accelerator_id| because the modifiers are the
         // same even if it is a grouped accelerators.
         const AcceleratorId& accelerator_id = item.accelerator_ids[0];
@@ -161,13 +185,21 @@ const std::vector<KeyboardShortcutItem>& GetKeyboardShortcutItemList() {
         for (auto modifier : {ui::EF_CONTROL_DOWN, ui::EF_ALT_DOWN,
                               ui::EF_SHIFT_DOWN, ui::EF_COMMAND_DOWN}) {
           if (accelerator_id.modifiers & modifier) {
+            // ui::VKEY_UNKNOWN is used as a separator and will be shown as a
+            // highlighted "+" sign between the bubble views and the rest of the
+            // text.
+            if (!item.shortcut_key_codes.empty())
+              item.shortcut_key_codes.emplace_back(ui::VKEY_UNKNOWN);
             item.shortcut_key_codes.emplace_back(
                 GetKeyCodeForModifier(modifier));
           }
         }
         // For non grouped accelerators, we need to populate the key as well.
-        if (item.accelerator_ids.size() == 1)
+        if (item.accelerator_ids.size() == 1) {
+          if (!item.shortcut_key_codes.empty())
+            item.shortcut_key_codes.emplace_back(ui::VKEY_UNKNOWN);
           item.shortcut_key_codes.emplace_back(accelerator_id.keycode);
+        }
       }
     }
   }

ui/chromeos/ksv/vector_icons/BUILD.gn

@@ -18,6 +18,8 @@ aggregate_vector_icons("ksv_vector_icons") {
     "ksv_search_no_result.icon",
     "ksv_search_start.1x.icon",
     "ksv_search_start.icon",
+    "ksv_separator_plus.1x.icon",
+    "ksv_separator_plus.icon",
   ]
 }
 

ui/chromeos/ksv/vector_icons/ksv_separator_plus.1x.icon

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+CANVAS_DIMENSIONS, 27,
+MOVE_TO, 18, 14,
+R_H_LINE_TO, -3,
+R_V_LINE_TO, 3,
+R_H_LINE_TO, -2,
+R_V_LINE_TO, -3,
+R_H_LINE_TO, -3,
+R_V_LINE_TO, -2,
+R_H_LINE_TO, 3,
+V_LINE_TO, 9,
+R_H_LINE_TO, 2,
+R_V_LINE_TO, 3,
+R_H_LINE_TO, 3,
+CLOSE,
+END

ui/chromeos/ksv/vector_icons/ksv_separator_plus.icon

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+CANVAS_DIMENSIONS, 54,
+MOVE_TO, 35, 29,
+R_H_LINE_TO, -6,
+R_V_LINE_TO, 6,
+R_H_LINE_TO, -4,
+R_V_LINE_TO, -6,
+R_H_LINE_TO, -6,
+R_V_LINE_TO, -4,
+R_H_LINE_TO, 6,
+R_V_LINE_TO, -6,
+R_H_LINE_TO, 4,
+R_V_LINE_TO, 6,
+R_H_LINE_TO, 6,
+CLOSE,
+END

ui/chromeos/ksv/views/keyboard_shortcut_item_view.cc

@@ -8,11 +8,15 @@
 #include <vector>
 
 #include "base/i18n/rtl.h"
+#include "base/strings/utf_string_conversions.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/chromeos/ksv/keyboard_shortcut_item.h"
 #include "ui/chromeos/ksv/keyboard_shortcut_viewer_metadata.h"
+#include "ui/chromeos/ksv/vector_icons/vector_icons.h"
 #include "ui/chromeos/ksv/views/bubble_view.h"
+#include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/border.h"
+#include "ui/views/controls/image_view.h"
 #include "ui/views/controls/styled_label.h"
 
 namespace keyboard_shortcut_viewer {
@@ -23,6 +27,31 @@ namespace {
 // TODO(wutao): to be decided by UX specs.
 constexpr float kShortcutViewWitdhRatio = 0.618f;
 
+// Creates the separator view between bubble views of modifiers and key.
+std::unique_ptr<views::View> CreateSeparatorView() {
+  constexpr SkColor kSeparatorColor =
+      SkColorSetARGBMacro(0xFF, 0x1A, 0x73, 0xE8);
+  constexpr int kIconSize = 27;
+
+  std::unique_ptr<views::ImageView> separator_view =
+      std::make_unique<views::ImageView>();
+  separator_view->SetImage(
+      gfx::CreateVectorIcon(kKsvSeparatorPlusIcon, kSeparatorColor));
+  separator_view->SetImageSize(gfx::Size(kIconSize, kIconSize));
+  separator_view->set_owned_by_client();
+  return separator_view;
+}
+
+// Creates the bubble view for modifiers and key.
+std::unique_ptr<views::View> CreateBubbleView(
+    const base::string16& bubble_text) {
+  auto bubble_view = std::make_unique<BubbleView>();
+  bubble_view->set_owned_by_client();
+  // TODO(wutao): add icons for keys.
+  bubble_view->SetText(bubble_text);
+  return bubble_view;
+}
+
 }  // namespace
 
 KeyboardShortcutItemView::KeyboardShortcutItemView(
@@ -40,6 +69,9 @@ KeyboardShortcutItemView::KeyboardShortcutItemView(
 
   std::vector<size_t> offsets;
   std::vector<base::string16> replacement_strings;
+  const size_t shortcut_key_codes_size = item.shortcut_key_codes.size();
+  offsets.reserve(shortcut_key_codes_size);
+  replacement_strings.reserve(shortcut_key_codes_size);
   for (ui::KeyboardCode key_code : item.shortcut_key_codes)
     replacement_strings.emplace_back(GetStringForKeyboardCode(key_code));
 
@@ -57,17 +89,18 @@ KeyboardShortcutItemView::KeyboardShortcutItemView(
   shortcut_label_view_->SetHorizontalAlignment(
       base::i18n::IsRTL() ? gfx::ALIGN_LEFT : gfx::ALIGN_RIGHT);
   DCHECK_EQ(replacement_strings.size(), offsets.size());
+  // If the replacement string is " ", it indicates to insert a seperator view.
+  const base::string16 separator_string = base::ASCIIToUTF16(" ");
   for (size_t i = 0; i < offsets.size(); ++i) {
     views::StyledLabel::RangeStyleInfo style_info;
-    // TODO(wutao): add icons for keys.
-    // TODO(wutao): finalize the sytles with UX specs.
-    style_info.override_color = SK_ColorBLUE;
     style_info.disable_line_wrapping = true;
-    auto bubble_view = std::make_unique<BubbleView>();
-    bubble_view->set_owned_by_client();
-    bubble_view->SetText(replacement_strings[i]);
-    style_info.custom_view = bubble_view.get();
-    shortcut_label_view_->AddCustomView(std::move(bubble_view));
+    const base::string16& replacement_string = replacement_strings[i];
+    std::unique_ptr<views::View> custom_view =
+        replacement_string == separator_string
+            ? CreateSeparatorView()
+            : CreateBubbleView(replacement_string);
+    style_info.custom_view = custom_view.get();
+    shortcut_label_view_->AddCustomView(std::move(custom_view));
     shortcut_label_view_->AddStyleRange(
         gfx::Range(offsets[i], offsets[i] + replacement_strings[i].length()),
         style_info);

ui/chromeos/ksv/views/keyboard_shortcut_view.cc

@@ -146,7 +146,8 @@ void KeyboardShortcutView::InitViews() {
 
   // Init views of TabbedPane and KeyboardShortcutItemListView.
   tabbed_pane_ =
-      new views::TabbedPane(views::TabbedPane::Orientation::kVertical);
+      new views::TabbedPane(views::TabbedPane::Orientation::kVertical,
+                            views::TabbedPane::TabStripStyle::kHighlight);
   ShortcutCategory current_category = ShortcutCategory::kUnknown;
   KeyboardShortcutItemListView* item_list_view;
   for (auto* item_view : shortcut_views_) {