Skip to content

Latest commit

 

History

History
1350 lines (942 loc) · 40.9 KB

layout.md

File metadata and controls

1350 lines (942 loc) · 40.9 KB

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.

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); Views 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 Views should not use these directly, but use Label and other such controls, providing the appropriate context and style.

|||---|||

Avoid

Current code uses file scoped hard-coded padding values for its layout constants.

Best practice

A better approach would be to use layout constants sourced from the ChromeLayoutProvider.

|||---|||

|||---|||

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(std::u16string());

  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));
}



SubtleNotificationView::SubtleNotificationView()
    : instruction_view_(nullptr) {
  ...
  AddChildView(std::make_unique<InstructionView>(
      std::u16string()));

  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 Views, with as many levels as necessary to make each View as simple as possible. In such hierarchies, most non-leaf Views 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 LayoutManagers 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.

|||---|||

Avoid

The following old code used Layout() to have its label text fill the dialog.

Best practice

Current code uses a FillLayout to achieve the same result.

|||---|||

|||---|||

int RelaunchRequiredDialogView::GetHeightForWidth(
    int width) const {
  const gfx::Insets insets = GetInsets();
  return body_label_->GetHeightForWidth(
      width - insets.width()) + insets.height();
}

void RelaunchRequiredDialogView::Layout(PassKey) {
  body_label_->SetBoundsRect(GetContentsBounds());
}
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 Views 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.

|||---|||

Avoid

The following is old code that calculated bubble padding through calculations involving the control insets.

Best practice

Current code uses a combination of margin and padding on the ColorPickerView to ensure proper alignment.

|||---|||

|||---|||

TabGroupEditorBubbleView::TabGroupEditorBubbleView(
    const Browser* browser,
    const tab_groups::TabGroupId& group,
    TabGroupHeader* anchor_view,
    std::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::BindRepeating(
                &TabGroupEditorBubbleView::UpdateGroup,
                base::Unretained(this))));
  ...
}
























