Sad tab redesign for Windows, Linux and CrOS

chrome/browser/ui/views/sad_tab_view.cc

Index: chrome/browser/ui/views/sad_tab_view.cc
diff --git a/chrome/browser/ui/views/sad_tab_view.cc b/chrome/browser/ui/views/sad_tab_view.cc
index 173455ecb87ba8aed454c7f4ceb5db9b65843fd6..cef597b42dcb09cc667e8485e45fe8e0568d2933 100644
--- a/chrome/browser/ui/views/sad_tab_view.cc
+++ b/chrome/browser/ui/views/sad_tab_view.cc
@@ -9,6 +9,7 @@
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/chrome_pages.h"
@@ -22,11 +23,13 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/views/background.h"
+#include "ui/views/controls/button/blue_button.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/button/label_button_border.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
+#include "ui/views/controls/styled_label.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/widget/widget.h"
 
@@ -35,16 +38,45 @@ using content::WebContents;
 
 namespace {
 
-const int kPadding = 20;
+const int kPadding = 16;
+const int kTopPadding = 100;

[msw, 2015/05/11 18:27:09]

Why are we doing such excessive padding instead of centering?

[edwardjung, 2015/05/14 18:31:27]

The design was updated to be vertically anchored to the top with a maximum top
padding. This makes it consistent with other interstitial pages. The bug thread
has the updated design. As you indicated, on smaller screen sizes the padding
should be reduced and this is indeed how the other interstitials work.

I've altered this to go for a top weighted layout which more closely matches the
design whilst remaining somewhat responsive to the screen size. 

+const int kLineHeight = 24;
+const int kMaxContentWidth = 600;
+const int kMinColumnWidth = 100;
+const int kLabelHeight = 50;
 const float kMessageSize = 0.65f;
-const SkColor kTextColor = SK_ColorWHITE;
-const SkColor kCrashColor = SkColorSetRGB(35, 48, 64);
-const SkColor kKillColor = SkColorSetRGB(57, 48, 88);
+const SkColor kTextColor = SkColorSetRGB(100, 100, 100);

[msw, 2015/05/11 18:27:09]

This isn't cool. Can we use (or make these) native theme colors?

[edwardjung, 2015/05/14 18:31:26]

Per the chat with ainslie and bettes to not use the native theme colours for
dialogues and windows, as we are treating the interstitials as a set which
should be consistent and not determined by the OS level theme. 

I did try the colours suggested kColorId_DialogBackground or
kColorId_WindowBackground but they came out as black.

[msw, 2015/05/14 23:50:19]

Post screenshots as I suggest, ainslie and bettes might not object.
If the background was black, the enum usage wasn't correct.

[edwardjung, 2015/05/18 12:02:35]

You assumed correct, I wasn't passing the enum to the getSystemColor call.

+const SkColor kTitleColor = SkColorSetRGB(51, 51, 51);
+const SkColor kCrashColor = SkColorSetRGB(247, 247, 247);
+const SkColor kKillColor = SkColorSetRGB(247, 247, 247);
+// const SkColor kButtonColor = SkColorSetRGB(76, 142, 250);

[msw, 2015/05/11 18:27:10]

Please review your own code thoroughly before asking others to do so.

[edwardjung, 2015/05/14 18:31:27]

Apologies again.

 
 const char kCategoryTagCrash[] = "Crash";
 
 }  // namespace
 
+namespace views {
+
+// Extends Label to allow defining a maximum width.
+class LabelWithMaxWidth : public Label {

[msw, 2015/05/11 18:27:09]

Can you instead call Label::SizeToFit for the multi-line labels (with the max of
the desired kMaxContentWidth and the available container/tab width) and let the
single line labels just use their preferred size.

[edwardjung, 2015/05/14 18:31:27]

Thanks for the suggestion. I've updated SadTabView::Layout which calls
SizeToFit. Removed this class.

+ public:
+  explicit LabelWithMaxWidth(const base::string16& text) : Label(text) {
+  }
+
+  ~LabelWithMaxWidth() override;
+
+  gfx::Size GetPreferredSize() const override {
+    gfx::Size out = Label::GetPreferredSize();
+    int width = std::min(out.width(), kMaxContentWidth);

[msw, 2015/05/11 18:27:09]

Might any single-line labels go through this code path with translated text that
exceeds this width? I worry that this width-capping isn't correct everywhere.

[edwardjung, 2015/05/14 18:31:26]

I removed this class per suggestion above.

+    out.SetSize(width, Label::GetHeightForWidth(width));
+    return out;
+  }
+};
+
+LabelWithMaxWidth::~LabelWithMaxWidth() {}

[msw, 2015/05/11 18:27:09]

nit: inline

[edwardjung, 2015/05/14 18:31:27]

No longer applicable

+
+} // namespace views
+
 SadTabView::SadTabView(WebContents* web_contents, chrome::SadTabKind kind)
     : web_contents_(web_contents),
       kind_(kind),
