Introduce a type of View background that stays in sync with its host

chrome/browser/ui/views/sad_tab_view.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/ui/views/sad_tab_view.h"
 
 #include <string>
 
 #include "base/metrics/histogram_macros.h"
 #include "build/build_config.h"
@@ skipping 18 matching lines @@
 
 constexpr int kMaxContentWidth = 600;
 constexpr int kMinColumnWidth = 120;
 constexpr int kTitleBottomSpacing = 13;
 
 }  // namespace
 
 SadTabView::SadTabView(content::WebContents* web_contents,
                        chrome::SadTabKind kind)
     : SadTab(web_contents, kind) {
-  // Set the background color.
-  set_background(
-      views::Background::CreateSolidBackground(GetNativeTheme()->GetSystemColor(
-          ui::NativeTheme::kColorId_DialogBackground)));
+  set_background(views::Background::CreateThemedSolidBackground(
+      this, ui::NativeTheme::kColorId_DialogBackground));
 
   views::GridLayout* layout = new views::GridLayout(this);
   SetLayoutManager(layout);
 
   const int column_set_id = 0;
   views::ColumnSet* columns = layout->AddColumnSet(column_set_id);
   columns->AddPaddingColumn(1, views::kPanelSubVerticalSpacing);
   columns->AddColumn(views::GridLayout::LEADING, views::GridLayout::LEADING, 0,
                      views::GridLayout::USE_PREF, 0, kMinColumnWidth);
   columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::LEADING, 0,
                      views::GridLayout::USE_PREF, 0, kMinColumnWidth);
   columns->AddPaddingColumn(1, views::kPanelSubVerticalSpacing);
 
   views::ImageView* image = new views::ImageView();
 
   image->SetImage(
       gfx::CreateVectorIcon(kCrashedTabIcon, 48, gfx::kChromeIconGrey));
   layout->AddPaddingRow(1, views::kPanelVerticalSpacing);
   layout->StartRow(0, column_set_id);
   layout->AddView(image, 2, 1);
 
-  title_ = CreateLabel(l10n_util::GetStringUTF16(GetTitle()));
+  title_ = new views::Label(l10n_util::GetStringUTF16(GetTitle()));

[msw, 2017/04/19 17:47:36]

These labels and links no longer initialize the background color for
readability. Why shouldn't they call SetBackgroundColor any more?

[Evan Stade, 2017/04/19 18:25:53]

they shouldn't need to because any coherent NativeTheme ensures that default
text color for both labels and links render readably on the DialogBackground
color.

   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   title_->SetFontList(rb.GetFontList(ui::ResourceBundle::LargeFont));
   title_->SetMultiLine(true);
   title_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   layout->StartRowWithPadding(0, column_set_id, 0,
                               views::kPanelVerticalSpacing);
   layout->AddView(title_, 2, 1);
 
-  const SkColor text_color = GetNativeTheme()->GetSystemColor(
-      ui::NativeTheme::kColorId_LabelDisabledColor);
-
-  message_ = CreateLabel(l10n_util::GetStringUTF16(GetMessage()));
+  message_ = new views::Label(l10n_util::GetStringUTF16(GetMessage()));
 
   message_->SetMultiLine(true);
-  message_->SetEnabledColor(text_color);
+  message_->SetEnabled(false);
   message_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   message_->SetLineHeight(views::kPanelSubVerticalSpacing);
 
   layout->StartRowWithPadding(0, column_set_id, 0, kTitleBottomSpacing);
   layout->AddView(message_, 2, 1, views::GridLayout::LEADING,
                   views::GridLayout::LEADING);
 
   action_button_ = views::MdTextButton::CreateSecondaryUiBlueButton(
       this, l10n_util::GetStringUTF16(GetButtonTitle()));
-  help_link_ =
-      CreateLink(l10n_util::GetStringUTF16(GetHelpLinkTitle()), text_color);
+  help_link_ = new views::Link(l10n_util::GetStringUTF16(GetHelpLinkTitle()));

[msw, 2017/04/19 17:47:36]

This link no longer gets the color of a disabled label; why change that?

[Evan Stade, 2017/04/19 18:25:53]

I thought it better to match how all the other links in Chrome look, and this
also means that the link color will update when switching native themes. (And
since this is called before SadTabView is added to a widget hierarchy, that's
important.)

[msw, 2017/04/19 19:00:54]

Presumably a UX designer explicitly selected the disabled text theme color for
this sad tab link, shouldn't you ping them and/or ensure this matches
Cocoa/Android/iOS?

More generally, how do controls with custom (or pre-calculated themed) colors
handle backgrounds that change with the native theme? It seems like native theme
changes should also (for example) update a label's background color and either
update a themed foreground text color or recalculate a readable text color as
needed... That seems difficult to accomplish, so presumably any labels used on
top of CreateThemedSolidBackground views should never set non-default text
colors nor use SetBackgroundColor? It seems like this is a recipe for having
weird mismatching colors on native theme changes unless we carefully audit all
related views... I found a couple questionable examples by doing another quick
pass on c/b/u/v.

[Evan Stade, 2017/04/20 04:44:22]

I would ping Hwi, but she is ooo. I see this as one of many places where in the
past we made disjointed UI decisions that we'd like to be more coherent now
(hence project name "harmony"). I think the only real use case for
SetEnabledColor on views::Link is for links affected by custom browser themes
(e.g. the link to import bookmarks).
yes
Could you point them out? (Or are you referring to the ones you have already?)

In PasswordGenerationPopupViewViews I see many potential color/theme related
bugs, both with and without this change.

[msw, 2017/04/20 18:21:45]

I'll go along with that if you plan on doing the same thing for other platforms'
Sad tabs... I presume we also care about cross-platform 'harmony'.
Yeah, I pointed out the issues I found just from reading code (looking for
colors used near your updated backgrounds).
Let's not introduce more :)

+  help_link_->set_listener(this);
   layout->StartRowWithPadding(0, column_set_id, 0,
                               views::kPanelVerticalSpacing);
   layout->AddView(help_link_, 1, 1, views::GridLayout::LEADING,
                   views::GridLayout::CENTER);
   layout->AddView(action_button_, 1, 1, views::GridLayout::TRAILING,
                   views::GridLayout::LEADING);
 
   layout->AddPaddingRow(2, views::kPanelSubVerticalSpacing);
 
   views::Widget::InitParams sad_tab_params(
@@ skipping 42 matching lines @@
 }
 
 void SadTabView::OnPaint(gfx::Canvas* canvas) {
   if (!painted_) {
     RecordFirstPaint();
     painted_ = true;
   }
   View::OnPaint(canvas);
 }
 
-views::Label* SadTabView::CreateLabel(const base::string16& text) {
-  views::Label* label = new views::Label(text);
-  label->SetBackgroundColor(background()->get_color());
-  return label;
-}
-
-views::Link* SadTabView::CreateLink(const base::string16& text,
-                                    const SkColor& color) {
-  views::Link* link = new views::Link(text);
-  link->SetBackgroundColor(background()->get_color());
-  link->SetEnabledColor(color);
-  link->set_listener(this);
-  return link;
-}
-
 namespace chrome {
 
 SadTab* SadTab::Create(content::WebContents* web_contents,
                        SadTabKind kind) {
   return new SadTabView(web_contents, kind);
 }
 
 }  // namespace chrome