ksv: Fix bold text alignment

The StyledLabel calls two different ctors of Label based on the styled
range, one with |custom_font| and the other one with |text_style|. The
two ctors will create labels with different heights, which causes a
misalignment between normal and bold texts.

This cl adds an ash typography to support semi bold text style to
emphasize a matched search string. Then the label in the styled range
will call a same ctor of Label with |text_style| to eliminate the
difference.

Bug: 1061271
Test: New unittest
Change-Id: I5b7415ef3b7bd06c5ad296fe9e5211ddd18685ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2108061
Commit-Queue: Tao Wu <wutao@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751222}

ash/public/cpp/ash_typography.cc

@@ -24,6 +24,12 @@ void ApplyAshFontStyles(int context,
       *size_delta = 15;
       break;
   }
+
+  switch (style) {
+    case STYLE_EMPHASIZED:
+      *font_weight = gfx::Font::Weight::SEMIBOLD;
+      break;
+  }
 }
 
 }  // namespace ash

ash/public/cpp/ash_typography.h

@@ -29,6 +29,16 @@ enum AshTextContext {
   ASH_TEXT_CONTEXT_END
 };
 
+enum AshTextStyle {
+  ASH_TEXT_STYLE_START = views::style::VIEWS_TEXT_STYLE_END,
+
+  // Used to draw attention to a section of body text such as a matched search
+  // string.
+  STYLE_EMPHASIZED = ASH_TEXT_STYLE_START,
+
+  ASH_TEXT_STYLE_END
+};
+
 // Sets the |size_delta| and |font_weight| for ash-specific text contexts.
 // Values are only set for contexts specific to ash.
 void ASH_PUBLIC_EXPORT ApplyAshFontStyles(int context,

ash/shortcut_viewer/views/keyboard_shortcut_view.cc

@@ -12,6 +12,7 @@
 #include "ash/display/privacy_screen_controller.h"
 #include "ash/public/cpp/app_list/internal_app_id_constants.h"
 #include "ash/public/cpp/app_types.h"
+#include "ash/public/cpp/ash_typography.h"
 #include "ash/public/cpp/resources/grit/ash_public_unscaled_resources.h"
 #include "ash/public/cpp/shelf_item.h"
 #include "ash/public/cpp/window_properties.h"
@@ -516,7 +517,8 @@ void KeyboardShortcutView::ShowSearchResults(
       search_query);
   ShortcutCategory current_category = ShortcutCategory::kUnknown;
   bool has_category_item = false;
-  std::vector<KeyboardShortcutItemView*> shortcut_items;
+  found_shortcut_items_.clear();
+
   for (const auto& item_view : shortcut_views_) {
     base::string16 description_text =
         item_view->description_label_view()->GetText();
@@ -547,8 +549,7 @@ void KeyboardShortcutView::ShowSearchResults(
             item_view->description_label_view();
         // Clear previous styles.
         description_label_view->ClearStyleRanges();
-        style.custom_font = description_label_view->GetDefaultFontList().Derive(
-            0, gfx::Font::FontStyle::NORMAL, gfx::Font::Weight::BOLD);
+        style.text_style = ash::AshTextStyle::STYLE_EMPHASIZED;
         description_label_view->AddStyleRange(
             gfx::Range(match_index, match_index + match_length), style);
         // Apply new styles to highlight matched search query.
@@ -556,14 +557,14 @@ void KeyboardShortcutView::ShowSearchResults(
       }
 
       found_items_list_view->AddChildView(item_view.get());
-      shortcut_items.emplace_back(item_view.get());
+      found_shortcut_items_.emplace_back(item_view.get());
     }
   }
 
   std::vector<base::string16> replacement_strings;
-  const int number_search_results = shortcut_items.size();
+  const int number_search_results = found_shortcut_items_.size();
   if (!found_items_list_view->children().empty()) {
-    UpdateAXNodeDataPosition(shortcut_items);
+    UpdateAXNodeDataPosition(found_shortcut_items_);
     replacement_strings.emplace_back(
         base::NumberToString16(number_search_results));
 
@@ -624,6 +625,11 @@ KSVSearchBoxView* KeyboardShortcutView::GetSearchBoxViewForTesting() {
   return search_box_view_.get();
 }
 
+const std::vector<KeyboardShortcutItemView*>&
+KeyboardShortcutView::GetFoundShortcutItemsForTesting() const {
+  return found_shortcut_items_;
+}
+
 }  // namespace keyboard_shortcut_viewer
 
 namespace ash {

ash/shortcut_viewer/views/keyboard_shortcut_view.h

@@ -91,6 +91,8 @@ class KeyboardShortcutView : public views::WidgetDelegateView,
   const std::vector<std::unique_ptr<KeyboardShortcutItemView>>&
   GetShortcutViewsForTesting() const;
   KSVSearchBoxView* GetSearchBoxViewForTesting();
+  const std::vector<KeyboardShortcutItemView*>&
+  GetFoundShortcutItemsForTesting() const;
 
   // Owned by views hierarchy.
   // The container for category tabs and lists of KeyboardShortcutItemViews.
@@ -107,6 +109,9 @@ class KeyboardShortcutView : public views::WidgetDelegateView,
   // KeyboardShortcutItemView layout when switching between tabs and search.
   std::vector<std::unique_ptr<KeyboardShortcutItemView>> shortcut_views_;
 
+  // Contains all the found shortcut items.
+  std::vector<KeyboardShortcutItemView*> found_shortcut_items_;
+
   // An illustration to indicate no search results found. Since this view need
   // to be added and removed frequently from the |search_results_container_|, it
   // is not owned by view hierarchy to avoid recreating it.

ash/shortcut_viewer/views/keyboard_shortcut_view_unittest.cc

@@ -12,6 +12,7 @@
 #include "ash/test/ash_test_base.h"
 #include "base/bind.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/aura/window.h"
 #include "ui/compositor/test/test_utils.h"
@@ -26,7 +27,8 @@ namespace keyboard_shortcut_viewer {
 
 class KeyboardShortcutViewTest : public ash::AshTestBase {
  public:
-  KeyboardShortcutViewTest() = default;
+  KeyboardShortcutViewTest()
+      : ash::AshTestBase(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
   ~KeyboardShortcutViewTest() override = default;
 
   views::Widget* Toggle() { return KeyboardShortcutView::Toggle(GetContext()); }
@@ -45,7 +47,7 @@ class KeyboardShortcutViewTest : public ash::AshTestBase {
   }
 
   const std::vector<std::unique_ptr<KeyboardShortcutItemView>>&
-  GetShortcutViews() {
+  GetShortcutViews() const {
     DCHECK(GetView());
     return GetView()->GetShortcutViewsForTesting();
   }
@@ -55,6 +57,11 @@ class KeyboardShortcutViewTest : public ash::AshTestBase {
     return GetView()->GetSearchBoxViewForTesting();
   }
 
+  const std::vector<KeyboardShortcutItemView*>& GetFoundShortcutItems() const {
+    DCHECK(GetView());
+    return GetView()->GetFoundShortcutItemsForTesting();
+  }
+
   void KeyPress(ui::KeyboardCode key_code, bool should_insert) {
     ui::KeyEvent event(ui::ET_KEY_PRESSED, key_code, ui::EF_NONE);
     GetSearchBoxView()->OnKeyEvent(&event);
@@ -232,4 +239,44 @@ TEST_F(KeyboardShortcutViewTest, ToggleWindow) {
   EXPECT_TRUE(widget->IsClosed());
 }
 
+// Test that the sub-labels of the |description_label_view| in the search
+// results page have the same height and are vertically aligned.
+TEST_F(KeyboardShortcutViewTest, ShouldAlignSubLabelsInSearchResults) {
+  // Show the widget.
+  Toggle();
+
+  EXPECT_TRUE(GetFoundShortcutItems().empty());
+  // Type a letter and show the search results.
+  KeyPress(ui::VKEY_A, /*should_insert=*/true);
+  auto time_out = base::TimeDelta::FromMilliseconds(300);
+  task_environment_->FastForwardBy(time_out);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(GetFoundShortcutItems().empty());
+
+  for (const auto* item_view : GetFoundShortcutItems()) {
+    ASSERT_EQ(2u, item_view->children().size());
+
+    const views::View* description = item_view->children()[0];
+    // Skip if it only has one label.
+    if (description->children().size() == 1)
+      continue;
+
+    // The sub-labels in |description_label_view_| have the same height and are
+    // vertically aligned in each line.
+    int height = 0;
+    int center_y = 0;
+    for (const auto* child : description->children()) {
+      // The first view in each line.
+      if (child->bounds().x() == 0) {
+        height = child->bounds().height();
+        center_y = child->bounds().CenterPoint().y();
+        continue;
+      }
+
+      EXPECT_EQ(height, child->GetPreferredSize().height());
+      EXPECT_EQ(center_y, child->bounds().CenterPoint().y());
+    }
+  }
+}
+
 }  // namespace keyboard_shortcut_viewer