@@ -84,6 +116,19 @@ SadTabView::SadTabView(WebContents* web_contents, chrome::SadTabKind kind)
       NOTREACHED();
   }
 
+// Linux and Windows font size discrepency

[msw, 2015/05/11 18:27:09]

indentation...

[edwardjung, 2015/05/14 18:31:27]

Done.

+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+#if defined(OS_WIN)

[msw, 2015/05/11 18:27:09]

This is not acceptable. Pick a ui::ResourceBundle::MediumFont, SmallFont,
BaseFont, LargeFont or whatever and just use that. Having consistent UI
throughout our product is more important than matching mock perfectly.

[edwardjung, 2015/05/14 18:31:26]

In our minds, we are trying to achieve consistency with the other interstitials
UIs. However we agreed to use the standard platform font sizes for the
accessibility. BaseFont is an acceptable size.

+  const int kTitleFontAdjustment = 4;
+  gfx::FontList body_font_list_ =
+      rb.GetFontList(ui::ResourceBundle::MediumFont);
+  gfx::FontList link_font_list_ = medium_font_list_.DeriveWithSizeDelta(-1);
+#else
+  const int kTitleFontAdjustment = 0;
+  gfx::FontList body_font_list_ =
+      rb.GetFontList(ui::ResourceBundle::SmallFont);
+#endif
+
   // Set the background color.
   set_background(views::Background::CreateSolidBackground(
       (kind_ == chrome::SAD_TAB_KIND_CRASHED) ? kCrashColor : kKillColor));
@@ -93,78 +138,119 @@ SadTabView::SadTabView(WebContents* web_contents, chrome::SadTabKind kind)
 
   const int column_set_id = 0;
   views::ColumnSet* columns = layout->AddColumnSet(column_set_id);
-  columns->AddPaddingColumn(1, kPadding);
-  columns->AddColumn(views::GridLayout::CENTER, views::GridLayout::CENTER,
-                     0, views::GridLayout::USE_PREF, 0, 0);
-  columns->AddPaddingColumn(1, kPadding);
+  columns->AddPaddingColumn(0.5, kPadding);

[msw, 2015/05/11 18:27:09]

You can leave both of these at 1 for evenly balanced re-sizing.

[edwardjung, 2015/05/14 18:31:27]

Done.

+  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(0.5, kPadding);
 
   views::ImageView* image = new views::ImageView();
   image->SetImage(ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
-      (kind_ == chrome::SAD_TAB_KIND_CRASHED) ? IDR_SAD_TAB : IDR_KILLED_TAB));

[msw, 2015/05/11 18:27:09]

Are we no longer using IDR_KILLED_TAB? Remove the image assets, enum, etc.

[edwardjung, 2015/05/14 18:31:26]

That's correct, they will use the same image. I'll remove.

-  layout->StartRowWithPadding(0, column_set_id, 1, kPadding);
-  layout->AddView(image);
+      IDR_SAD_TAB));
+  layout->StartRowWithPadding(0, column_set_id, 0, kTopPadding);
+  layout->AddView(image, 2, 1);
 
   views::Label* title = CreateLabel(l10n_util::GetStringUTF16(
       (kind_ == chrome::SAD_TAB_KIND_CRASHED) ?
           IDS_SAD_TAB_TITLE : IDS_KILLED_TAB_TITLE));
-  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-  title->SetFontList(rb.GetFontList(ui::ResourceBundle::MediumFont));
-  layout->StartRowWithPadding(0, column_set_id, 0, kPadding);
-  layout->AddView(title);
+  // Font size is too large on Linux.

[msw, 2015/05/11 18:27:09]

No, if that's really the case we need a fix to the font size enums, etc.

[edwardjung, 2015/05/14 18:31:27]

Removed the font adjustment

+  title->SetFontList(rb.GetFontList(
+      ui::ResourceBundle::LargeFont).DeriveWithSizeDelta(kTitleFontAdjustment));
+  title->SetEnabledColor(kTitleColor);
+  layout->StartRowWithPadding(0, column_set_id, 0, kPadding * 2);
+  layout->AddView(title, 2, 1);
 
   message_ = CreateLabel(l10n_util::GetStringUTF16(
       (kind_ == chrome::SAD_TAB_KIND_CRASHED) ?
           IDS_SAD_TAB_MESSAGE : IDS_KILLED_TAB_MESSAGE));
+  message_->SetFontList(body_font_list_);
   message_->SetMultiLine(true);
+  message_->SetEnabledColor(kTextColor);
+  message_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  message_->SetLineHeight(kLineHeight);
+
   layout->StartRowWithPadding(0, column_set_id, 0, kPadding);
