[Doc] Add views layout best practices

Author: pkasting@chromium.org
Examples: tluk@chromium.org
Markdown: weili@chromium.org
Bug: none
Change-Id: Ibb81c34929b33479124d49aa2654468fc89564bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2224604
Commit-Queue: Wei Li <weili@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774840}

docs/ui/learn/bestpractices/layout.md

+# Best practice: layout
+
+The most important principles when working with Views layout are abstraction
+and encapsulation. The goal of the guidance here is to split the broad work of
+"layout" into many discrete pieces, each of which can be easily understood,
+tested, and changed in isolation. Compliant code should be more readable,
+performant, and maintainable; more adaptable to future modifications; and more
+stylistically consistent with other UI elements both now and in the future.
+
+[TOC]
+
+## Express layout values logically
+
+Both in mocks and code, **layout values should be described in functional terms
+that conform to the relevant specs** (particularly the
+[Material Refresh][] and older [Harmony][] specs). Rather than a mock saying
+a dialog title is "15 pt high", it should say it is the "Title 1" or
+"DIALOG\_TITLE" style; two non-interacting controls on the same line should not
+be separated by "16 dip" but by [`DISTANCE_UNRELATED_CONTROL_HORIZONTAL`][].
+Designers and engineers can reference a [dictionary][] to agree on common
+terminology. Don't simply back-figure the constant to use based on what
+produces the same value as the mock, as future design system changes will
+cause subtle and confusing bugs. Similarly, don't hardcode designer-provided
+values that aren't currently present in Chrome, as the semantics of such
+one-off values are unclear and will cause maintenance problems.
+Work with the Toolkit and UX teams to modify the design and overall spec so
+the desired results can be achieved in a centralized way.
+
+Note: the concept in this section is general, but the linked specs are
+**Googlers Only**.
+
+[Material Refresh]: https://docs.google.com/presentation/d/1EO7TOpIMJ7QHjaTVw9St-q6naKwtXX2TwzMirG5EsKY/edit#slide=id.g3232c09376_6_794
+[Harmony]: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20%28MD%29/Secondary%20UI%20Previews%20and%20specs%20%28exports%29/Spec
+[`DISTANCE_UNRELATED_CONTROL_HORIZONTAL`]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/chrome_layout_provider.h;l=56;drc=ec62c9ac3ef71a7014e27c5d2cf98917a89e3524
+[dictionary]: http://go/DesktopDictionary
+
+## Obtain layout values from provider objects
+
+Once layout styles and values are expressed functionally, **the exact values
+should be obtained from relevant provider objects, not computed directly.**
+Code should not have any magic numbers for sizes, positions, elevations, and
+the like; this includes transformations like scaling values up or down by a
+fraction, or tweaking a provided value by a few DIPs. The most commonly used
+provider object is the [`LayoutProvider`][] (or its `chrome`/-side extension,
+[`ChromeLayoutProvider`][]); `View`s can obtain the global instance via a
+relevant [`Get()`][] method and ask it for relevant [distances][], [insets][],
+[corner radii][], [shadow elevations][], and the like. For text-setting
+controls like [`Label`][], the [`TypographyProvider`][] (or its
+`chrome`/-side extension, [`ChromeTypographyProvider`]),
+usually accessed via [global helper functions][], can provide appropriate
+[fonts][], [colors][], and [line heights][]. Most `View`s should not use these
+directly, but use `Label` and other such controls, providing the appropriate
+[context][] and [style][].
+
+[`LayoutProvider`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/layout_provider.h;l=129;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38
+[`ChromeLayoutProvider`]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/chrome_layout_provider.h;drc=ec62c9ac3ef71a7014e27c5d2cf98917a89e3524;l=76
+[`Get()`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/layout_provider.h;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38;l=135
+[distances]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/layout_provider.h;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38;l=148
+[insets]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/layout_provider.h;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38;l=144
+[corner radii]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/layout_provider.h;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38;l=168
+[shadow elevations]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/layout_provider.h;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38;l=172
+[`Label`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/controls/label.h;l=30;drc=59135b4042aa469752899e8e4bf2a0a81d3d320c
+[`TypographyProvider`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/style/typography_provider.h;l=22;drc=b5e29e075e814ed41e6727c281b69f797d8a1e10
+[`ChromeTypographyProvider`]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/chrome_typography_provider.h;l=13;drc=a7ee000c95842e2dce6397ca36926924f4cb322b
+[global helper functions]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/style/typography.h;l=109;drc=8f7db479018a99e5906876954de93ae6d23bee58
+[fonts]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/style/typography_provider.h;l=28;drc=b5e29e075e814ed41e6727c281b69f797d8a1e10
+[colors]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/style/typography_provider.h;l=32;drc=b5e29e075e814ed41e6727c281b69f797d8a1e10
+[line heights]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/style/typography_provider.h;l=37;drc=b5e29e075e814ed41e6727c281b69f797d8a1e10
+[context]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/style/typography.h;l=23;drc=8f7db479018a99e5906876954de93ae6d23bee58
+[style]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/style/typography.h;l=67;drc=8f7db479018a99e5906876954de93ae6d23bee58
+
+|||---|||
+
+#####
+
+**Avoid**
+
+[Current code][1] uses file scoped hard-coded padding values for its layout
+constants.
+
+[1]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/subtle_notification_view.cc;l=142;drc=787d0aacc071674dc83f6059072d15f8cfffbf84
+
+#####
+
+**Best practice**
+
+A better approach would be to use layout constants sourced from the
+[`ChromeLayoutProvider`][].
+
+|||---|||
+
+|||---|||
+
+#####
+
+``` cpp
+namespace {
+// Space between the site info label.
+const int kMiddlePaddingPx = 30;
+
+const int kOuterPaddingHorizPx = 40;
+const int kOuterPaddingVertPx = 8;
+} // namespace
+
+SubtleNotificationView::SubtleNotificationView()
+    : instruction_view_(nullptr) {
+  ...
+  instruction_view_ =
+      new InstructionView(base::string16());
+
+  int outer_padding_horiz = kOuterPaddingHorizPx;
+  int outer_padding_vert = kOuterPaddingVertPx;
+  AddChildView(instruction_view_);
+
+  SetLayoutManager(std::make_unique<views::BoxLayout>(
+      views::BoxLayout::Orientation::kHorizontal,
+      gfx::Insets(outer_padding_vert,
+                  outer_padding_horiz),
+      kMiddlePaddingPx));
+}
+```
+
+#####
+
+``` cpp
+
+
+
+
+
+
+
+
+SubtleNotificationView::SubtleNotificationView()
+    : instruction_view_(nullptr) {
+  ...
+  AddChildView(std::make_unique<InstructionView>(
+      base::string16()));
+
+  const gfx::Insets kDialogInsets =
+      ChromeLayoutProvider::Get()->GetInsetsMetric(
+      views::INSETS_DIALOG);
+  const int kHorizontalPadding =
+      ChromeLayoutProvider::Get()->GetDistanceMetric(
+          views::DISTANCE_RELATED_LABEL_HORIZONTAL);
+  SetLayoutManager(std::make_unique<views::BoxLayout>(
+      views::BoxLayout::Orientation::kHorizontal,
+      kDialogInsets, kHorizontalPadding));
+}
+```
+
+|||---|||
+
+
+## Use hierarchy liberally
+
+While not a layout-specific tactic, it simplifies many layout issues
+**to break a high-level UI construct into a hierarchy of `View`s**,
+with as many levels as necessary to make each `View` as simple as possible.
+In such hierarchies, most non-leaf `View`s will be nameless "containers" that
+simply size or group their immediate children, perhaps with padding between
+them or a margin around the outside. Each such `View` is easy to lay out,
+and you can later combine or factor out pieces of the hierarchy as appropriate,
+including adding helpers for common Material Design idioms to the core toolkit.
+
+
+## Use LayoutManagers
+
+**Avoid overriding [`Layout()`][] to programmatically lay out children.**
+In nearly all cases, the built-in [`LayoutManager`][]s can achieve the desired
+layout, and do so in a declarative rather than imperative fashion. The
+resulting code is often simpler and easier to understand. Writing a [bespoke
+`LayoutManager`][] is also possible, but less common.
+
+[`Layout()`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=730;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38
+[`LayoutManager`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/layout_manager.h;l=33;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38
+[bespoke `LayoutManager`]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/try_chrome_dialog_win/button_layout.h;l=30;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38
+
+|||---|||
+
+#####
+
+**Avoid**
+
+The following old code used Layout() to have its label text fill the dialog.
+
+#####
+
+**Best practice**
+
+[Current code][2] uses a [FillLayout][] to achieve the same result.
+
+[2]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/relaunch_notification/relaunch_required_dialog_view.cc;l=91;drc=1ec33e7c19e2d63b3f918df115c12f77f419645b
+[FillLayout]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/fill_layout.h
+
+|||---|||
+
+|||---|||
+
+#####
+
+``` cpp
+int RelaunchRequiredDialogView::GetHeightForWidth(
+    int width) const {
+  const gfx::Insets insets = GetInsets();
+  return body_label_->GetHeightForWidth(
+      width - insets.width()) + insets.height();
+}
+
+void RelaunchRequiredDialogView::Layout() {
+  body_label_->SetBoundsRect(GetContentsBounds());
+}
+```
+
+#####
+
+``` cpp
+RelaunchRequiredDialogView::RelaunchRequiredDialogView(
+    base::Time deadline,
+    base::RepeatingClosure on_accept)
+    : ...{
+  SetLayoutManager(
+      std::make_unique<views::FillLayout>());
+  ...
+}
+
+
+```
+
+|||---|||
+
+
+## Prefer intrinsic constraints to extrinsic computation
+
+Where possible, **express the desired outcome of layout in terms of intrinsic
+constraints for each `View`,** instead of trying to conditionally compute
+the desired output metrics. For example, using a [`ClassProperty`][]
+to set each child's [margins][] is less error-prone than trying to
+conditionally add padding `View`s between children. When coupled with
+[margin collapsing][] and [internal padding][], it's possible to do things
+like use [different padding amounts between different children][]
+or visually align elements without manually computing offsets.
+Such manual computation is prone to bugs if someone changes a size, padding
+value, or child order in one place without also updating related computations
+elsewhere.
+
+[`ClassProperty`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/base/class_property.h;l=55;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38
+[margins]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view_class_properties.h;l=30;drc=1449b8c60358c4cdea1722e4c1e8079bd1b5f306
+[margin collapsing]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/flex_layout.h;l=87;drc=62bf27aca5418212ceadd8daf9188d2aa437bfcc
+[internal padding]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view_class_properties.h;l=40;drc=1449b8c60358c4cdea1722e4c1e8079bd1b5f306
+[different padding amounts between different children]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/toolbar/toolbar_view.cc;l=974;drc=34a8c4215229379ced3586125399c7ad3c65b87f
+
+|||---|||
+
+#####
+
+**Avoid**
+
+The following is old code that calculated bubble padding through calculations
+involving the control insets.
+
+#####
+
+**Best practice**
+
+[Current code][3] uses a combination of margin and padding on the
+ColorPickerView to ensure proper alignment.
+
+[3]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/tabs/tab_group_editor_bubble_view.cc;l=89;drc=542c4c6ac89bc665807351d3fb4aca5ebddc82f8
+
+|||---|||
+
+|||---|||
+
+#####
+
+``` cpp
+TabGroupEditorBubbleView::TabGroupEditorBubbleView(
+    const Browser* browser,
+    const tab_groups::TabGroupId& group,
+    TabGroupHeader* anchor_view,
+    base::Optional<gfx::Rect> anchor_rect,
+    bool stop_context_menu_propagation)
+    : ... {
+
+  ...
+  const auto* layout_provider =
+      ChromeLayoutProvider::Get();
+  const int horizontal_spacing =
+      layout_provider->GetDistanceMetric(
+          views::DISTANCE_RELATED_CONTROL_HORIZONTAL);
+  const int vertical_menu_spacing =
+      layout_provider->GetDistanceMetric(
+          views::DISTANCE_RELATED_CONTROL_VERTICAL);
+
+  // The vertical spacing for the non menu items within
+  // the editor bubble.
+  const int vertical_dialog_content_spacing = 16;
+
+
+
+
+
+
+
+
+
+
+  views::View* group_modifier_container =
+      AddChildView(std::make_unique<views::View>());
+
+  gfx::Insets color_element_insets =
+      ChromeLayoutProvider::Get()->GetInsetsMetric(
+          views::INSETS_VECTOR_IMAGE_BUTTON);
+  group_modifier_container->SetBorder(
+      views::CreateEmptyBorder(
+          gfx::Insets(vertical_dialog_content_spacing,
+              horizontal_spacing -
+                  color_element_insets.left(),
+              vertical_dialog_content_spacing,
+              horizontal_spacing -
+                  color_element_insets.right())));
+  ...
+  // Add the text field for editing the title.
+  views::View* title_field_container =
+      group_modifier_container->AddChildView(
+          std::make_unique<views::View>());
+  title_field_container->SetBorder(
+      views::CreateEmptyBorder(gfx::Insets(
+          0, color_element_insets.left(),
+          vertical_dialog_content_spacing,
+          color_element_insets.right()))
+  ...
+  color_selector_ =
+    group_modifier_container->AddChildView(
+        std::make_unique<ColorPickerView>(
+            colors_, background_color(), initial_color,
+            base::Bind(
+                &TabGroupEditorBubbleView::UpdateGroup,
+                base::Unretained(this))));
+  ...
+}
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+```
+
+#####
+
+``` cpp
+TabGroupEditorBubbleView::TabGroupEditorBubbleView(
+    const Browser* browser,
+    const tab_groups::TabGroupId& group,
+    views::View* anchor_view,
+    base::Optional<gfx::Rect> anchor_rect,
+    TabGroupHeader* header_view,
+    bool stop_context_menu_propagation)
+    : ... {
+  ...
+  const auto* layout_provider =
+      ChromeLayoutProvider::Get();
+  const int horizontal_spacing =
+      layout_provider->GetDistanceMetric(
+          views::DISTANCE_RELATED_CONTROL_HORIZONTAL);
+  const int vertical_spacing =
+      layout_provider->GetDistanceMetric(
+          views::DISTANCE_RELATED_CONTROL_VERTICAL);
+
+  // The padding of the editing controls is adaptive,
+  // to improve the hit target size and screen real
+  // estate usage on touch devices.
+  const int group_modifier_vertical_spacing =
+      ui::TouchUiController::Get()->touch_ui() ?
+          vertical_spacing / 2 : vertical_spacing;
+  const gfx::Insets control_insets =
+      ui::TouchUiController::Get()->touch_ui()
+          ? gfx::Insets(5 * vertical_spacing / 4,
+            horizontal_spacing)
+          : gfx::Insets(vertical_spacing,
+                        horizontal_spacing);
+
+  views::View* group_modifier_container =
+      AddChildView(std::make_unique<views::View>());
+  group_modifier_container->SetBorder(
+      views::CreateEmptyBorder(gfx::Insets(
+          group_modifier_vertical_spacing, 0)));
+
+  views::FlexLayout* group_modifier_container_layout =
+      group_modifier_container->SetLayoutManager(
+          std::make_unique<views::FlexLayout>());
+  group_modifier_container_layout
+      ->SetOrientation(
+          views::LayoutOrientation::kVertical)
+      .SetIgnoreDefaultMainAxisMargins(true);
+
+
+  // Add the text field for editing the title.
+  views::View* title_field_container =
+      group_modifier_container->AddChildView(
+          std::make_unique<views::View>());
+  title_field_container->SetBorder(
+      views::CreateEmptyBorder(
+          control_insets.top(), control_insets.left(),
+          group_modifier_vertical_spacing,
+          control_insets.right()));
+
+  ...
+  const tab_groups::TabGroupColorId initial_color_id =
+      InitColorSet();
+  color_selector_ =
+      group_modifier_container->AddChildView(
+          std::make_unique<ColorPickerView>(
+              this, colors_, initial_color_id,
+              base::Bind(
+                &TabGroupEditorBubbleView::UpdateGroup,
+                base::Unretained(this))));
+  color_selector_->SetProperty(
+      views::kMarginsKey,
+      gfx::Insets(0, control_insets.left(), 0,
+                  control_insets.right()));
+  ...
+}
+
+ColorPickerView::ColorPickerView(
+    const views::BubbleDialogDelegateView* bubble_view,
+    const TabGroupEditorBubbleView::Colors& colors,
+    tab_groups::TabGroupColorId initial_color_id,
+    ColorSelectedCallback callback)
+    : callback_(std::move(callback)) {
+  ...
+  // Set the internal padding to be equal to the
+  // horizontal insets of a color picker element,
+  // since that is the amount by which the color
+  // picker's margins should be adjusted to make it
+  // visually align with other controls.
+  gfx::Insets child_insets = elements_[0]->GetInsets();
+  SetProperty(views::kInternalPaddingKey,
+              gfx::Insets(0, child_insets.left(), 0,
+                          child_insets.right()));
+}
+```
+
+|||---|||
+
+## Use GridLayout with caution
+
+[`GridLayout`][] is a `LayoutManager` used for tabular layouts. Much like
+table-based layout in HTML, it can achieve almost any desired effect, and in
+some scenarios (e.g.  creating an actual table) is the best tool. Used
+indiscriminately, it can be cryptic, verbose, and error-prone. Accordingly,
+**use `GridLayout` only when creating a true grid, not simply for selective
+horizontal and vertical alignment.** For simple layouts, [`BoxLayout`][] and
+[`FlexLayout`][] are better choices; for more complex layouts, representing
+sections or groups hierarchically may result in simpler inner layouts that
+can be nested within an overall layout.
+
+[`GridLayout`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/grid_layout.h;l=79;drc=8cab3382ac9b70b7ecfe29ae03b1b7ee8f4e01fa
+[`BoxLayout`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/box_layout.h;l=28;drc=5b9e43d976aca377588875fc59c5348ede02a8b5
+[`FlexLayout`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/layout/flex_layout.h;l=73;drc=62bf27aca5418212ceadd8daf9188d2aa437bfcc
+
+|||---|||
+
+#####
+
+**Avoid**
+
+The following old code uses a [`GridLayout`][] to create a HoverButton with
+a stacked title and subtitle flanked on by views on both sides.
+
+#####
+
+**Best practice**
+
+[Current code][4] uses [`FlexLayout`][] to achieve the desired result, resulting
+in clearer code.
+
+[4]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/hover_button.cc;l=106;drc=888af74006ea1c4ee9907d18c8df2a7ca424eab9
+
+|||---|||
+
+|||---|||
+
+#####
+
+``` cpp
+// Used to create the following layout
+// +-----------+---------------------+----------------+
+// | icon_view | title               | secondary_view |
+// +-----------+---------------------+----------------+
+// |           | subtitle            |                |
+// +-----------+---------------------+----------------+
+HoverButton::HoverButton(
+    views::ButtonListener* button_listener,
+    std::unique_ptr<views::View> icon_view,
+    const base::string16& title,
+    const base::string16& subtitle,
+    std::unique_ptr<views::View> secondary_view,
+    bool resize_row_for_secondary_view,
+    bool secondary_view_can_process_events) {
+  ...
+  views::GridLayout* grid_layout =
+      SetLayoutManager(
+          std::make_unique<views::GridLayout>());
+  ...
+  constexpr int kColumnSetId = 0;
+  views::ColumnSet* columns =
+      grid_layout->AddColumnSet(kColumnSetId);
+  columns->AddColumn(
+      views::GridLayout::CENTER,
+      views::GridLayout::CENTER,
+      views::GridLayout::kFixedSize,
+      views::GridLayout::USE_PREF, 0, 0);
+  columns->AddPaddingColumn(
+      views::GridLayout::kFixedSize,
+      icon_label_spacing);
+  columns->AddColumn(
+      views::GridLayout::FILL,
+      views::GridLayout::FILL,
+      1.0, views::GridLayout::USE_PREF, 0, 0);
+  ...
+  grid_layout->StartRow(
+      views::GridLayout::kFixedSize, kColumnSetId,
+      row_height);
+  icon_view_ = grid_layout->AddView(
+      std::move(icon_view), 1, num_labels);
+  ...
+  auto title_wrapper =
+      std::make_unique<SingleLineStyledLabelWrapper>(
+          title);
+  title_ = title_wrapper->label();
+  grid_layout->AddView(std::move(title_wrapper));
+
+  if (secondary_view) {
+    columns->AddColumn(
+        views::GridLayout::CENTER,
+        views::GridLayout::CENTER,
+        views::GridLayout::kFixedSize,
+        views::GridLayout::USE_PREF, 0, 0);
+    ...
+    secondary_view_ =
+        grid_layout->AddView(
+            std::move(secondary_view), 1, num_labels);
+    ...
+  }
+  if (!subtitle.empty()) {
+    grid_layout->StartRow(
+        views::GridLayout::kFixedSize, kColumnSetId,
+        row_height);
+    auto subtitle_label =
+        std::make_unique<views::Label>(
+            subtitle, views::style::CONTEXT_BUTTON,
+            views::style::STYLE_SECONDARY);
+    ...
+    grid_layout->SkipColumns(1);
+    subtitle_ =
+        grid_layout->AddView(std::move(subtitle_label));
+  }
+  ...
+}
+```
+
+#####
+
+``` cpp
+// Used to create the following layout
+// +-----------+---------------------+----------------+
+// |           | title               |                |
+// | icon_view |---------------------| secondary_view |
+// |           | subtitle            |                |
+// +-----------+---------------------+----------------+
+HoverButton::HoverButton(
+    views::ButtonListener* button_listener,
+    std::unique_ptr<views::View> icon_view,
+    const base::string16& title,
+    const base::string16& subtitle,
+    std::unique_ptr<views::View> secondary_view,
+    bool resize_row_for_secondary_view,
+    bool secondary_view_can_process_events) {
+  ...
+  SetLayoutManager(
+      std::make_unique<views::FlexLayout>())
+      ->SetCrossAxisAlignment(
+          views::LayoutAlignment::kCenter)
+      .SetChildViewIgnoredByLayout(
+          ink_drop_container(), true);
+  ...
+  icon_view_ =
+    AddChildView(std::make_unique<IconWrapper>(
+        std::move(icon_view), vertical_spacing))
+    ->icon();
+
+  // |label_wrapper| will hold both the title and
+  // subtitle if it exists.
+  auto label_wrapper = std::make_unique<views::View>();
+  title_ = label_wrapper->AddChildView(
+      std::make_unique<views::StyledLabel>(
+          title, nullptr));
+
+  if (!subtitle.empty()) {
+    auto subtitle_label =
+        std::make_unique<views::Label>(
+            subtitle, views::style::CONTEXT_BUTTON,
+            views::style::STYLE_SECONDARY);
+    ...
+    subtitle_ = label_wrapper->AddChildView(
+        std::move(subtitle_label));
+  }
+
+  label_wrapper->SetLayoutManager(
+      std::make_unique<views::FlexLayout>())
+      ->SetOrientation(
+          views::LayoutOrientation::kVertical)
+      .SetMainAxisAlignment(
+          views::LayoutAlignment::kCenter);
+  label_wrapper->SetProperty(
+      views::kFlexBehaviorKey,
+      views::FlexSpecification(
+          views::MinimumFlexSizeRule::kScaleToZero,
+          views::MaximumFlexSizeRule::kUnbounded));
+  label_wrapper->SetProperty(
+      views::kMarginsKey,
+      gfx::Insets(vertical_spacing, 0));
+  label_wrapper_ =
+      AddChildView(std::move(label_wrapper));
+  ...
+
+  if (secondary_view) {
+    ...
+    secondary_view->SetProperty(
+        views::kMarginsKey,
+        gfx::Insets(secondary_spacing,
+                    icon_label_spacing,
+                    secondary_spacing, 0));
+    secondary_view_ =
+        AddChildView(std::move(secondary_view));
+  }
+  ...
+}
+```
+
+|||---|||
+
+## Compute preferred/minimum sizes recursively from children
+
+**Avoid hardcoding preferred or minimum sizes,** including via metrics like
+[`DISTANCE_BUBBLE_PREFERRED_WIDTH`][].
+In many cases, `LayoutManager`s will provide reasonable values for these,
+and common codepaths like [`BubbleFrameView::GetFrameWidthForClientWidth()`][]
+can help ensure that the returned values are [conformed to spec][].
+When a `View` does need to calculate these manually, it should do so based on
+the corresponding values returned by its children, not by returning specific
+numbers (e.g. dialog preferred size is 300 by 150). In particular, assuming
+fonts will be in a certain size, or that a given fixed area is sufficient to
+display all necessary information, can cause hard-to-find localization and
+accessibility bugs for users with verbose languages or unusually large fonts.
+
+[`DISTANCE_BUBBLE_PREFERRED_WIDTH`]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/chrome_layout_provider.h;l=68;drc=ec62c9ac3ef71a7014e27c5d2cf98917a89e3524
+[`BubbleFrameView::GetFrameWidthForClientWidth()`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/bubble/bubble_frame_view.cc;l=688;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38
+[conformed to spec]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/bubble/bubble_frame_view.cc;l=698;drc=f5df5da5753795298349b2dd6325e2c5e6e13e38
+
+|||---|||
+
+#####
+
+**Avoid**
+
+Current code overloads CalculatePreferredSize() in the dialog view.
+
+#####
+
+**Best practice**
+
+A better approach would be to omit the overload completely and let leaf views
+size the dialog appropriately, relying on the minimum size fallbacks
+if necessary.
+
+|||---|||
+
+|||---|||
+
+#####
+
+``` cpp
+...
+gfx::Size
+CastDialogView::CalculatePreferredSize() const {
+  const int width =
+      ChromeLayoutProvider::Get()->GetDistanceMetric(
+          DISTANCE_BUBBLE_PREFERRED_WIDTH);
+  return gfx::Size(width, GetHeightForWidth(width));
+}
+```
+
+#####
+
+``` cpp
+...
+```
+
+|||---|||
+
+
+## Handle events directly, not via Layout()
+
+In addition to using `LayoutManager`s in place of manual layout,
+**avoid overriding `Layout()` to perform non-layout actions.** For example,
+instead of updating properties tied to a `View`'s size in `Layout()`,
+do so in [`OnBoundsChanged()`][];
+when the `View` in question is a child, make the child a `View` subclass
+with an `OnBoundsChanged()` override instead of having the parent both lay
+the child out and update its properties. Modify the
+[hit-testing and event-handling functions][] directly instead of laying out
+invisible `View`s to intercept events. Toggle child visibility directly in
+response to external events rather than calculating it inside `Layout()`.
+
+[`OnBoundsChanged()`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=1377;drc=34a8c4215229379ced3586125399c7ad3c65b87f
+[hit-testing and event-handling functions]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=919;drc=34a8c4215229379ced3586125399c7ad3c65b87f
+
+|||---|||
+
+#####
+
+**Avoid**
+
+Old code updated hit testing and button properties in the Layout() method.
+
+#####
+
+**Best practice**
+
+Current code wraps the buttons in a file scoped class with an
+OnBoundsChanged() method and modifies the hit testing functions directly to
+achieve the same result.
+
+|||---|||
+
+|||---|||
+
+#####
+
+``` cpp
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+FindBarView::FindBarView(FindBarHost* host)
+    : find_bar_host_(host) {
+  auto find_text = std::make_unique<views::Textfield>();
+  find_text_ = AddChildView(std::move(find_text));
+  ...
+  auto find_previous_button =
+      views::CreateVectorImageButton(this);
+  find_previous_button_ =
+      AddChildView(std::move(find_previous_button));
+  ...
+  auto find_next_button =
+      views::CreateVectorImageButton(this);
+  find_next_button_ =
+      AddChildView(std::move(find_next_button));
+  ...
+  auto close_button =
+      views::CreateVectorImageButton(this);
+  close_button_ =
+      AddChildView(std::move(close_button));
+}
+
+void FindBarView::Layout() {
+  views::View::Layout();
+  // The focus forwarder view is a hidden view that
+  // should cover the area between the find text box
+  // and the find button so that when the user clicks
+  // in that area we focus on the find text box.
+  const int find_text_edge =
+      find_text_->x() + find_text_->width();
+  focus_forwarder_view_->SetBounds(
+      find_text_edge, find_previous_button_->y(),
+      find_previous_button_->x() - find_text_edge,
+      find_previous_button_->height());
+
+  for (auto* button :
+       {find_previous_button_, find_next_button_,
+        close_button_}) {
+    constexpr int kCircleDiameterDp = 24;
+    auto highlight_path = std::make_unique<SkPath>();
+    // Use a centered circular shape for inkdrops and
+    // focus rings.
+    gfx::Rect circle_rect(button->GetLocalBounds());
+    circle_rect.ClampToCenteredSize(
+        gfx::Size(kCircleDiameterDp,
+                  kCircleDiameterDp));
+    highlight_path->addOval(
+        gfx::RectToSkRect(circle_rect));
+    button->SetProperty(views::kHighlightPathKey,
+                        highlight_path.release());
+  }
+}
+```
+
+#####
+
+``` cpp
+// An ImageButton that has a centered circular
+// highlight.
+class FindBarView::FindBarButton
+    : public views::ImageButton {
+ public:
+  using ImageButton::ImageButton;
+ protected:
+  void OnBoundsChanged(
+      const gfx::Rect& previous_bounds) override {
+    const gfx::Rect bounds = GetLocalBounds();
+    auto highlight_path = std::make_unique<SkPath>();
+    const gfx::Point center = bounds.CenterPoint();
+    const int radius = views::LayoutProvider::Get()
+        ->GetCornerRadiusMetric(
+            views::EMPHASIS_MAXIMUM, bounds.size());
+    highlight_path->addCircle(
+        center.x(), center.y(), radius);
+    SetProperty(views::kHighlightPathKey,
+                highlight_path.release());
+  }
+};
+
+bool FindBarView::OnMousePressed(
+    const ui::MouseEvent& event) {
+  // The find text box only extends to the match count
+  // label.  However, users expect to be able to click
+  // anywhere inside what looks like the find text
+  // box (including on or around the match_count label)
+  // and have focus brought to the find box. Cause
+  // clicks between the textfield and the find previous
+  // button to focus the textfield.
+  const int find_text_edge =
+      find_text_->bounds().right();
+  const gfx::Rect focus_area(
+      find_text_edge, find_previous_button_->y(),
+      find_previous_button_->x() - find_text_edge,
+      find_previous_button_->height());
+  if (!GetMirroredRect(focus_area).Contains(
+      event.location()))
+    return false;
+  find_text_->RequestFocus();
+  return true;
+
+FindBarView::FindBarView(FindBarHost* host)
+    : find_bar_host_(host) {
+  auto find_text = std::make_unique<views::Textfield>();
+  find_text_ = AddChildView(std::move(find_text));
+  ...
+  auto find_previous_button =
+      std::make_unique<FindBarButton>(this);
+  views::ConfigureVectorImageButton(
+      find_previous_button.get());
+  ...
+  auto find_next_button =
+      std::make_unique<FindBarButton>(this);
+  views::ConfigureVectorImageButton(
+      find_next_button.get());
+  ...
+  auto close_button =
+      std::make_unique<FindBarButton>(this);
+  views::ConfigureVectorImageButton(close_button.get());
+}
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+```
+
+|||---|||
+
+## Don't invoke Layout() directly
+
+**Avoid direct calls to `Layout()`.**
+These are typically used for three purposes:
+
+1.  *Calling `Layout()` on `this`, when something that affects layout has
+changed.* This forces a synchronous layout, which can lead to needless
+work (e.g. if several sequential changes each trigger layout). Use
+asynchronous layout\* instead. In many cases (such as
+[the preferred size changing][] or
+[a child needing layout][],
+a `View` will automatically mark itself as needing layout; when necessary, call
+[`InvalidateLayout()`][] to mark it manually.
+
+1.  *Calling `Layout()` or `InvalidateLayout()` on some `View` to notify it
+that something affecting its layout has changed.* Instead, ensure that
+`View` is notified of the underlying change (via specific method overrides or
+plumbing from a model object), and then invalidates its own layout when needed.
+
+1.  *Calling `Layout()` on some `View` to "ensure it's up to date" before
+reading some layout-related property off it.* Instead, plumb any relevant
+events to the current object, then handle them directly (e.g. override
+[`ChildPreferredSizeChanged()`][] or use a [`ViewObserver`][]
+to monitor the target `View`; then update local state as necessary and trigger
+handler methods).
+
+\* *How does asynchronous layout work?* In the browser, the compositor
+periodically [requests a LayerTreeHost update][].
+This ultimately calls back to [`Widget::LayoutRootViewIfNecessary()`][],
+recursively laying out invalidated `View`s within the `Widget`. In unittests,
+this compositor-driven sequence never occurs, so it's necessary to
+[call LayoutRootViewIfNecessary() manually][] when a test needs to ensure a
+`View`'s layout is up-to-date.  Many tests fail to do this, but currently pass
+because something triggers Layout() directly;
+accordingly, changing existing code from synchronous to asynchronous layout may
+require adding `LayoutRootViewIfNecessary()` calls to (possibly many) tests,
+and this is not a sign that the change is wrong.
+
+[the preferred size changing]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.cc;l=1673;drc=bc9a6d40468646be476c61b6637b51729bec7b6d
+[a child needing layout]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.cc;l=777;drc=bc9a6d40468646be476c61b6637b51729bec7b6d
+[`InvalidateLayout()`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=735;drc=c06f6b339b47ce2388624aa9a89334ace38a71e4
+[`ChildPreferredSizeChanged()`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=1381;drc=c06f6b339b47ce2388624aa9a89334ace38a71e4
+[`ViewObserver`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view_observer.h;l=17;drc=eb20fd77330dc4a89eecf17459263e5895e7f177
+[requests a LayerTreeHost update]: https://source.chromium.org/chromium/chromium/src/+/master:cc/trees/layer_tree_host.cc;l=304;drc=c06f6b339b47ce2388624aa9a89334ace38a71e4
+[`Widget::LayoutRootViewIfNecessary()`]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/widget/widget.h;l=946;drc=b1dcb398c454a576092d38d0d67db3709b2b2a9b
+[call LayoutRootViewIfNecessary() manually]: https://source.chromium.org/chromium/chromium/src/+/master:ui/views/widget/widget_unittest.cc;l=3110;drc=c06f6b339b47ce2388624aa9a89334ace38a71e4
+
+|||---|||
+
+#####
+
+**Avoid**
+
+[Current code][5] makes a direct and unnecessary call to Layout()
+
+[5]: https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/media_router/cast_dialog_view.cc;l=349;drc=18ca2de542bfa53802639dc5c85762b5e7b5bef6
+
+#####
+
+**Best practice**
+
+A better approach would be to call InvalidateLayout() and update the necessary tests.
+
+|||---|||
+
+|||---|||
+
+#####
+
+``` cpp
+void CastDialogView::PopulateScrollView(
+    const std::vector<UIMediaSink>& sinks) {
+  ...
+  Layout();
+}
+
+TEST_F(CastDialogViewTest, PopulateDialog) {
+  CastDialogModel model =
+      CreateModelWithSinks({CreateAvailableSink()});
+  InitializeDialogWithModel(model);
+
+  EXPECT_TRUE(dialog_->ShouldShowCloseButton());
+  EXPECT_EQ(model.dialog_header(),
+            dialog_->GetWindowTitle());
+  EXPECT_EQ(ui::DIALOG_BUTTON_NONE,
+            dialog_->GetDialogButtons());
+}
+
+
+```
+
+#####
+
+``` cpp
+void CastDialogView::PopulateScrollView(
+    const std::vector<UIMediaSink>& sinks) {
+  ...
+  InvalidateLayout();
+}
+
+TEST_F(CastDialogViewTest, PopulateDialog) {
+  CastDialogModel model =
+      CreateModelWithSinks({CreateAvailableSink()});
+  InitializeDialogWithModel(model);
+  CastDialogView::GetCurrentDialogWidget()
+      ->LayoutRootViewIfNecessary();
+
+  EXPECT_TRUE(dialog_->ShouldShowCloseButton());
+  EXPECT_EQ(model.dialog_header(),
+            dialog_->GetWindowTitle());
+  EXPECT_EQ(ui::DIALOG_BUTTON_NONE,
+            dialog_->GetDialogButtons());
+}
+```
+
+|||---|||
+
+
+## Consider different objects for different layouts
+
+If a surface needs very different appearances in different states (e.g. a
+dialog whose content changes at each of several steps, or a container whose
+layout toggles between disparate orientations), **use different `View`s to
+contain the distinct states** instead of manually adding and removing
+children and changing layout properties at each step. It's easier to reason
+about several distinct fixed-layout `View`s than a single object whose layout
+and children vary over time, and often more performant as well.
+
+|||---|||
+
+#####
+
+**Avoid**
+
+[Current code][6] holds both horizontal and vertical time views and replaces
+the children and LayoutManager on orientation change.
+
+[6]: https://source.chromium.org/chromium/chromium/src/+/master:ash/system/time/time_view.h;l=35;drc=7d8bc7f807a433e6a127806e991fe780aa27ce77;bpv=1;bpt=0?originalUrl=https:%2F%2Fcs.chromium.org%2F
+
+#####
+
+**Best practice**
+
+A better approach would encapsulate the horizontal and vertical time views
+into separate views.
+
+|||---|||
+
+|||---|||
+
+#####
+
+``` cpp
+class ASH_EXPORT TimeView : public ActionableView,
+                            public ClockObserver {
+  ...
+ private:
+  ...
+  std::unique_ptr<views::Label> horizontal_label_;
+  std::unique_ptr<views::Label> vertical_label_hours_;
+  std::unique_ptr<views::Label> vertical_label_minutes_;
+  ...
+};
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+void TimeView::SetupLabels() {
+  horizontal_label_.reset(new views::Label());
+  SetupLabel(horizontal_label_.get());
+  vertical_label_hours_.reset(new views::Label());
+  SetupLabel(vertical_label_hours_.get());
+  vertical_label_minutes_.reset(new views::Label());
+  SetupLabel(vertical_label_minutes_.get());
+  ...
+}
+
+
+
+void TimeView::UpdateClockLayout(
+    ClockLayout clock_layout) {
+  // Do nothing if the layout hasn't changed.
+  if (((clock_layout == ClockLayout::HORIZONTAL_CLOCK) ?
+      horizontal_label_ : vertical_label_hours_)
+      ->parent() == this)
+    return;
+
+  SetBorder(views::NullBorder());
+  if (clock_layout == ClockLayout::HORIZONTAL_CLOCK) {
+    RemoveChildView(vertical_label_hours_.get());
+    RemoveChildView(vertical_label_minutes_.get());
+    SetLayoutManager(
+        std::make_unique<views::FillLayout>());
+    AddChildView(horizontal_label_.get());
+  } else {
+    RemoveChildView(horizontal_label_.get());
+    // Remove the current layout manager since it could
+    // be the FillLayout which only allows one child.
+    SetLayoutManager(nullptr);
+    // Pre-add the children since ownership is being
+    // retained by this.
+    AddChildView(vertical_label_hours_.get());
+    AddChildView(vertical_label_minutes_.get());
+    views::GridLayout* layout =
+        SetLayoutManager(
+            std::make_unique<views::GridLayout>());
+    const int kColumnId = 0;
+    views::ColumnSet* columns =
+        layout->AddColumnSet(kColumnId);
+    columns->AddPaddingColumn(
+        0, kVerticalClockLeftPadding);
+    columns->AddColumn(views::GridLayout::TRAILING,
+                       views::GridLayout::CENTER,
+                       0, views::GridLayout::USE_PREF,
+                       0, 0);
+    layout->AddPaddingRow(0, kClockLeadingPadding);
+    layout->StartRow(0, kColumnId);
+    // Add the views as existing since ownership isn't
+    // being transferred.
+    layout->AddExistingView(
+        vertical_label_hours_.get());
+    layout->StartRow(0, kColumnId);
+    layout->AddExistingView(
+        vertical_label_minutes_.get());
+    layout->AddPaddingRow(
+        0, kVerticalClockMinutesTopOffset);
+  }
+  Layout();
+}
+```
+
+#####
+
+``` cpp
+class ASH_EXPORT TimeView : public ActionableView,
+                            public ClockObserver {
+  ...
+ private:
+  class HorizontalLabelView;
+  class VerticalLabelView;
+  ...
+  HorizontalLabelView* horizontal_label_;
+  VerticalLabelView* vertical_label_;
+  ...
+};
+
+TimeView::HorizontalLabelView::HorizontalLabelView() {
+  SetLayoutManager(
+      std::make_unique<views::FillLayout>());
+  views::Label* time_label =
+      AddChildView(std::make_unique<views::Label>());
+  SetupLabels(time_label);
+  ...
+}
+
+TimeView::VerticalLabelView::VerticalLabelView() {
+  views::GridLayout* layout =
+        SetLayoutManager(
+            std::make_unique<views::GridLayout>());
+  views::Label* label_hours =
+      AddChildView(std::make_unique<views::Label>());
+  views::Label* label_minutes =
+      AddChildView(std::make_unique<views::Label>());
+  SetupLabel(label_hours);
+  SetupLabel(label_minutes);
+  const int kColumnId = 0;
+  views::ColumnSet* columns =
+      layout->AddColumnSet(kColumnId);
+  columns->AddPaddingColumn(
+      0, kVerticalClockLeftPadding);
+  columns->AddColumn(
+      views::GridLayout::TRAILING,
+      views::GridLayout::CENTER,
+      0, views::GridLayout::USE_PREF, 0, 0);
+  layout->AddPaddingRow(0, kClockLeadingPadding);
+  layout->StartRow(0, kColumnId);
+  // Add the views as existing since ownership isn't
+  // being transferred.
+  layout->AddExistingView(label_hours);
+  layout->StartRow(0, kColumnId);
+  layout->AddExistingView(label_minutes);
+  layout->AddPaddingRow(
+      0, kVerticalClockMinutesTopOffset);
+  ...
+}
+
+void TimeView::TimeView(ClockLayout clock_layout,
+                        ClockModel* model) {
+  ...
+  horizontal_label_ =
+     AddChildView(
+        std::make_unique<HorizontalLabelView>());
+  vertical_label_ =
+     AddChildView(
+        std::make_unique<VerticalLabelView());
+  ...
+}
+
+void TimeView::UpdateClockLayout(
+    ClockLayout clock_layout) {
+  ...
+  const bool is_horizontal =
+      clock_layout == ClockLayout::HORIZONTAL_CLOCK;
+  horizontal_label_->SetVisible(is_horizontal);
+  vertical_label_->SetVisible(!is_horizontal);
+  InvalidateLayout();
+}
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+```
+
+|||---|||
+

docs/ui/learn/index.md

@@ -3,6 +3,7 @@
 # Best Practices
 
 * [Colors](bestpractices/colors.md): How to work with Chromium colors.
+* [Layout](bestpractices/layout.md): How to use Views layout.
 * [Ownership](bestpractices/ownership.md): How to manage Views object lifetimes.
 
 # Architecture Overview