Polish VC DLC UI download states

- Support the proper color changes for kPending/kError
 - Add pixel tests for kPending/kError
 - Support the new label for kError
 - Ensure tooltips update properly with the download state
   labels.

Bug: b:315188874
Change-Id: I066b6236fd196920aaddbc9fdc3a20130b7af669
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5512030
Reviewed-by: Andre Le <leandre@chromium.org>
Commit-Queue: Alex Newcomer <newcomer@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1297257}

ash/ash_strings.grd

@@ -950,6 +950,9 @@ Style notes:
       <message name="IDS_ASH_FEATURE_TILE_DOWNLOAD_IN_PROGRESS_TITLE" desc="The title of a feature tile that currently has an in-progress download.">
         Downloading <ph name="DOWNLOAD_PERCENT">$1<ex>7</ex></ph>%
       </message>
+      <message name="IDS_ASH_FEATURE_TILE_DOWNLOAD_ERROR" desc="The title of a feature tile that has an error downloading.">
+        <ph name="FEATURE_NAME">$1<ex>Live Caption</ex></ph> is unavailable
+      </message>
 
       <!-- Upgrade notifications -->
       <message name="IDS_RELAUNCH_REQUIRED_TITLE_DAYS" desc="The title of a dialog that tells users the device must be restarted within two or more days.">

ash/ash_strings_grd/IDS_ASH_FEATURE_TILE_DOWNLOAD_ERROR.png.sha1

@@ -0,0 +1 @@
+67e762c2d518949957e4ffd0da14da69472581d7

ash/system/unified/feature_tile.cc

@@ -14,6 +14,7 @@
 #include "ash/style/dark_light_mode_controller_impl.h"
 #include "ash/style/typography.h"
 #include "ash/system/tray/tray_constants.h"
+#include "base/auto_reset.h"
 #include "base/strings/string_number_conversions.h"
 #include "cc/paint/paint_flags.h"
 #include "components/vector_icons/vector_icons.h"
@@ -198,6 +199,25 @@ FeatureTile::~FeatureTile() {
   title_container_->RemoveObserver(this);
 }
 