-  layout->AddView(message_);
+  layout->AddView(message_, 2, 1, views::GridLayout::LEADING,
+        views::GridLayout::LEADING);
 
   if (web_contents_) {
-    layout->StartRowWithPadding(0, column_set_id, 0, kPadding);
-    reload_button_ = new views::LabelButton(
+    help_link_ = CreateLink(l10n_util::GetStringUTF16(
+          (kind_ == chrome::SAD_TAB_KIND_CRASHED) ?
+              IDS_SAD_TAB_HELP_LINK : IDS_LEARN_MORE));

[msw, 2015/05/11 18:27:09]

Do both mocks actually have a help link?

[edwardjung, 2015/05/14 18:31:27]

You're correct, only chrome://kill has the help link. Moved to the right
section.

+    help_link_->SetFontList(body_font_list_);
+
+    reload_button_ = new views::BlueButton(
         this,
         l10n_util::GetStringUTF16(IDS_SAD_TAB_RELOAD_LABEL));
-    reload_button_->SetStyle(views::Button::STYLE_BUTTON);
-    // Always render the reload button with chrome style borders; never rely on
-    // native styles.
-    reload_button_->SetBorder(scoped_ptr<views::Border>(
-        new views::LabelButtonBorder(reload_button_->style())));
-    layout->AddView(reload_button_);
-
-    help_link_ = CreateLink(l10n_util::GetStringUTF16(
-        (kind_ == chrome::SAD_TAB_KIND_CRASHED) ?
-            IDS_SAD_TAB_HELP_LINK : IDS_LEARN_MORE));
+    reload_button_->SetFontList(body_font_list_);
+    reload_button_->SetMinSize(gfx::Size(90, 36));
 
     if (kind_ == chrome::SAD_TAB_KIND_CRASHED) {
       size_t offset = 0;
       base::string16 help_text(
           l10n_util::GetStringFUTF16(IDS_SAD_TAB_HELP_MESSAGE,
                                      base::string16(), &offset));
-      views::Label* help_prefix = CreateLabel(help_text.substr(0, offset));
-      views::Label* help_suffix = CreateLabel(help_text.substr(offset));
-
-      const int help_column_set_id = 1;
-      views::ColumnSet* help_columns = layout->AddColumnSet(help_column_set_id);
-      help_columns->AddPaddingColumn(1, kPadding);
-      // Center three middle columns for the help's [prefix][link][suffix].
-      for (size_t column = 0; column < 3; column++)
-        help_columns->AddColumn(views::GridLayout::CENTER,
-            views::GridLayout::CENTER, 0, views::GridLayout::USE_PREF, 0, 0);
-      help_columns->AddPaddingColumn(1, kPadding);
-
-      layout->StartRowWithPadding(0, help_column_set_id, 0, kPadding);
-      layout->AddView(help_prefix);
-      layout->AddView(help_link_);
-      layout->AddView(help_suffix);
+
+      base::string16 link_text =
+          l10n_util::GetStringUTF16(IDS_SAD_TAB_HELP_LINK);

[msw, 2015/05/11 18:27:08]

Why is this different than |help_link_|'s text? Is this right for both pages?

[edwardjung, 2015/05/14 18:31:26]

This is the suggestion link which only appears on chrome://kill  
 
ainslie is suggesting some new text, so this may get consolidated.

+
+      base::string16 help_prefix = help_text.substr(0, offset);
+      base::string16 help_suffix = help_text.substr(offset);
+      base::string16 help_message_string = help_prefix;
+      help_message_string.append(link_text).append(help_suffix);
+
+      views::StyledLabel* help_message_ =
+          new views::StyledLabel(help_message_string, this);
+
+      views::StyledLabel::RangeStyleInfo link_style =
+           views::StyledLabel::RangeStyleInfo::CreateForLink();
+      link_style.font_style = gfx::Font::UNDERLINE;
+      link_style.color = kTextColor;
+
+      views::StyledLabel::RangeStyleInfo normal_style =
+           views::StyledLabel::RangeStyleInfo::CreateForLink();
+      normal_style.font_style = gfx::Font::NORMAL;
+      normal_style.color = kTextColor;
+      normal_style.is_link = false;
+
+      help_message_->SetBaseFontList(body_font_list_);
+      help_message_->SetDefaultStyle(normal_style);
+      help_message_->SetLineHeight(kLineHeight);
+
+      help_message_->AddStyleRange(gfx::Range(help_prefix.length(),
+                                   help_message_string.length() - 1),
+                                   link_style);
+
+      layout->StartRowWithPadding(0, column_set_id, 0, kPadding);
+      layout->AddView(help_message_, 2, 1, views::GridLayout::LEADING,
+          views::GridLayout::TRAILING, kMaxContentWidth, kLabelHeight);

[msw, 2015/05/11 18:27:10]

Will longer translated text be able to wrap and use its needed height?

[edwardjung, 2015/05/14 18:31:27]

You're right, the height isn't enough here for longer text at the most narrow
viewport. Changed to setting the height via SizeToFit() in SadTabView::Layout().

+      layout->StartRowWithPadding(0, column_set_id, 0, kPadding);
+      layout->SkipColumns(1);
     } else {
+      base::string16 feedback_text =
+          l10n_util::GetStringUTF16(IDS_KILLED_TAB_FEEDBACK_LINK);
+      views::StyledLabel* feedback_link_ =

[msw, 2015/05/11 18:27:08]

Why is this not simply a views::Link? There is no ranged style here...
If we need a multi-line link, it should be trivial to extend views::Link to
support multiline via Label.

[edwardjung, 2015/05/14 18:31:27]

I'll do as suggested.

[msw, 2015/05/14 23:50:19]

Did you try this?

+          new views::StyledLabel(feedback_text, this);
+
+      views::StyledLabel::RangeStyleInfo link_style =
+           views::StyledLabel::RangeStyleInfo::CreateForLink();
+      link_style.font_style = gfx::Font::UNDERLINE;
+      link_style.color = kTextColor;
+
+      feedback_link_->SetBaseFontList(body_font_list_);
+      feedback_link_->SetDefaultStyle(link_style);
+      feedback_link_->SetLineHeight(kLineHeight);
+
       layout->StartRowWithPadding(0, column_set_id, 0, kPadding);
-      layout->AddView(help_link_);
+      layout->AddView(feedback_link_, 2, 1, views::GridLayout::LEADING,
+          views::GridLayout::LEADING, kMaxContentWidth, kLabelHeight);
 
-      feedback_link_ = CreateLink(
-          l10n_util::GetStringUTF16(IDS_KILLED_TAB_FEEDBACK_LINK));
       layout->StartRowWithPadding(0, column_set_id, 0, kPadding);
-      layout->AddView(feedback_link_);
+      layout->AddView(help_link_);
     }
+    layout->AddView(reload_button_, 1, 1, views::GridLayout::TRAILING,
+        views::GridLayout::LEADING);
   }
   layout->AddPaddingRow(1, kPadding);
 }
