Improve font/color handling for download items.

The first change here is to set the font and color in the typography
provider, letting DownloadItemView work in contexts and styles instead
of physical attributes.  This is not a complete conversion (yet) since
the existing FontList members are still present, but I'll remove those
in a future patch.  The biggest effect of this change is to make the
file name once again be 13 pt instead of 12 pt.  It also makes the
dangerous download and deep scanning labels use the same font as the
file name.

The second change is to lay out the text using full label heights
instead of baselines and (strange) single-dip offsets.  Using baselines
dates back to the original work on Material Design top chrome in 2015,
and I can't find a spec (though bugs from the time mention one).  This
was questioned by sky@ (the reviewer) when it landed, and it looks both
semantically questionable and hazardous-for-i18n to me.  As demonstrated
in the screenshots on the bug, using standard Harmony line heights and
putting the labels flush results in a very similar appearance to the old
behavior (which wouldn't be the case if we made the font change without
also making this change), except perhaps slightly less buggy.

Bug: 1095970
Change-Id: I53f9876d9b7bc5523adb9cd910a659a00717d130
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2249583Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779403}

chrome/browser/ui/views/chrome_typography.cc

@@ -77,9 +77,8 @@ void ApplyCommonFontStyles(int context,
               LocationBarView::GetAvailableTextHeight(),
               ui::TouchUiController::Get()->touch_ui() ? 15 : 14);
       *size_delta = omnibox_primary_delta;
