From b167f41ce785881ea141c39ae1cdc39f5145edfc Mon Sep 17 00:00:00 2001 From: Patti Date: Mon, 16 Apr 2018 05:09:35 +0000 Subject: [PATCH] Omnibox/Views: Remove LocationBarView stroke for touch mode. This patch removes the 1px outline on LocationBarView and switches to using a views::FocusRing in touch mode. See screenshots - https://drive.google.com/file/d/1g4O_mh4oPb8PJIAb7pjDq0rK32KIUUvP/view?usp=sharing Bug: 801583, 829574 Change-Id: I6498ff15d3b1be6a03cab92e9aae8971b54c358e Reviewed-on: https://chromium-review.googlesource.com/1002716 Commit-Queue: Patti Reviewed-by: Michael Wasserman Reviewed-by: Peter Kasting Cr-Commit-Position: refs/heads/master@{#550935} --- chrome/browser/ui/layout_constants.cc | 8 +- chrome/browser/ui/omnibox/omnibox_theme.cc | 4 + chrome/browser/ui/omnibox/omnibox_theme.h | 1 + chrome/browser/ui/views/find_bar_host.cc | 5 +- .../views/harmony/chrome_layout_provider.cc | 6 +- .../background_with_1_px_border.cc | 21 ++--- .../background_with_1_px_border.h | 8 +- .../ui/views/location_bar/bubble_icon_view.cc | 2 +- .../location_bar/icon_label_bubble_view.cc | 2 +- .../views/location_bar/keyword_hint_view.cc | 12 +-- .../views/location_bar/location_bar_view.cc | 89 +++++++++++++++---- .../ui/views/location_bar/location_bar_view.h | 12 +++ .../omnibox/omnibox_popup_contents_view.cc | 14 +-- .../ui/views/omnibox/omnibox_result_view.cc | 5 +- ui/views/controls/focus_ring.cc | 56 +++++++----- ui/views/controls/focus_ring.h | 32 +++++-- 16 files changed, 184 insertions(+), 93 deletions(-) diff --git a/chrome/browser/ui/layout_constants.cc b/chrome/browser/ui/layout_constants.cc index f1984669e247a..ab5dab4531094 100644 --- a/chrome/browser/ui/layout_constants.cc +++ b/chrome/browser/ui/layout_constants.cc @@ -58,13 +58,7 @@ int GetLayoutConstant(LayoutConstant constant) { case LOCATION_BAR_ICON_SIZE: return touch_optimized_material ? 20 : 16; case LOCATION_BAR_ICON_INTERIOR_PADDING: - if (touch_optimized_material) { - // TODO(crbug.com/801583): This should actually be 8, but until - // LocationBarView is updated to remove its stroke, subtract the dips - // reserved for the stroke first. - return 7; - } - return 4; + return touch_optimized_material ? 8 : 4; case TABSTRIP_NEW_TAB_BUTTON_SPACING: { // In non-touch optimized UI, we make the new tab button overlap with the // last tab in the tabstrip (i.e negative spacing). However, in diff --git a/chrome/browser/ui/omnibox/omnibox_theme.cc b/chrome/browser/ui/omnibox/omnibox_theme.cc index 6e584a19408a4..3b7261f74c400 100644 --- a/chrome/browser/ui/omnibox/omnibox_theme.cc +++ b/chrome/browser/ui/omnibox/omnibox_theme.cc @@ -65,6 +65,8 @@ ui::NativeTheme::ColorId GetLegacyColorId(ui::NativeTheme* native_theme, : NativeId::kColorId_LinkEnabled; case OmniboxPart::LOCATION_BAR_TEXT_DEFAULT: return NativeId::kColorId_TextfieldDefaultColor; + case OmniboxPart::LOCATION_BAR_FOCUS_RING: + return NativeId::kColorId_FocusedBorderColor; case OmniboxPart::RESULTS_BACKGROUND: return NormalHoveredSelected( state, NativeId::kColorId_ResultsTableNormalBackground, @@ -230,6 +232,8 @@ SkColor GetOmniboxColor(OmniboxPart part, return GetSecurityChipColor(tint, state); case OmniboxPart::LOCATION_BAR_SELECTED_KEYWORD: return dark ? gfx::kGoogleGrey100 : gfx::kGoogleBlue600; + case OmniboxPart::LOCATION_BAR_FOCUS_RING: + return dark ? gfx::kGoogleBlueDark600 : gfx::kGoogleBlue600; case OmniboxPart::RESULTS_BACKGROUND: // The spec calls for transparent black (or white) overlays for hover (8%) // and select (6%), which can overlap (for 14%). Pre-blend these with the diff --git a/chrome/browser/ui/omnibox/omnibox_theme.h b/chrome/browser/ui/omnibox/omnibox_theme.h index cdb4303f39496..88e484ca2d08d 100644 --- a/chrome/browser/ui/omnibox/omnibox_theme.h +++ b/chrome/browser/ui/omnibox/omnibox_theme.h @@ -18,6 +18,7 @@ enum class OmniboxPart { LOCATION_BAR_TEXT_DEFAULT, LOCATION_BAR_TEXT_DIMMED, LOCATION_BAR_BUBBLE_OUTLINE, + LOCATION_BAR_FOCUS_RING, RESULTS_BACKGROUND, // Background of the results dropdown. RESULTS_ICON, diff --git a/chrome/browser/ui/views/find_bar_host.cc b/chrome/browser/ui/views/find_bar_host.cc index 65fb03f8f8945..2e5d3fff92919 100644 --- a/chrome/browser/ui/views/find_bar_host.cc +++ b/chrome/browser/ui/views/find_bar_host.cc @@ -265,9 +265,8 @@ gfx::Rect FindBarHost::GetDialogPosition(gfx::Rect avoid_overlapping_rect) { if (widget_bounds.IsEmpty()) return gfx::Rect(); - gfx::Insets insets = - view()->border()->GetInsets() - - gfx::Insets(0, BackgroundWith1PxBorder::kLocationBarBorderThicknessDip); + gfx::Insets insets = view()->border()->GetInsets() - + gfx::Insets(0, LocationBarView::GetBorderThicknessDip()); // Ask the view how large an area it needs to draw on. gfx::Size prefsize = view()->GetPreferredSize(); diff --git a/chrome/browser/ui/views/harmony/chrome_layout_provider.cc b/chrome/browser/ui/views/harmony/chrome_layout_provider.cc index 3dba57205a555..96cbf1f3677d7 100644 --- a/chrome/browser/ui/views/harmony/chrome_layout_provider.cc +++ b/chrome/browser/ui/views/harmony/chrome_layout_provider.cc @@ -122,8 +122,10 @@ bool ChromeLayoutProvider::IsHarmonyMode() const { int ChromeLayoutProvider::GetCornerRadiusMetric( ChromeEmphasisMetric emphasis_metric, const gfx::Rect& bounds) const { - // Outside of MD (refresh) mode, just stick to the current fixed value. - return emphasis_metric == EMPHASIS_HIGH ? 0 : 4; + // Use the current fixed value for non-EMPHASIS_HIGH. + return emphasis_metric == EMPHASIS_HIGH + ? std::min(bounds.width(), bounds.height()) / 2 + : 4; } int ChromeLayoutProvider::GetShadowElevationMetric( diff --git a/chrome/browser/ui/views/location_bar/background_with_1_px_border.cc b/chrome/browser/ui/views/location_bar/background_with_1_px_border.cc index 43639df159def..a7e1c4c8321e8 100644 --- a/chrome/browser/ui/views/location_bar/background_with_1_px_border.cc +++ b/chrome/browser/ui/views/location_bar/background_with_1_px_border.cc @@ -5,6 +5,7 @@ #include "chrome/browser/ui/views/location_bar/background_with_1_px_border.h" #include "cc/paint/paint_flags.h" +#include "chrome/browser/ui/views/location_bar/location_bar_view.h" #include "third_party/skia/include/core/SkPath.h" #include "third_party/skia/include/pathops/SkPathOps.h" #include "ui/base/material_design/material_design_controller.h" @@ -19,11 +20,6 @@ BackgroundWith1PxBorder::BackgroundWith1PxBorder(SkColor background, SetNativeControlColor(background); } -// static -bool BackgroundWith1PxBorder::IsRounded() { - return ui::MaterialDesignController::IsNewerMaterialUi(); -} - void BackgroundWith1PxBorder::PaintFocusRing(gfx::Canvas* canvas, ui::NativeTheme* theme, const gfx::Rect& local_bounds) { @@ -42,7 +38,7 @@ void BackgroundWith1PxBorder::Paint(gfx::Canvas* canvas, } float BackgroundWith1PxBorder::GetBorderRadius(int height_in_px) const { - if (IsRounded()) { + if (LocationBarView::IsRounded()) { // This method returns the inner radius of the border, so subtract 1 pixel // off the final border radius since the border thickness is always 1px. return height_in_px / 2.f - 1; @@ -60,13 +56,12 @@ void BackgroundWith1PxBorder::Paint(gfx::Canvas* canvas, gfx::RectF border_rect_f(bounds); border_rect_f.Scale(scale); - // Inset by |kLocationBarBorderThicknessDip|, then draw the border along the - // outside edge of the result. Make sure the inset amount is a whole number so - // the border will still be aligned to the pixel grid. std::floor is chosen - // here to ensure the border will be fully contained within the - // |kLocationBarBorderThicknessDip| region. - border_rect_f.Inset( - gfx::InsetsF(std::floor(kLocationBarBorderThicknessDip * scale))); + // Inset by |kBorderThicknessDip|, then draw the border along the outside edge + // of the result. Make sure the inset amount is a whole number so the border + // will still be aligned to the pixel grid. std::floor is chosen here to + // ensure the border will be fully contained within the |kBorderThicknessDip| + // region. + border_rect_f.Inset(gfx::InsetsF(std::floor(kBorderThicknessDip * scale))); SkRRect inner_rect(SkRRect::MakeRectXY(gfx::RectFToSkRect(border_rect_f), inner_border_radius, diff --git a/chrome/browser/ui/views/location_bar/background_with_1_px_border.h b/chrome/browser/ui/views/location_bar/background_with_1_px_border.h index 9cc4eef82df29..18172cd35c9a6 100644 --- a/chrome/browser/ui/views/location_bar/background_with_1_px_border.h +++ b/chrome/browser/ui/views/location_bar/background_with_1_px_border.h @@ -21,19 +21,17 @@ class View; // BackgroundWith1PxBorder renders a solid background color, with a one pixel // border with rounded corners. This accounts for the scaling of the canvas, so // that the border is one pixel regardless of display scaling. +// TODO(patricialor): Delete this & replace with CreateRoundRectWith1PxPainter. class BackgroundWith1PxBorder : public views::Background { public: - // The thickness of the location bar's border in DIP. - static constexpr int kLocationBarBorderThicknessDip = 1; + // The thickness of the border in DIP. + static constexpr int kBorderThicknessDip = 1; // The legacy (non touch/material) border radius. static constexpr int kLegacyBorderRadiusPx = 2; BackgroundWith1PxBorder(SkColor background, SkColor border); - // Whether the OmniboxBackgroundBorder is a pill shape. - static bool IsRounded(); - void set_blend_mode(SkBlendMode blend_mode) { blend_mode_ = blend_mode; } // Paints a blue focus ring that draws over the top of the existing border. diff --git a/chrome/browser/ui/views/location_bar/bubble_icon_view.cc b/chrome/browser/ui/views/location_bar/bubble_icon_view.cc index e8e4eb38d68f2..3c181dde9de46 100644 --- a/chrome/browser/ui/views/location_bar/bubble_icon_view.cc +++ b/chrome/browser/ui/views/location_bar/bubble_icon_view.cc @@ -205,7 +205,7 @@ SkColor BubbleIconView::GetInkDropBaseColor() const { } std::unique_ptr BubbleIconView::CreateInkDropMask() const { - if (!BackgroundWith1PxBorder::IsRounded()) + if (!LocationBarView::IsRounded()) return nullptr; return std::make_unique(size(), gfx::Insets(), height() / 2.f); diff --git a/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc b/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc index 319a665784a9d..a39d76c327336 100644 --- a/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc +++ b/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc @@ -307,7 +307,7 @@ SkColor IconLabelBubbleView::GetInkDropBaseColor() const { std::unique_ptr IconLabelBubbleView::CreateInkDropMask() const { - if (!BackgroundWith1PxBorder::IsRounded()) + if (!LocationBarView::IsRounded()) return nullptr; return std::make_unique( ink_drop_container_->size(), gfx::Insets(), diff --git a/chrome/browser/ui/views/location_bar/keyword_hint_view.cc b/chrome/browser/ui/views/location_bar/keyword_hint_view.cc index c6a93fa72b6d2..10de732b94ede 100644 --- a/chrome/browser/ui/views/location_bar/keyword_hint_view.cc +++ b/chrome/browser/ui/views/location_bar/keyword_hint_view.cc @@ -52,7 +52,7 @@ KeywordHintView::KeywordHintView(views::ButtonListener* listener, constexpr int kPaddingInsideBorder = 5; // Even though the border is 1 px thick visibly, it takes 1 DIP logically for // the non-rounded style. - const int horizontal_padding = BackgroundWith1PxBorder::IsRounded() + const int horizontal_padding = LocationBarView::IsRounded() ? GetCornerRadius() : kPaddingInsideBorder + 1; chip_label_->SetBorder( @@ -160,7 +160,7 @@ void KeywordHintView::SetKeyword(const base::string16& keyword, } gfx::Insets KeywordHintView::GetInsets() const { - if (!BackgroundWith1PxBorder::IsRounded()) + if (!LocationBarView::IsRounded()) return gfx::Insets(0, GetLayoutConstant(LOCATION_BAR_ICON_INTERIOR_PADDING)); @@ -203,7 +203,7 @@ void KeywordHintView::Layout() { gfx::Size leading_size(leading_label_->GetPreferredSize()); leading_label_->SetBounds(GetInsets().left(), 0, show_labels ? leading_size.width() : 0, height()); - const int chip_height = BackgroundWith1PxBorder::IsRounded() + const int chip_height = LocationBarView::IsRounded() ? GetLayoutConstant(LOCATION_BAR_ICON_SIZE) + chip_container_->GetInsets().height() : height(); @@ -225,7 +225,7 @@ gfx::Size KeywordHintView::CalculatePreferredSize() const { } void KeywordHintView::OnBoundsChanged(const gfx::Rect& previous_bounds) { - if (BackgroundWith1PxBorder::IsRounded()) { + if (LocationBarView::IsRounded()) { const int chip_corner_radius = GetCornerRadius(); chip_label_->SetBorder(views::CreateEmptyBorder( gfx::Insets(GetInsets().top(), chip_corner_radius, GetInsets().bottom(), @@ -237,7 +237,7 @@ void KeywordHintView::OnBoundsChanged(const gfx::Rect& previous_bounds) { views::Label* KeywordHintView::CreateLabel(SkColor text_color, SkColor background_color) { views::Label* label = - new views::Label(base::string16(), BackgroundWith1PxBorder::IsRounded() + new views::Label(base::string16(), LocationBarView::IsRounded() ? CONTEXT_OMNIBOX_DECORATION : CONTEXT_OMNIBOX_PRIMARY); label->SetEnabledColor(text_color); @@ -247,7 +247,7 @@ views::Label* KeywordHintView::CreateLabel(SkColor text_color, } int KeywordHintView::GetCornerRadius() const { - if (!BackgroundWith1PxBorder::IsRounded()) + if (!LocationBarView::IsRounded()) return GetLayoutConstant(LOCATION_BAR_BUBBLE_CORNER_RADIUS); return chip_container_->height() / 2; } diff --git a/chrome/browser/ui/views/location_bar/location_bar_view.cc b/chrome/browser/ui/views/location_bar/location_bar_view.cc index b7bb115370245..035c6784661e3 100644 --- a/chrome/browser/ui/views/location_bar/location_bar_view.cc +++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc @@ -43,6 +43,7 @@ #include "chrome/browser/ui/views/autofill/save_card_icon_view.h" #include "chrome/browser/ui/views/chrome_platform_style.h" #include "chrome/browser/ui/views/frame/browser_view.h" +#include "chrome/browser/ui/views/harmony/chrome_layout_provider.h" #include "chrome/browser/ui/views/harmony/chrome_typography.h" #include "chrome/browser/ui/views/location_bar/background_with_1_px_border.h" #include "chrome/browser/ui/views/location_bar/bubble_icon_view.h" @@ -125,8 +126,7 @@ using views::View; namespace { bool InTouchableMode() { - return ui::MaterialDesignController::GetMode() == - ui::MaterialDesignController::MATERIAL_TOUCH_OPTIMIZED; + return ui::MaterialDesignController::IsTouchOptimizedUiEnabled(); } OmniboxTint GetTintForProfile(Profile* profile) { @@ -146,6 +146,36 @@ OmniboxTint GetTintForProfile(Profile* profile) { return OmniboxTint::LIGHT; } +// Returns true when a views::FocusRing should be used. +bool ShouldUseFocusRingView(bool show_focus_ring) { + return (show_focus_ring && LocationBarView::IsRounded()) || + ChromePlatformStyle::ShouldOmniboxUseFocusRing(); +} + +// Installs a focus ring on the LocationBarView if it's focused and returns it. +views::View* InstallFocusRing(LocationBarView* view) { + if (!view->HasFocus()) + return nullptr; + return views::FocusRing::Install( + view, view->GetColor(OmniboxPart::LOCATION_BAR_FOCUS_RING), + view->GetBorderRadius()); +} + +views::View* UninstallFocusRing(LocationBarView* view) { + // This should be a no-op if the focus ring doesn't exist. + views::FocusRing::Uninstall(view); + return nullptr; +} + +// Helper function to create a rounded rect background (no stroke). +std::unique_ptr CreateRoundRectBackground(SkColor bg_color, + float radius) { + std::unique_ptr background = CreateBackgroundFromPainter( + views::Painter::CreateSolidRoundRectPainter(bg_color, radius)); + background->SetNativeControlColor(bg_color); + return background; +} + } // namespace // LocationBarView ----------------------------------------------------------- @@ -163,7 +193,8 @@ LocationBarView::LocationBarView(Browser* browser, browser_(browser), delegate_(delegate), is_popup_mode_(is_popup_mode), - tint_(GetTintForProfile(profile)) { + tint_(GetTintForProfile(profile)), + focus_ring_(nullptr) { edit_bookmarks_enabled_.Init( bookmarks::prefs::kEditBookmarksEnabled, profile->GetPrefs(), base::Bind(&LocationBarView::UpdateWithoutTabRestore, @@ -294,6 +325,22 @@ SkColor LocationBarView::GetOpaqueBorderColor(bool incognito) const { ThemeProperties::COLOR_TOOLBAR, incognito)); } +// static +int LocationBarView::GetBorderThicknessDip() { + return IsRounded() ? 0 : BackgroundWith1PxBorder::kBorderThicknessDip; +} + +// static +bool LocationBarView::IsRounded() { + return ui::MaterialDesignController::IsNewerMaterialUi(); +} + +float LocationBarView::GetBorderRadius() { + return IsRounded() ? ChromeLayoutProvider::Get()->GetCornerRadiusMetric( + EMPHASIS_HIGH, GetLocalBounds()) + : GetLayoutConstant(LOCATION_BAR_BUBBLE_CORNER_RADIUS); +} + SkColor LocationBarView::GetSecurityChipColor( security_state::SecurityLevel security_level) const { // Only used in ChromeOS. @@ -345,6 +392,8 @@ void LocationBarView::SetImeInlineAutocompletion(const base::string16& text) { void LocationBarView::SetShowFocusRect(bool show) { show_focus_rect_ = show; + focus_ring_ = ShouldUseFocusRingView(show) ? InstallFocusRing(this) + : UninstallFocusRing(this); SchedulePaint(); } @@ -718,13 +767,12 @@ int LocationBarView::GetAvailableTextHeight() { } void LocationBarView::OnOmniboxFocused() { - if (ChromePlatformStyle::ShouldOmniboxUseFocusRing()) - views::FocusRing::Install(this); + if (ShouldUseFocusRingView(show_focus_rect_)) + focus_ring_ = InstallFocusRing(this); } void LocationBarView::OnOmniboxBlurred() { - if (ChromePlatformStyle::ShouldOmniboxUseFocusRing()) - views::FocusRing::Uninstall(this); + focus_ring_ = UninstallFocusRing(this); } // static @@ -753,7 +801,7 @@ SkColor LocationBarView::GetBorderColor() const { gfx::Rect LocationBarView::GetLocalBoundsWithoutEndcaps() const { const float device_scale_factor = layer()->device_scale_factor(); const int border_radius = - BackgroundWith1PxBorder::IsRounded() + IsRounded() ? height() / 2 : gfx::ToCeiledInt(BackgroundWith1PxBorder::kLegacyBorderRadiusPx / device_scale_factor); @@ -763,22 +811,29 @@ gfx::Rect LocationBarView::GetLocalBoundsWithoutEndcaps() const { } int LocationBarView::GetHorizontalEdgeThickness() const { - return is_popup_mode_ - ? 0 - : BackgroundWith1PxBorder::kLocationBarBorderThicknessDip; + return is_popup_mode_ ? 0 : GetBorderThicknessDip(); } void LocationBarView::RefreshBackground() { SkColor background_color = GetColor(OmniboxPart::LOCATION_BAR_BACKGROUND); SkColor border_color = GetBorderColor(); - // When the omnibox dropdown is open, match its color. - if (InTouchableMode() && GetOmniboxPopupView()->IsOpen()) { - background_color = border_color = GetColor(OmniboxPart::RESULTS_BACKGROUND); + if (InTouchableMode()) { + // When the omnibox dropdown is open, match its color and remove the focus + // ring. + if (GetOmniboxPopupView()->IsOpen()) { + background_color = border_color = + GetColor(OmniboxPart::RESULTS_BACKGROUND); + } + if (focus_ring_) + focus_ring_->SetVisible(!GetOmniboxPopupView()->IsOpen()); } if (is_popup_mode_) { SetBackground(views::CreateSolidBackground(background_color)); + } else if (GetBorderThicknessDip() == 0) { + SetBackground( + CreateRoundRectBackground(background_color, GetBorderRadius())); } else { SetBackground(std::make_unique(background_color, border_color)); @@ -1068,6 +1123,7 @@ void LocationBarView::OnBoundsChanged(const gfx::Rect& previous_bounds) { OmniboxPopupView* popup = GetOmniboxPopupView(); if (popup->IsOpen()) popup->UpdatePopupAppearance(); + RefreshBackground(); } void LocationBarView::OnFocus() { @@ -1077,8 +1133,7 @@ void LocationBarView::OnFocus() { void LocationBarView::OnPaint(gfx::Canvas* canvas) { View::OnPaint(canvas); - if (show_focus_rect_ && omnibox_view_->HasFocus() && - !ChromePlatformStyle::ShouldOmniboxUseFocusRing()) { + if (show_focus_rect_ && omnibox_view_->HasFocus() && !focus_ring_) { static_cast(background()) ->PaintFocusRing(canvas, GetNativeTheme(), GetLocalBounds()); } @@ -1174,6 +1229,6 @@ void LocationBarView::SetFocusAndSelection(bool select_all) { // static int LocationBarView::GetTotalVerticalPadding() { - return BackgroundWith1PxBorder::kLocationBarBorderThicknessDip + + return GetBorderThicknessDip() + GetLayoutConstant(LOCATION_BAR_ELEMENT_PADDING); } diff --git a/chrome/browser/ui/views/location_bar/location_bar_view.h b/chrome/browser/ui/views/location_bar/location_bar_view.h index 2f220a74b6a4a..212ffdb8a209c 100644 --- a/chrome/browser/ui/views/location_bar/location_bar_view.h +++ b/chrome/browser/ui/views/location_bar/location_bar_view.h @@ -105,6 +105,15 @@ class LocationBarView : public LocationBar, ~LocationBarView() override; + // Returns the location bar border thickness in DIPs. + static int GetBorderThicknessDip(); + + // Whether the location bar is a pill shape. + static bool IsRounded(); + + // Returns the location bar border radius in DIPs. + float GetBorderRadius(); + // Initializes the LocationBarView. void Init(); @@ -450,6 +459,9 @@ class LocationBarView : public LocationBar, // focused. Used when the toolbar is in full keyboard accessibility mode. bool show_focus_rect_ = false; + // The focus ring view. + views::View* focus_ring_; + // Tracks this preference to determine whether bookmark editing is allowed. BooleanPrefMember edit_bookmarks_enabled_; diff --git a/chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc b/chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc index c1910b8d4f50c..90e5123958ae1 100644 --- a/chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc +++ b/chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc @@ -56,7 +56,7 @@ base::LazyInstance::DestructorAtExit g_bottom_shadow = constexpr int kPopupVerticalPadding = 4; bool IsNarrow() { - return BackgroundWith1PxBorder::IsRounded() || + return LocationBarView::IsRounded() || base::FeatureList::IsEnabled(omnibox::kUIExperimentNarrowDropdown); } @@ -176,7 +176,7 @@ class OmniboxPopupContentsView::AutocompletePopupWidget params.bounds = bounds; params.context = parent_widget->GetNativeWindow(); - if (BackgroundWith1PxBorder::IsRounded()) + if (LocationBarView::IsRounded()) RoundedOmniboxResultsFrame::OnBeforeWidgetInit(¶ms); else animator_ = std::make_unique(this, bounds); @@ -185,7 +185,7 @@ class OmniboxPopupContentsView::AutocompletePopupWidget } void SetPopupContentsView(OmniboxPopupContentsView* contents) { - if (BackgroundWith1PxBorder::IsRounded()) { + if (LocationBarView::IsRounded()) { SetContentsView(new RoundedOmniboxResultsFrame( contents, contents->location_bar_view_->tint())); } else { @@ -343,7 +343,7 @@ void OmniboxPopupContentsView::UpdatePopupAppearance() { child_at(i)->SetVisible(false); gfx::Rect new_target_bounds = UpdateMarginsAndGetTargetBounds(); - if (IsNarrow() && !BackgroundWith1PxBorder::IsRounded()) { + if (IsNarrow() && !LocationBarView::IsRounded()) { SkColor background_color = GetOmniboxColor(OmniboxPart::RESULTS_BACKGROUND, GetTint()); auto border = std::make_unique( @@ -467,7 +467,7 @@ void OmniboxPopupContentsView::OnGestureEvent(ui::GestureEvent* event) { // OmniboxPopupContentsView, private: gfx::Rect OmniboxPopupContentsView::UpdateMarginsAndGetTargetBounds() { - if (BackgroundWith1PxBorder::IsRounded()) { + if (LocationBarView::IsRounded()) { // The rounded popup is always offset the same amount from the omnibox. gfx::Rect content_rect = location_bar_view_->GetBoundsInScreen(); gfx::Insets popup_insets = @@ -519,7 +519,7 @@ int OmniboxPopupContentsView::CalculatePopupHeight() { // amount of space between the text and the popup border as there is in the // interior between each row of text. int height = popup_height; - if (BackgroundWith1PxBorder::IsRounded()) { + if (LocationBarView::IsRounded()) { height += RoundedOmniboxResultsFrame::GetNonResultSectionHeight(); } else { height += kPopupVerticalPadding * 2 + g_top_shadow.Get().height() + @@ -530,7 +530,7 @@ int OmniboxPopupContentsView::CalculatePopupHeight() { void OmniboxPopupContentsView::LayoutChildren() { gfx::Rect contents_rect = GetContentsBounds(); - if (!BackgroundWith1PxBorder::IsRounded()) { + if (!LocationBarView::IsRounded()) { contents_rect.Inset(gfx::Insets(kPopupVerticalPadding, 0)); contents_rect.Inset(start_margin_, g_top_shadow.Get().height(), end_margin_, 0); diff --git a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc index fa1b8a302de1d..5f2f88dec5c27 100644 --- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc +++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc @@ -84,7 +84,7 @@ int GetIconAlignmentOffset() { // (i.e. the views::Background). The traditional popup is designed with its // selection shape mimicking the internal shape of the omnibox border. Inset // to be consistent with the border drawn in BackgroundWith1PxBorder. - int offset = BackgroundWith1PxBorder::kLocationBarBorderThicknessDip; + int offset = LocationBarView::GetBorderThicknessDip(); // The touch-optimized popup selection always fills the results frame. So to // align icons, inset additionally by the frame alignment inset on the left. @@ -581,8 +581,7 @@ void OmniboxResultView::Layout() { // improve readability/maintainability. if (keyword_match) { - int kw_x = - start_x + BackgroundWith1PxBorder::kLocationBarBorderThicknessDip; + int kw_x = start_x + LocationBarView::GetBorderThicknessDip(); keyword_view_->icon()->SetPosition(gfx::Point( kw_x, (keyword_view_->height() - keyword_view_->icon()->height()) / 2)); kw_x += keyword_view_->icon()->width() + horizontal_padding; diff --git a/ui/views/controls/focus_ring.cc b/ui/views/controls/focus_ring.cc index a5a44bac7244b..ad16ff283adbc 100644 --- a/ui/views/controls/focus_ring.cc +++ b/ui/views/controls/focus_ring.cc @@ -5,21 +5,14 @@ #include "ui/views/controls/focus_ring.h" #include "ui/gfx/canvas.h" -#include "ui/views/controls/focusable_border.h" namespace views { namespace { -// The stroke width of the focus border in dp. +// The default stroke width of the focus border in dp. constexpr float kFocusHaloThicknessDp = 2.f; -// The focus indicator should hug the normal border, when present (as in the -// case of text buttons). Since it's drawn outside the parent view, we have to -// increase the rounding slightly. -constexpr float kFocusHaloCornerRadiusDp = - FocusableBorder::kCornerRadiusDp + kFocusHaloThicknessDp / 2.f; - FocusRing* GetFocusRing(View* parent) { for (int i = 0; i < parent->child_count(); ++i) { if (parent->child_at(i)->GetClassName() == FocusRing::kViewClassName) @@ -33,19 +26,34 @@ FocusRing* GetFocusRing(View* parent) { const char FocusRing::kViewClassName[] = "FocusRing"; // static -views::View* FocusRing::Install(View* parent, - ui::NativeTheme::ColorId override_color_id) { +FocusRing* FocusRing::Install(View* parent, + SkColor color, + float corner_radius) { FocusRing* ring = GetFocusRing(parent); if (!ring) { - ring = new FocusRing(); + ring = new FocusRing(color, corner_radius); parent->AddChildView(ring); } - ring->override_color_id_ = override_color_id; ring->Layout(); ring->SchedulePaint(); return ring; } +// static +FocusRing* FocusRing::Install(View* parent, + ui::NativeTheme::ColorId override_color_id) { + SkColor ring_color = parent->GetNativeTheme()->GetSystemColor( + override_color_id == ui::NativeTheme::kColorId_NumColors + ? ui::NativeTheme::kColorId_FocusedBorderColor + : override_color_id); + FocusRing* ring = Install(parent, ring_color); + DCHECK(ring); + ring->override_color_id_ = override_color_id; + if (!ring->view_observer_.IsObserving(parent)) + ring->view_observer_.Add(parent); + return ring; +} + // static void FocusRing::Uninstall(View* parent) { delete GetFocusRing(parent); @@ -75,21 +83,27 @@ void FocusRing::Layout() { void FocusRing::OnPaint(gfx::Canvas* canvas) { cc::PaintFlags flags; flags.setAntiAlias(true); - flags.setColor( - SkColorSetA(GetNativeTheme()->GetSystemColor( - override_color_id_ != ui::NativeTheme::kColorId_NumColors - ? override_color_id_ - : ui::NativeTheme::kColorId_FocusedBorderColor), - 0x66)); + flags.setColor(SkColorSetA(color_, 0x66)); flags.setStyle(cc::PaintFlags::kStroke_Style); flags.setStrokeWidth(kFocusHaloThicknessDp); gfx::RectF rect(GetLocalBounds()); rect.Inset(gfx::InsetsF(kFocusHaloThicknessDp / 2.f)); - canvas->DrawRoundRect(rect, kFocusHaloCornerRadiusDp, flags); + // The focus indicator should hug the normal border, when present (as in the + // case of text buttons). Since it's drawn outside the parent view, increase + // the rounding slightly by adding half the ring thickness. + canvas->DrawRoundRect(rect, corner_radius_ + kFocusHaloThicknessDp / 2.f, + flags); +} + +void FocusRing::OnViewNativeThemeChanged(View* observed_view) { + if (override_color_id_) { + color_ = observed_view->GetNativeTheme()->GetSystemColor( + override_color_id_.value()); + } } -FocusRing::FocusRing() - : override_color_id_(ui::NativeTheme::kColorId_NumColors) { +FocusRing::FocusRing(SkColor color, float corner_radius) + : color_(color), corner_radius_(corner_radius), view_observer_(this) { InitFocusRing(this); } diff --git a/ui/views/controls/focus_ring.h b/ui/views/controls/focus_ring.h index 9350679e62383..df1f1b330cad3 100644 --- a/ui/views/controls/focus_ring.h +++ b/ui/views/controls/focus_ring.h @@ -5,8 +5,11 @@ #ifndef UI_VIEWS_CONTROLS_FOCUS_RING_H_ #define UI_VIEWS_CONTROLS_FOCUS_RING_H_ +#include "base/scoped_observer.h" #include "ui/native_theme/native_theme.h" +#include "ui/views/controls/focusable_border.h" #include "ui/views/view.h" +#include "ui/views/view_observer.h" #include "ui/views/views_export.h" namespace views { @@ -14,16 +17,25 @@ namespace views { // FocusRing is a View that is designed to act as an indicator of focus for its // parent. It is a stand-alone view that paints to a layer which extends beyond // the bounds of its parent view. -class VIEWS_EXPORT FocusRing : public View { +class VIEWS_EXPORT FocusRing : public View, public ViewObserver { public: static const char kViewClassName[]; // Create a FocusRing and adds it to |parent|, or updates the one that already - // exists. |override_color_id| will be used in place of the default coloration + // exists with the given |color| and |corner_radius|. + // TODO(crbug.com/831926): Prefer using the below Install() method - this one + // should eventually be removed. + static FocusRing* Install( + View* parent, + SkColor color, + float corner_radius = FocusableBorder::kCornerRadiusDp); + + // Similar to FocusRing::Install(View, SkColor, float), but + // |override_color_id| will be used in place of the default coloration // when provided. - static View* Install(View* parent, - ui::NativeTheme::ColorId override_color_id = - ui::NativeTheme::kColorId_NumColors); + static FocusRing* Install(View* parent, + ui::NativeTheme::ColorId override_color_id = + ui::NativeTheme::kColorId_NumColors); // Removes the FocusRing from |parent|. static void Uninstall(View* parent); @@ -36,12 +48,18 @@ class VIEWS_EXPORT FocusRing : public View { void Layout() override; void OnPaint(gfx::Canvas* canvas) override; + // ViewObserver: + void OnViewNativeThemeChanged(View* observed_view) override; + protected: - FocusRing(); + FocusRing(SkColor color, float corner_radius); ~FocusRing() override; private: - ui::NativeTheme::ColorId override_color_id_; + SkColor color_; + float corner_radius_; + base::Optional override_color_id_; + ScopedObserver view_observer_; DISALLOW_COPY_AND_ASSIGN(FocusRing); };