@@ -188,6 +274,25 @@ void SadTabView::LinkClicked(views::Link* source, int event_flags) {
   }
 }
 
+void SadTabView::StyledLabelLinkClicked(const gfx::Range& range,

[msw, 2015/05/11 18:27:09]

Ugh StyledLabel... This callback doesn't even specify what StyledLabel was
clicked, it must be recorded elsewhere or assumed, and the gfx::Range is useless
without knowing the text... I really want to destroy/rewrite StyledLabel.

+                                        int event_flags) {
+

[msw, 2015/05/11 18:27:09]

Remove blank line.

[edwardjung, 2015/05/14 18:31:26]

Done.

+  // Will this work for i18n other languages in RTL for example?

[msw, 2015/05/11 18:27:09]

I doubt it... this does not seem acceptable to me.

[edwardjung, 2015/05/14 18:31:26]

Removed this check as there is now only a single instance of StyledLabel.

+  if (range.start() > 0) {
+    GURL help_url((kind_ == chrome::SAD_TAB_KIND_CRASHED) ?

[msw, 2015/05/11 18:27:09]

Don't copy this code... refactor it to be used here and above.

[edwardjung, 2015/05/14 18:31:26]

Done.

[msw, 2015/05/14 23:50:19]

Not done, afaict.

[edwardjung, 2015/05/18 12:02:35]

Misunderstood.

+        chrome::kCrashReasonURL : chrome::kKillReasonURL);
+    OpenURLParams params(
+        help_url, content::Referrer(), CURRENT_TAB,
+        ui::PAGE_TRANSITION_LINK, false);
+    web_contents_->OpenURL(params);
+  } else if (range.start() == 0) {
+    chrome::ShowFeedbackPage(
+        chrome::FindBrowserWithWebContents(web_contents_),
+        l10n_util::GetStringUTF8(IDS_KILLED_TAB_FEEDBACK_MESSAGE),
+        std::string(kCategoryTagCrash));
+  }
+}
+
 void SadTabView::ButtonPressed(views::Button* sender,
                                const ui::Event& event) {
   DCHECK(web_contents_);
@@ -259,7 +364,7 @@ void SadTabView::Close() {
 }
 
 views::Label* SadTabView::CreateLabel(const base::string16& text) {
-  views::Label* label = new views::Label(text);
+  views::Label* label = new views::LabelWithMaxWidth(text);
   label->SetBackgroundColor(background()->get_color());
   label->SetEnabledColor(kTextColor);
   return label;