TabGroupEditorBubbleView::TabGroupEditorBubbleView(
    const Browser* browser,
    const tab_groups::TabGroupId& group,
    views::View* anchor_view,
    std::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::BindRepeating(
                &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 TableLayout with caution

TableLayout 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 TableLayout only when creating a true grid or table, 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.

|||---|||

Avoid

The following old code uses a TableLayout to create a HoverButton with a stacked title and subtitle flanked on by views on both sides.

Best practice

Current code uses FlexLayout to achieve the desired result, resulting in clearer code.

|||---|||

|||---|||

// Used to create the following layout
// +-----------+---------------------+----------------+
// | icon_view | title               | secondary_view |
// +-----------+---------------------+----------------+
// |           | subtitle            |                |
// +-----------+---------------------+----------------+
HoverButton::HoverButton(
    ...
    std::unique_ptr<views::View> icon_view,
    const std::u16string& title,
    const std::u16string& subtitle,
    std::unique_ptr<views::View> secondary_view,
    ...) {
  ...
  views::TableLayout* table_layout =
      SetLayoutManager(
          std::make_unique<views::TableLayout>());
  ...
  table_layout->AddColumn(
      views::LayoutAlignment::kCenter,
      views::LayoutAlignment::kCenter,
      views::TableLayout::kFixedSize,
      views::TableLayout::kUsePreferred, 0, 0);
  table_layout->AddPaddingColumn(
      views::TableLayout::kFixedSize,
      icon_label_spacing);
  table_layout->AddColumn(
      views::LayoutAlignment::kStretch,
      views::LayoutAlignment::kStretch, 1.0f,
      views::TableLayout::kUsePreferred, 0, 0);
  ...
  table_layout->AddRows(
      1, views::TableLayout::kFixedSize,
      row_height);
  icon_view_ = AddChildView(
      std::move(icon_view), 1, num_labels);
  ...
  auto title_wrapper =
      std::make_unique<SingleLineStyledLabelWrapper>(
          title);
  title_ = title_wrapper->label();
  AddChildView(std::move(title_wrapper));

  if (secondary_view) {
    table_layout->AddColumn(
        views::LayoutAlignment::kCenter,
        views::LayoutAlignment::kCenter,
        views::TableLayout::kFixedSize,
        views::TableLayout::kUsePreferred, 0, 0);
    ...
    secondary_view_ = AddChildView(
        std::move(secondary_view), 1, num_labels);
    ...
  }
  if (!subtitle.empty()) {
    table_layout->AddRows(
        1, views::TableLayout::kFixedSize,
        row_height);
    auto subtitle_label =
        std::make_unique<views::Label>(
            subtitle, views::style::CONTEXT_BUTTON,
            views::style::STYLE_SECONDARY);
    ...
    AddChildView(std::make_unique<views::View>());
    subtitle_ =
        AddChildView(std::move(subtitle_label));
  }
  ...
}



// Used to create the following layout
// +-----------+---------------------+----------------+
// |           | title               |                |
// | icon_view |---------------------| secondary_view |
// |           | subtitle            |                |
// +-----------+---------------------+----------------+
HoverButton::HoverButton(
    ...
    std::unique_ptr<views::View> icon_view,
    const std::u16string& title,
    const std::u16string& subtitle,
    std::unique_ptr<views::View> secondary_view,
    ...) {
  ...
  // Set the layout manager to ignore the
  // ink_drop_container to ensure the ink drop tracks
  // the bounds of its parent.
  ink_drop_container()->SetProperty(
      views::kViewIgnoredByLayoutKey, true);

  SetLayoutManager(
      std::make_unique<views::FlexLayout>())
      ->SetCrossAxisAlignment(
          views::LayoutAlignment::kCenter);
  ...
  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, LayoutManagers 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.

|||---|||

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.

|||---|||

|||---|||

...
gfx::Size
CastDialogView::CalculatePreferredSize() const {
  const int width =
      ChromeLayoutProvider::Get()->GetDistanceMetric(
          DISTANCE_BUBBLE_PREFERRED_WIDTH);
  return gfx::Size(width, GetHeightForWidth(width));
}
...






|||---|||

Handle events directly, not via Layout()

In addition to using LayoutManagers 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 Views to intercept events. Toggle child visibility directly in response to external events rather than calculating it inside Layout().

|||---|||

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.

|||---|||

|||---|||







































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(PassKey) {
  LayoutSuperclass<views::View>(this);
  // 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());
  }
}
// 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::kMaximum, 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 DeprecatedLayoutImmediately()

Avoid calls to DeprecatedLayoutImmediately(). These are typically used for three purposes:

  1. Calling DeprecatedLayoutImmediately() 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.

  2. Calling DeprecatedLayoutImmediately() 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.

  3. Calling DeprecatedLayoutImmediately() 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 Views within the Widget. In unittests, this compositor-driven sequence never occurs, so it's necessary to call RunScheduledLayout() 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 RunScheduledLayout() calls to (possibly many) tests, and this is not a sign that the change is wrong.

|||---|||

Avoid

Current code makes an unnecessary call to DeprecatedLayoutImmediately()

Best practice

A better approach would be to call InvalidateLayout() and update the necessary tests.

|||---|||

|||---|||

void CastDialogView::PopulateScrollView(
    const std::vector<UIMediaSink>& sinks) {
  ...
  DeprecatedLayoutImmediately();
}

TEST_F(CastDialogViewTest, PopulateDialog) {
  CastDialogModel model =
      CreateModelWithSinks({CreateAvailableSink()});
  InitializeDialogWithModel(model);

  EXPECT_TRUE(dialog_->ShouldShowCloseButton());
  EXPECT_EQ(model.dialog_header(),
            dialog_->GetWindowTitle());
  EXPECT_EQ(static_cast<int>(ui::mojom::DialogButton::kNone),
            dialog_->GetDialogButtons());
}

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(static_cast<int>(ui::mojom::DialogButton::kNone),
            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 Views 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 Views than a single object whose layout and children vary over time, and often more performant as well.

|||---|||

Avoid

Current code holds both horizontal and vertical time views and replaces the children and LayoutManager on orientation change.

Best practice

A better approach would encapsulate the horizontal and vertical time views into separate views.

|||---|||

|||---|||

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);
  }
  DeprecatedLayoutImmediately();
}
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::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);
  SetLayoutManager(
      std::make_unique<views::TableLayout>())
      ->AddPaddingColumn(
          views::TableLayout::kFixedSize,
          kVerticalClockLeftPadding)
      .AddColumn(
          views::LayoutAlignment::kEnd,
          views::LayoutAlignment::kCenter,
          views::TableLayout::kFixedSize,
          views::TableLayout::kUsePreferred, 0, 0)
      .AddPaddingRow(
          views::TableLayout::kFixedSize,
          kClockLeadingPadding)
      .AddRows(2, views::TableLayout::kFixedSize)
      .AddPaddingRow(
          views::TableLayout::kFixedSize,
          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();
}








































|||---|||