diff options
Diffstat (limited to 'chromium/docs/ui/learn/bestpractices/layout.md')
| -rw-r--r-- | chromium/docs/ui/learn/bestpractices/layout.md | 1350 |
1 files changed, 1350 insertions, 0 deletions
diff --git a/chromium/docs/ui/learn/bestpractices/layout.md b/chromium/docs/ui/learn/bestpractices/layout.md new file mode 100644 index 00000000000..5740c73dd03 --- /dev/null +++ b/chromium/docs/ui/learn/bestpractices/layout.md @@ -0,0 +1,1350 @@ +# 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(); +} + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +``` + +|||---||| + |