+void FeatureTile::OnSetTooltipText(const std::u16string& tooltip_text) {
+  if (!features::IsVcDlcUiEnabled() || updating_download_state_labels_) {
+    return;
+  }
+
+  // Keep track of the client set tooltip, so it can be restored if a
+  // `DownloadState` tooltip has been temporarily set.
+  client_specified_tooltip_text_ = tooltip_text;
+
+  // If the tooltip was set while a temporary downloading tooltip text was set,
+  // restore it. This will be reset to the `client_specified_tooltip_text_` once
+  // the download state changes back to the final state (if it's not
+  // `DownloadState::kError`).
+  if (download_state_ != DownloadState::kDownloaded &&
+      download_state_ != DownloadState::kNone) {
+    UpdateLabelForDownloadState();
+  }
+}
+
 void FeatureTile::CreateChildViews() {
   const bool is_compact = type_ == TileType::kCompact;
 
@@ -340,15 +360,23 @@ void FeatureTile::CreateDecorativeDrillInArrow() {
 
 void FeatureTile::UpdateColors() {
   ui::ColorId background_color;
-  ui::ColorId foreground_color;
-  ui::ColorId foreground_optional_color;
-
   if (GetEnabled()) {
     background_color =
         toggled_
             ? background_toggled_color_.value_or(
                   cros_tokens::kCrosSysSystemPrimaryContainer)
             : background_color_.value_or(cros_tokens::kCrosSysSystemOnBase);
+  } else {
+    background_color = background_disabled_color_.value_or(
+        cros_tokens::kCrosSysDisabledContainer);
+  }
+
+  ui::ColorId foreground_color;
+  ui::ColorId foreground_optional_color;
+  // The `DownloadState::kPending` state should have the same colors on the
+  // labels and images as an enabled button, per the spec. The labels and images
+  // will only look disabled if the button was disabled for other reasons.
+  if (GetEnabled() || download_state_ == DownloadState::kPending) {
     foreground_color =
         toggled_ ? foreground_toggled_color_.value_or(
                        cros_tokens::kCrosSysSystemOnPrimaryContainer)
@@ -359,8 +387,6 @@ void FeatureTile::UpdateColors() {
                  : foreground_optional_color_.value_or(
                        cros_tokens::kCrosSysOnSurfaceVariant);
   } else {
-    background_color = background_disabled_color_.value_or(
-        cros_tokens::kCrosSysDisabledContainer);
     foreground_color =
         foreground_disabled_color_.value_or(cros_tokens::kCrosSysDisabled);
     foreground_optional_color =
@@ -552,9 +578,6 @@ void FeatureTile::SetLabel(const std::u16string& label) {
   }
 
   label_->SetText(label);
-  if (GetTooltipText().empty()) {
-    SetTooltipText(label);
-  }
 }
 
 int FeatureTile::GetSubLabelMaxWidth() const {
@@ -604,18 +627,34 @@ void FeatureTile::SetDownloadState(DownloadState state, int progress) {
       return;
     }
   }
-
-  // Set the new download state and update the tile to reflect it.
-  if (state == DownloadState::kDownloading) {
-    CHECK_GE(progress, 0)
-        << "Expected download progress to be in the range [0, 100], actual: "
-        << progress;
-    CHECK_LE(progress, 100)
-        << "Expected download progress to be in the range [0, 100], actual: "
-        << progress;
-    download_progress_percent_ = progress;
-  }
   download_state_ = state;
+
+  switch (download_state_) {
+    case DownloadState::kDownloading:
+      CHECK_GE(progress, 0)
+          << "Expected download progress to be in the range [0, 100], actual: "
+          << progress;
+      CHECK_LE(progress, 100)
+          << "Expected download progress to be in the range [0, 100], actual: "
+          << progress;
+      SetEnabled(true);
+      download_progress_percent_ = progress;
+      break;
+    case DownloadState::kError:
+    case DownloadState::kPending:
+      SetEnabled(false);
+      download_progress_percent_ = 0;
+      break;
+    case DownloadState::kDownloaded:
+      SetEnabled(true);
+      download_progress_percent_ = 0;
+      break;
+    case DownloadState::kNone:
+      SetEnabled(true);
+      download_progress_percent_ = 0;
+      break;
+  }
+
   UpdateColors();
   UpdateLabelForDownloadState();
 }
@@ -721,17 +760,29 @@ void FeatureTile::SetDownloadLabel(const std::u16string& download_label) {
   CHECK(features::IsVcDlcUiEnabled())
       << "Download states are not supported when `VcDlcUi` is disabled";
   label_->SetText(download_label);
+  SetTooltipText(download_label);
 }
 
 void FeatureTile::UpdateLabelForDownloadState() {
   // Download state is only supported when `VcDlcUi` is enabled.
   CHECK(features::IsVcDlcUiEnabled())
       << "Download states are not supported when `VcDlcUi` is disabled";
+
+  base::AutoReset<bool> reset(&updating_download_state_labels_, true);
+
   switch (download_state_) {
+    case DownloadState::kError:
+      SetDownloadLabel(l10n_util::GetStringFUTF16(
+          IDS_ASH_FEATURE_TILE_DOWNLOAD_ERROR, client_specified_label_text_));
+      break;
     case DownloadState::kNone:
     case DownloadState::kDownloaded:
-    case DownloadState::kError:
+      // If a download happened, `SetLabel()` saved the previous label
+      // in `client_specified_label_text_`, so re-set those here.
       label_->SetText(client_specified_label_text_);
+      SetTooltipText(client_specified_tooltip_text_.empty()
+                         ? client_specified_label_text_
+                         : client_specified_tooltip_text_);
       break;
     case DownloadState::kPending:
       SetDownloadLabel(l10n_util::GetStringUTF16(

ash/system/unified/feature_tile.h

@@ -230,6 +230,9 @@ class ASH_EXPORT FeatureTile : public views::Button {
     const ui::ColorId background_color_id_;
   };
 
+  // views::Button:
+  void OnSetTooltipText(const std::u16string& tooltip_text) override;
+
   // Creates child views of Feature Tile. The constructed view will vary
   // depending on the button's `type_`.
   void CreateChildViews();
@@ -308,11 +311,12 @@ class ASH_EXPORT FeatureTile : public views::Button {
   bool toggled_ = false;
 
   // The non-download-related (a.k.a. "client-specified") text of this tile's
-  // label. The tile's label may change when its downloading state changes, so
-  // this is used to store the original, client-specified label for later
-  // reference (e.g. when a download finishes and the tile needs to show the
-  // original label again).
+  // label/tooltip. The tile's label/tooltip may change when its downloading
+  // state changes, so this is used to store the original, client-specified
+  // label/tooltip for later reference (e.g. when a download finishes and the
+  // tile needs to show the original label again).
   std::u16string client_specified_label_text_;
+  std::u16string client_specified_tooltip_text_;
 
   // The type of the feature tile that determines how it lays out its view.
   TileType type_;
@@ -325,6 +329,9 @@ class ASH_EXPORT FeatureTile : public views::Button {
   // download by default.
   DownloadState download_state_ = DownloadState::kNone;
 
+  // True when `UpdateLabelForDownloadState()` is happening.
+  bool updating_download_state_labels_ = false;
+
   // The download progress, as an integer percentage in the range [0, 100]. Only
   // has meaning when the tile is in an active download state.
   int download_progress_percent_ = 0;

ash/system/unified/feature_tile_pixeltest.cc

@@ -363,4 +363,20 @@ TEST_F(FeatureTileVcDlcUiEnabledPixelTest, DownloadInProgress) {
       /*revision_number=*/1, widget_.get()));
 }
 
+// Tests the UI of a compact tile that has an error during download.
+TEST_F(FeatureTileVcDlcUiEnabledPixelTest, ErrorInDlcDownload) {
+  tile()->SetDownloadState(FeatureTile::DownloadState::kError, 0);
+  EXPECT_TRUE(GetPixelDiffer()->CompareUiComponentsOnPrimaryScreen(
+      "error",
+      /*revision_number=*/0, widget_.get()));
+}
+
+// Tests the UI of a compact tile that has a pending download.
+TEST_F(FeatureTileVcDlcUiEnabledPixelTest, PendingDownload) {
+  tile()->SetDownloadState(FeatureTile::DownloadState::kPending, 0);
+  EXPECT_TRUE(GetPixelDiffer()->CompareUiComponentsOnPrimaryScreen(
+      "pending",
+      /*revision_number=*/0, widget_.get()));
+}
+
 }  // namespace ash

ash/system/unified/feature_tile_unittest.cc

@@ -509,79 +509,105 @@ TEST_P(FeatureTileTest, AccessibilityRoles) {
   EXPECT_EQ(node_data4.role, ax::mojom::Role::kButton);
 }
 
-// Tests that the tile's label is set according to its download state.
-TEST_P(FeatureTileTest, DownloadLabel) {
+// Tests that the tile's label and tooltip are set according to the feature tile
+// DLC's download state.
+TEST_P(FeatureTileTest, DownloadLabelAndTooltip) {
   // Download states are only supported when `VcDlcUi` is enabled.
   if (!IsVcDlcUiEnabled()) {
     return;
   }
 
   // Create a tile and verify that it has its client-specified label by default.
-  std::u16string default_label(u"Default label");
+  std::u16string client_specified_label(u"Client Specified Label");
+  std::u16string client_specified_tooltip(u"Client Specified Tooltip");
   FeatureTile tile(views::Button::PressedCallback(),
                    /*is_togglable=*/true);
-  tile.SetLabel(default_label);
-  EXPECT_EQ(default_label, tile.label()->GetText());
+  tile.SetLabel(client_specified_label);
+  tile.SetTooltipText(client_specified_tooltip);
+  EXPECT_EQ(client_specified_label, tile.label()->GetText());
+  EXPECT_EQ(client_specified_tooltip, tile.GetTooltipText());
 
-  // Set the tile to have a pending download and verify that the label is
-  // updated accordingly.
+  // Set the tile to have a pending download and verify that the label and
+  // tooltip are updated.
   tile.SetDownloadState(FeatureTile::DownloadState::kPending, /*progress=*/0);
+
   EXPECT_EQ(GetExpectedDownloadPendingLabel(), tile.label()->GetText());
+  EXPECT_EQ(GetExpectedDownloadPendingLabel(), tile.GetTooltipText());
 
   // Set the tile to have an in-progress download and verify that the label is
   // updated accordingly.
   tile.SetDownloadState(FeatureTile::DownloadState::kDownloading,
                         /*progress=*/7);
+
   EXPECT_EQ(GetExpectedDownloadInProgressLabel(/*progress=*/7),
             tile.label()->GetText());
+  EXPECT_EQ(GetExpectedDownloadInProgressLabel(/*progress=*/7),
+            tile.GetTooltipText());
 
   // Set the tile to have a successfully-completed download and verify that the
   // label is set to the client-specified label.
   tile.SetDownloadState(FeatureTile::DownloadState::kDownloaded,
                         /*progress=*/0);
-  EXPECT_EQ(default_label, tile.label()->GetText());
+
+  EXPECT_EQ(client_specified_label, tile.label()->GetText());
+  EXPECT_EQ(client_specified_tooltip, tile.GetTooltipText());
 
   // Set the tile to have an error with its download and verify that the label
-  // is set to the client-specified label.
+  // is set to the client-specified label with additional info.
   tile.SetDownloadState(FeatureTile::DownloadState::kError, /*progress=*/0);
-  EXPECT_EQ(default_label, tile.label()->GetText());
+  EXPECT_EQ(l10n_util::GetStringFUTF16(IDS_ASH_FEATURE_TILE_DOWNLOAD_ERROR,
+                                       client_specified_label),
+            tile.label()->GetText());
+  EXPECT_EQ(l10n_util::GetStringFUTF16(IDS_ASH_FEATURE_TILE_DOWNLOAD_ERROR,
+                                       client_specified_label),
+            tile.GetTooltipText());
 
   // Set the tile to have no associated download and verify that the label is
   // set to the client-specified label.
   tile.SetDownloadState(FeatureTile::DownloadState::kNone, /*progress=*/0);
-  EXPECT_EQ(default_label, tile.label()->GetText());
+  EXPECT_EQ(client_specified_label, tile.label()->GetText());
+  EXPECT_EQ(client_specified_tooltip, tile.GetTooltipText());
 }
 
-// Tests that updates to the tile's client-specified label are delayed until the
-// current download is finished.
-TEST_P(FeatureTileTest, LabelUpdatesDelayedDuringDownload) {
+// Tests that updates to the tile's client-specified label and tooltip are
+// delayed until the current download is finished.
+TEST_P(FeatureTileTest, LabelAndTooltipUpdatesDelayedDuringDownload) {
   // Download states are only supported when `VcDlcUi` is enabled.
   if (!IsVcDlcUiEnabled()) {
     return;
   }
 
   // Create a tile and set it to have an in-progress download.
-  std::u16string label_1(u"Default label");
+  std::u16string label_1(u"Client Specified Label");
+  std::u16string tooltip_1(u"Client Specified Tooltip");
   FeatureTile tile(views::Button::PressedCallback(),
                    /*is_togglable=*/true);
   tile.SetLabel(label_1);
+  tile.SetTooltipText(tooltip_1);
   tile.SetDownloadState(FeatureTile::DownloadState::kDownloading,
                         /*progress=*/7);
   ASSERT_EQ(GetExpectedDownloadInProgressLabel(/*progress=*/7),
             tile.label()->GetText());
+  ASSERT_EQ(GetExpectedDownloadInProgressLabel(/*progress=*/7),
+            tile.GetTooltipText());
 
-  // Change the tile's client-specified label and verify that the change is not
-  // yet reflected due to the on-going download.
-  std::u16string label_2(u"New label");
+  // Change the tile's client-specified label and tooltip, and verify that the
+  // change is not yet reflected due to the on-going download.
+  std::u16string label_2(u"New Label");
+  std::u16string tooltip_2(u"New Tooltip");
   tile.SetLabel(label_2);
+  tile.SetTooltipText(tooltip_2);
   EXPECT_EQ(GetExpectedDownloadInProgressLabel(/*progress=*/7),
             tile.label()->GetText());
+  EXPECT_EQ(GetExpectedDownloadInProgressLabel(/*progress=*/7),
+            tile.GetTooltipText());
 
   // Set the tile's download to be finished and verify that the tile now has the
   // new client-specified label.
   tile.SetDownloadState(FeatureTile::DownloadState::kDownloaded,
                         /*progress=*/0);
   EXPECT_EQ(label_2, tile.label()->GetText());
+  EXPECT_EQ(tooltip_2, tile.GetTooltipText());
 
   // Set the tile to have a pending download.
   tile.SetDownloadState(FeatureTile::DownloadState::kPending, /*progress=*/0);
@@ -590,14 +616,22 @@ TEST_P(FeatureTileTest, LabelUpdatesDelayedDuringDownload) {
   // Change the tile's client-specified label and verify that the change is not
   // yet reflected due to the pending download.
   std::u16string label_3(u"Another new label");
+  std::u16string tooltip_3(u"Another new label");
   tile.SetLabel(label_3);
+  tile.SetTooltipText(tooltip_3);
   EXPECT_EQ(GetExpectedDownloadPendingLabel(), tile.label()->GetText());
+  EXPECT_EQ(GetExpectedDownloadPendingLabel(), tile.GetTooltipText());
 
   // Set the tile's download to be finished (with an error this time, for
   // for variety) and verify that the tile now has the new client-specified
-  // label.
+  // label with the error message.
   tile.SetDownloadState(FeatureTile::DownloadState::kError, /*progress=*/0);
-  EXPECT_EQ(label_3, tile.label()->GetText());
+  EXPECT_EQ(
+      l10n_util::GetStringFUTF16(IDS_ASH_FEATURE_TILE_DOWNLOAD_ERROR, label_3),
+      tile.label()->GetText());
+  EXPECT_EQ(
+      l10n_util::GetStringFUTF16(IDS_ASH_FEATURE_TILE_DOWNLOAD_ERROR, label_3),
+      tile.GetTooltipText());
 }
 
 }  // namespace ash