Put alert dialog text in a scroll view.

ui/views/controls/message_box_view.cc

Index: ui/views/controls/message_box_view.cc
diff --git a/ui/views/controls/message_box_view.cc b/ui/views/controls/message_box_view.cc
index 671ca6194ec432baa908efb7eabbc6a81f50f297..15bb9c8ebfec77b515644283a108ea43ab749026 100644
--- a/ui/views/controls/message_box_view.cc
+++ b/ui/views/controls/message_box_view.cc
@@ -15,7 +15,10 @@
 #include "ui/views/controls/image_view.h"

[msw, 2014/07/09 15:56:59]

nit: remove this

[meacer, 2014/07/09 18:06:56]

Done.

 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
+#include "ui/views/controls/scroll_view.h"
+#include "ui/views/controls/scrollbar/overlay_scroll_bar.h"
 #include "ui/views/controls/textfield/textfield.h"
+#include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/layout/layout_constants.h"
 #include "ui/views/widget/widget.h"
@@ -76,7 +79,6 @@ MessageBoxView::InitParams::~InitParams() {
 
 MessageBoxView::MessageBoxView(const InitParams& params)
     : prompt_field_(NULL),
-      icon_(NULL),
       checkbox_(NULL),
       link_(NULL),
       message_width_(params.message_width) {
@@ -93,14 +95,6 @@ bool MessageBoxView::IsCheckBoxSelected() {
   return checkbox_ ? checkbox_->checked() : false;
 }
 
-void MessageBoxView::SetIcon(const gfx::ImageSkia& icon) {
-  if (!icon_)
-    icon_ = new ImageView();
-  icon_->SetImage(icon);
-  icon_->SetBounds(0, 0, icon.width(), icon.height());
-  ResetLayoutManager();
-}
-
 void MessageBoxView::SetCheckBoxLabel(const base::string16& label) {
   if (!checkbox_)
     checkbox_ = new Checkbox(label);
@@ -209,20 +203,10 @@ void MessageBoxView::ResetLayoutManager() {
   GridLayout* layout = GridLayout::CreatePanel(this);
   SetLayoutManager(layout);
 
-  gfx::Size icon_size;
-  if (icon_)
-    icon_size = icon_->GetPreferredSize();
-
   // Add the column set for the message displayed at the top of the dialog box.
   // And an icon, if one has been set.

[msw, 2014/07/09 15:56:59]

nit: remove this part of the comment

[meacer, 2014/07/09 18:06:56]

Done.

   const int message_column_view_set_id = 0;
   ColumnSet* column_set = layout->AddColumnSet(message_column_view_set_id);
-  if (icon_) {
-    column_set->AddColumn(GridLayout::LEADING, GridLayout::LEADING, 0,
-                          GridLayout::FIXED, icon_size.width(),
-                          icon_size.height());
-    column_set->AddPaddingColumn(0, kUnrelatedControlHorizontalSpacing);
-  }
   column_set->AddColumn(GridLayout::FILL, GridLayout::FILL, 1,
                         GridLayout::FIXED, message_width_, 0);
 
@@ -230,24 +214,22 @@ void MessageBoxView::ResetLayoutManager() {
   const int extra_column_view_set_id = 1;
   if (prompt_field_ || checkbox_ || link_) {
     column_set = layout->AddColumnSet(extra_column_view_set_id);
-    if (icon_) {
-      column_set->AddPaddingColumn(
-          0, icon_size.width() + kUnrelatedControlHorizontalSpacing);
-    }
     column_set->AddColumn(GridLayout::FILL, GridLayout::FILL, 1,
                           GridLayout::USE_PREF, 0, 0);
   }
 
-  for (size_t i = 0; i < message_labels_.size(); ++i) {
-    layout->StartRow(i, message_column_view_set_id);
-    if (icon_) {
-      if (i == 0)
-        layout->AddView(icon_);
-      else
-        layout->SkipColumns(1);
-    }
-    layout->AddView(message_labels_[i]);
-  }
+  const int kMaxScrollViewHeight = 600;

[sky, 2014/07/09 16:20:15]

Why do you need to clip this? Wouldn't it be better to use work area?

[meacer, 2014/07/09 18:06:56]

The clipping + overlay scroll bar gave a useful combination where both the
dialog was constrained to a maximum height and the scroll bar was visible when
needed.

It looks like the scroll contents aren't visible at all when the view isn't
clipped. I don't have much expertise with the views, so maybe I'm missing
something?

[sky, 2014/07/09 23:21:08]

'Clip' was a bad choice on my side. I'm suggesting you use the work area of the
monitor you're on and not an arbitrary value.
ScrollView is a bit odd, you may need to explicitly set the bounds of
message_contents.

[meacer, 2014/07/10 00:27:53]

The message view is constrained by the browser window, which means I'll need to
scale according to the browser window and not the whole screen, and will have to
pass the browser window to MessageView. Does that sound reasonable?

[msw, 2014/07/10 00:34:58]

You may be able to override MessageBoxView::GetMinimumSize (and ditto for the
relevant DialogDelegate?) to let the dialog be sized automatically to fit within
the browser window (kind of like CollectedCookiesViews, which albeit isn't
perfect...).

[meacer, 2014/07/10 19:36:10]

I've tried a couple of approaches, and none seem to work other than using
ClipHeightTo with a pixel value:

- Overriding GetMinimumSize doesn't clamp the height to the browser window.
- message_contents_->SetBoundsRect doesn't set a height for the scroll view.
- NativeScrollBar doesn't show even when I set message_contents_->SetBoundsRect.

+  views::View* message_contents = new views::View();
+  message_contents->SetLayoutManager(
+      new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0));
+  for (size_t i = 0; i < message_labels_.size(); ++i)
+    message_contents->AddChildView(message_labels_[i]);
+  ScrollView* scroll_view = new views::ScrollView();
+  scroll_view->SetVerticalScrollBar(new views::OverlayScrollBar(false));

[sky, 2014/07/09 16:20:15]

Why are you going with overlay?

[meacer, 2014/07/09 18:06:56]

Oddly, NativeScrollBar doesn't show for me at all, and Kennedy looked a bit too
thick and I didn't see it used elsewhere so wanted to play safe. Happy to change
it if you feel otherwise though.

[sky, 2014/07/09 23:21:08]

You should go with the default. The NativeScrollBar should only show if needed,
although it may vary depending upon platform (NativeTheme).

+  scroll_view->ClipHeightTo(0, kMaxScrollViewHeight);
+  scroll_view->SetContents(message_contents);
+  layout->StartRow(0, message_column_view_set_id);
+  layout->AddView(scroll_view);
 
   if (prompt_field_) {
     layout->AddPaddingRow(0, inter_row_vertical_spacing_);