-      if (context == CONTEXT_OMNIBOX_DEEMPHASIZED) {
-        (*size_delta)--;
-      }
+      if (context == CONTEXT_OMNIBOX_DEEMPHASIZED)
+        --*size_delta;
       break;
     }
     case CONTEXT_OMNIBOX_DECORATION: {

chrome/browser/ui/views/chrome_typography.h

@@ -52,6 +52,12 @@ enum ChromeTextContext {
 
   // ToolbarButton label
   CONTEXT_TOOLBAR_BUTTON,
+
+  // Most text in the download shelf.  Usually 13pt.
+  CONTEXT_DOWNLOAD_SHELF,
+
+  // Status labels in the download shelf.  Usually 10pt.
+  CONTEXT_DOWNLOAD_SHELF_STATUS,
 };
 
 enum ChromeTextStyle {

chrome/browser/ui/views/chrome_typography_provider.cc

@@ -4,8 +4,10 @@
 
 #include "chrome/browser/ui/views/chrome_typography_provider.h"
 
+#include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/ui/views/chrome_typography.h"
 #include "ui/base/resource/resource_bundle.h"
+#include "ui/base/theme_provider.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/platform_font.h"
@@ -31,6 +33,7 @@ const gfx::FontList& ChromeTypographyProvider::GetFont(int context,
   constexpr int kTouchableLabelSize = 14;
   constexpr int kBodyTextLargeSize = 13;
   constexpr int kDefaultSize = 12;
+  constexpr int kStatusSize = 10;
 
   std::string typeface;
   int size_delta = kDefaultSize - gfx::PlatformFont::kDefaultBaseFontSize;
@@ -53,14 +56,18 @@ const gfx::FontList& ChromeTypographyProvider::GetFont(int context,
       size_delta =
           kTouchableLabelSize - gfx::PlatformFont::kDefaultBaseFontSize;
       break;
+    case views::style::CONTEXT_MESSAGE_BOX_BODY_TEXT:
     case CONTEXT_BODY_TEXT_LARGE:
     case CONTEXT_TAB_HOVER_CARD_TITLE:
-    case views::style::CONTEXT_MESSAGE_BOX_BODY_TEXT:
+    case CONTEXT_DOWNLOAD_SHELF:
       size_delta = kBodyTextLargeSize - gfx::PlatformFont::kDefaultBaseFontSize;
       break;
     case CONTEXT_HEADLINE:
       size_delta = kHeadlineSize - gfx::PlatformFont::kDefaultBaseFontSize;
       break;
+    case CONTEXT_DOWNLOAD_SHELF_STATUS:
+      size_delta = kStatusSize - gfx::PlatformFont::kDefaultBaseFontSize;
+      break;
     default:
       break;
   }
@@ -74,10 +81,11 @@ const gfx::FontList& ChromeTypographyProvider::GetFont(int context,
     // Limit emphasizing text to contexts where it's obviously correct. If you
     // hit this DCHECK, ensure it's sane and UX-approved to extend it to your
     // new case (e.g. don't add CONTEXT_BUTTON_MD).
-    DCHECK(context == CONTEXT_BODY_TEXT_LARGE ||
-           context == CONTEXT_BODY_TEXT_SMALL ||
+    DCHECK(context == views::style::CONTEXT_LABEL ||
            context == views::style::CONTEXT_MESSAGE_BOX_BODY_TEXT ||
-           context == views::style::CONTEXT_LABEL);
+           context == CONTEXT_BODY_TEXT_LARGE ||
+           context == CONTEXT_BODY_TEXT_SMALL ||
+           context == CONTEXT_DOWNLOAD_SHELF);
     font_weight = gfx::Font::Weight::SEMIBOLD;
   }
 
@@ -104,6 +112,25 @@ SkColor ChromeTypographyProvider::GetColor(const views::View& view,
   if (context == CONTEXT_BODY_TEXT_LARGE || context == CONTEXT_BODY_TEXT_SMALL)
     context = views::style::CONTEXT_LABEL;
 
+  if (context == CONTEXT_DOWNLOAD_SHELF ||
+      (context == CONTEXT_DOWNLOAD_SHELF_STATUS &&
+       style == views::style::STYLE_DISABLED)) {
+    // TODO(pkasting): Instead of reusing COLOR_BOOKMARK_TEXT, use dedicated
+    // values.
+    const auto* theme_provider = view.GetThemeProvider();
+    if (!theme_provider)
+      return gfx::kPlaceholderColor;
+    const SkColor base_color =
+        theme_provider->GetColor(ThemeProperties::COLOR_BOOKMARK_TEXT);
+    // TODO(pkasting): Should use some way of dimming text that's as analogous
+    // as possible to e.g. enabled vs. disabled labels.
+    const SkColor dimmed_color = SkColorSetA(base_color, 0xC7);
+    if (style == views::style::STYLE_DISABLED)
+      return dimmed_color;
+    if (context == CONTEXT_DOWNLOAD_SHELF)
+      return base_color;
+  }
+
   // Monospaced styles have the same colors as their normal counterparts.
   if (style == STYLE_PRIMARY_MONOSPACED) {
     style = views::style::STYLE_PRIMARY;
@@ -184,10 +211,11 @@ int ChromeTypographyProvider::GetLineHeight(int context, int style) const {
       return kButtonAbsoluteHeight;
     case views::style::CONTEXT_DIALOG_TITLE:
       return title_height;
-    case CONTEXT_BODY_TEXT_LARGE:
-    case CONTEXT_TAB_HOVER_CARD_TITLE:
     case views::style::CONTEXT_MESSAGE_BOX_BODY_TEXT:
     case views::style::CONTEXT_TABLE_ROW:
+    case CONTEXT_BODY_TEXT_LARGE:
+    case CONTEXT_TAB_HOVER_CARD_TITLE:
+    case CONTEXT_DOWNLOAD_SHELF:
       return body_large_height;
     case CONTEXT_HEADLINE:
       return headline_height;

chrome/browser/ui/views/download/download_item_view.cc

@@ -240,14 +240,14 @@ DownloadItemView::DownloadItemView(DownloadUIModel::DownloadUIModelPtr download,
     file_name_style = STYLE_EMPHASIZED;
 #endif
   auto file_name_label = std::make_unique<views::Label>(
-      ElidedFilename(), views::style::CONTEXT_LABEL, file_name_style);
+      ElidedFilename(), CONTEXT_DOWNLOAD_SHELF, file_name_style);
   file_name_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   file_name_label->GetViewAccessibility().OverrideIsIgnored(true);
   file_name_label_ = AddChildView(std::move(file_name_label));
 
   auto status_label = std::make_unique<views::Label>(
-      l10n_util::GetStringUTF16(IDS_DOWNLOAD_STATUS_STARTING));
-  status_label->SetFontList(status_font_list_);
+      l10n_util::GetStringUTF16(IDS_DOWNLOAD_STATUS_STARTING),
+      CONTEXT_DOWNLOAD_SHELF_STATUS);
   status_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   status_label->GetViewAccessibility().OverrideIsIgnored(true);
   status_label_ = AddChildView(std::move(status_label));
@@ -294,13 +294,6 @@ void DownloadItemView::StopDownloadProgress() {
   progress_timer_.Stop();
 }
 
-// static
-SkColor DownloadItemView::GetTextColorForThemeProvider(
-    const ui::ThemeProvider* theme) {
-  return theme ? theme->GetColor(ThemeProperties::COLOR_BOOKMARK_TEXT)
-               : gfx::kPlaceholderColor;
-}
-
 void DownloadItemView::OnExtractIconComplete(IconLoader::IconSize icon_size,
                                              gfx::Image icon_bitmap) {
   if (!icon_bitmap.IsEmpty()) {
@@ -533,8 +526,7 @@ void DownloadItemView::Layout() {
     file_name_label_->SetBoundsRect(
         gfx::Rect(mirrored_x, file_name_y, kTextWidth, font_list_.GetHeight()));
 
-    int status_y =
-        file_name_y + font_list_.GetBaseline() + kVerticalTextPadding;
+    int status_y = file_name_y + font_list_.GetHeight();
     bool should_expand_for_status_text =
         (model_->GetDangerType() ==
          download::DOWNLOAD_DANGER_TYPE_DEEP_SCANNED_SAFE);
@@ -557,14 +549,13 @@ void DownloadItemView::UpdateDropdownButton() {
   views::SetImageFromVectorIcon(
       dropdown_button_,
       dropdown_state_ == PUSHED ? kCaretDownIcon : kCaretUpIcon,
-      GetTextColor());
+      GetThemeProvider()->GetColor(ThemeProperties::COLOR_BOOKMARK_TEXT));
 }
 
 gfx::Size DownloadItemView::CalculatePreferredSize() const {
   int width = 0;
   // We set the height to the height of two rows or text plus margins.
-  int child_height = font_list_.GetBaseline() + kVerticalTextPadding +
-                     status_font_list_.GetHeight();
+  int child_height = font_list_.GetHeight() + status_font_list_.GetHeight();
 
   if (IsShowingWarningDialog() || IsShowingMixedContentDialog()) {
     // Width.
@@ -663,12 +654,6 @@ void DownloadItemView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   node_data->SetDescription(base::string16());
 }
 
-void DownloadItemView::AddedToWidget() {
-  // Only required because OnThemeChanged is not called when a View is added to
-  // a Widget.
-  UpdateDropdownButton();
-}
-
 void DownloadItemView::OnThemeChanged() {
   views::View::OnThemeChanged();
   UpdateColorsFromTheme();
@@ -783,9 +768,9 @@ void DownloadItemView::OnPaint(gfx::Canvas* canvas) {
 }
 
 int DownloadItemView::GetYForFilenameText() const {
-  int text_height = font_list_.GetBaseline();
+  int text_height = font_list_.GetHeight();
   if (!status_label_->GetText().empty())
-    text_height += kVerticalTextPadding + status_font_list_.GetBaseline();
+    text_height += status_font_list_.GetHeight();
   return (height() - text_height) / 2;
 }
 
@@ -936,18 +921,16 @@ void DownloadItemView::UpdateColorsFromTheme() {
       std::make_unique<SeparatorBorder>(GetThemeProvider()->GetColor(
           ThemeProperties::COLOR_TOOLBAR_VERTICAL_SEPARATOR)));
 
-  // Use a slightly dimmed version of the base text color.
-  SkColor dimmed_text_color = SkColorSetA(GetTextColor(), 0xC7);
-  file_name_label_->SetEnabledColor(GetEnabled() ? GetTextColor()
-                                                 : dimmed_text_color);
+  file_name_label_->SetTextStyle(GetEnabled() ? views::style::STYLE_PRIMARY
+                                              : views::style::STYLE_DISABLED);
   if (model_->GetDangerType() ==
       download::DOWNLOAD_DANGER_TYPE_DEEP_SCANNED_SAFE) {
-    status_label_->SetEnabledColor(SK_ColorGREEN);
+    status_label_->SetTextStyle(STYLE_GREEN);
   } else if (model_->GetDangerType() ==
              download::DOWNLOAD_DANGER_TYPE_DEEP_SCANNED_OPENED_DANGEROUS) {
-    status_label_->SetEnabledColor(SK_ColorRED);
+    status_label_->SetTextStyle(STYLE_RED);
   } else {
-    status_label_->SetEnabledColor(dimmed_text_color);
+    status_label_->SetTextStyle(views::style::STYLE_PRIMARY);
   }
   SkColor background_color =
       GetThemeProvider()->GetColor(ThemeProperties::COLOR_DOWNLOAD_SHELF);
@@ -985,21 +968,15 @@ void DownloadItemView::ShowContextMenuImpl(const gfx::Rect& rect,
 }
 
 void DownloadItemView::SetDropdownState(State new_state) {
-  // Avoid extra SchedulePaint()s if the state is going to be the same and
-  // |dropdown_button_| has already been initialized.
-  if (dropdown_state_ == new_state &&
-      !dropdown_button_->GetImage(views::Button::STATE_NORMAL).isNull())
-    return;
-
   if (new_state != dropdown_state_) {
     dropdown_button_->AnimateInkDrop(new_state == PUSHED
                                          ? views::InkDropState::ACTIVATED
                                          : views::InkDropState::DEACTIVATED,
                                      nullptr);
+    dropdown_state_ = new_state;
+    UpdateDropdownButton();
+    SchedulePaint();
   }
-  dropdown_state_ = new_state;
-  UpdateDropdownButton();
-  SchedulePaint();
 }
 
 void DownloadItemView::SetMode(Mode mode) {
@@ -1193,6 +1170,7 @@ void DownloadItemView::ShowWarningDialog() {
       model_->GetWarningText(font_list_, kTextWidth);
   auto dangerous_download_label = std::make_unique<views::StyledLabel>(
       dangerous_label, /*listener=*/nullptr);
+  dangerous_download_label->SetTextContext(CONTEXT_DOWNLOAD_SHELF);
   dangerous_download_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   dangerous_download_label->SetAutoColorReadabilityEnabled(false);
   dangerous_download_label->set_can_process_events_within_subtree(false);
@@ -1301,6 +1279,7 @@ void DownloadItemView::ShowDeepScanningDialog() {
 
   auto deep_scanning_label = std::make_unique<views::StyledLabel>(
       deep_scanning_text, /*listener=*/nullptr);
+  deep_scanning_label->SetTextContext(CONTEXT_DOWNLOAD_SHELF);
   deep_scanning_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   deep_scanning_label->SetAutoColorReadabilityEnabled(false);
   deep_scanning_label->set_can_process_events_within_subtree(false);
@@ -1599,10 +1578,6 @@ void DownloadItemView::ProgressTimerFired() {
     SchedulePaint();
 }
 
-SkColor DownloadItemView::GetTextColor() const {
-  return GetTextColorForThemeProvider(GetThemeProvider());
-}
-
 base::string16 DownloadItemView::GetStatusText() const {
   if (model_->GetDangerType() ==
       download::DownloadDangerType::DOWNLOAD_DANGER_TYPE_DEEP_SCANNED_SAFE) {

chrome/browser/ui/views/download/download_item_view.h

@@ -46,10 +46,6 @@ class ImageSkia;
 class SlideAnimation;
 }
 
-namespace ui {
-class ThemeProvider;
-}
-
 namespace views {
 class ImageButton;
 class Label;
@@ -75,9 +71,6 @@ class DownloadItemView : public views::View,
   void StartDownloadProgress();
   void StopDownloadProgress();
 
-  // Returns the base color for text on this download item, based on |theme|.
-  static SkColor GetTextColorForThemeProvider(const ui::ThemeProvider* theme);
-
   void OnExtractIconComplete(IconLoader::IconSize icon_size, gfx::Image icon);
 
   // Returns the DownloadUIModel object belonging to this item.
@@ -119,7 +112,6 @@ class DownloadItemView : public views::View,
  protected:
   // views::View:
   void OnPaint(gfx::Canvas* canvas) override;
-  void AddedToWidget() override;
   void OnThemeChanged() override;
 
  private:
@@ -140,9 +132,6 @@ class DownloadItemView : public views::View,
 
   static constexpr int kTextWidth = 140;
 
-  // Vertical padding between filename and status text.
-  static constexpr int kVerticalTextPadding = 1;
-
   static constexpr int kTooltipMaxWidth = 800;
 
   // Padding before the icon and at end of the item.

chrome/browser/ui/views/download/download_shelf_view.cc

@@ -322,7 +322,7 @@ void DownloadShelfView::UpdateColorsFromTheme() {
 
   views::SetImageFromVectorIcon(
       close_button_, vector_icons::kCloseRoundedIcon,
-      DownloadItemView::GetTextColorForThemeProvider(GetThemeProvider()));
+      GetThemeProvider()->GetColor(ThemeProperties::COLOR_BOOKMARK_TEXT));
 }
 
 void DownloadShelfView::AddedToWidget() {