Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Rounded Corners]: Fix WebContents shadow in light mode #25738

Merged
merged 9 commits into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 36 additions & 14 deletions browser/ui/views/frame/brave_browser_view_layout.cc
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,9 @@ void BraveBrowserViewLayout::Layout(views::View* host) {
}

void BraveBrowserViewLayout::LayoutVerticalTabs() {
if (!vertical_tab_strip_host_.get())
if (!vertical_tab_strip_host_.get()) {
return;
}

if (!tabs::utils::ShouldShowVerticalTabs(browser_view_->browser())) {
vertical_tab_strip_host_->SetBorder(nullptr);
Expand Down Expand Up @@ -108,10 +109,11 @@ void BraveBrowserViewLayout::LayoutVerticalTabs() {
insets = AdjustInsetsConsideringFrameBorder(insets);
#endif

if (insets.IsEmpty())
if (insets.IsEmpty()) {
vertical_tab_strip_host_->SetBorder(nullptr);
else
} else {
vertical_tab_strip_host_->SetBorder(views::CreateEmptyBorder(insets));
}

const auto width =
vertical_tab_strip_host_->GetPreferredSize().width() + insets.width();
Expand Down Expand Up @@ -147,8 +149,9 @@ int BraveBrowserViewLayout::LayoutTabStripRegion(int top) {

int BraveBrowserViewLayout::LayoutBookmarkAndInfoBars(int top,
int browser_view_y) {
if (!vertical_tab_strip_host_ || !ShouldPushBookmarkBarForVerticalTabs())
if (!vertical_tab_strip_host_ || !ShouldPushBookmarkBarForVerticalTabs()) {
return BrowserViewLayout::LayoutBookmarkAndInfoBars(top, browser_view_y);
}

auto new_rect = vertical_layout_rect_;
new_rect.Inset(GetInsetsConsideringVerticalTabHost());
Expand All @@ -157,8 +160,9 @@ int BraveBrowserViewLayout::LayoutBookmarkAndInfoBars(int top,
}

int BraveBrowserViewLayout::LayoutInfoBar(int top) {
if (!vertical_tab_strip_host_)
if (!vertical_tab_strip_host_) {
return BrowserViewLayout::LayoutInfoBar(top);
}

if (ShouldPushBookmarkBarForVerticalTabs()) {
// Insets are already applied from LayoutBookmarkAndInfoBar().
Expand Down Expand Up @@ -204,8 +208,8 @@ void BraveBrowserViewLayout::LayoutSideBar(gfx::Rect& contents_bounds) {
#endif

gfx::Rect separator_bounds;

if (sidebar_container_->sidebar_on_left()) {
const bool on_left = sidebar_container_->sidebar_on_left();
if (on_left) {
contents_bounds.set_x(contents_bounds.x() + sidebar_bounds.width());

// When vertical tabs and the sidebar are adjacent, add a separator between
Expand All @@ -224,10 +228,22 @@ void BraveBrowserViewLayout::LayoutSideBar(gfx::Rect& contents_bounds) {
sidebar_bounds.set_x(contents_bounds.right());
}

// Side panel doesn't need margin as sidebar UI and contents container
// will have margins if needed.
gfx::Insets panel_margins = GetContentsMargins();
panel_margins.set_left_right(0, 0);
if (BraveBrowser::ShouldUseBraveWebViewRoundedCorners(
browser_view_->browser())) {
// In rounded mode, there is already a gap between the sidebar and the main
// contents view, so we only remove from the margin from that side (we need
// to keep it between the sidebar controls and the sidebar content).
if (on_left) {
panel_margins.set_right(0);
} else {
panel_margins.set_left(0);
}
} else {
// Side panel doesn't need margin as sidebar UI and contents container
// will have margins if needed.
panel_margins.set_left_right(0, 0);
}
sidebar_container_->side_panel()->SetProperty(views::kMarginsKey,
panel_margins);

Expand All @@ -250,14 +266,20 @@ void BraveBrowserViewLayout::UpdateContentsContainerInsets(
// Control contents's margin with sidebar & vertical tab state.
gfx::Insets contents_margins = GetContentsMargins();

// In rounded corners mode, we need to include a little margin so we have
// somewhere to draw the shadow.
int contents_margin_for_rounded_corners =
BraveContentsViewUtil::GetRoundedCornersWebViewMargin(
browser_view_->browser());

// Don't need contents container's left or right margin with vertical tab as
// vertical tab itself has sufficient padding.
if (tabs::utils::ShouldShowVerticalTabs(browser_view_->browser()) &&
!IsFullscreenForBrowser()) {
if (tabs::utils::IsVerticalTabOnRight(browser_view_->browser())) {
contents_margins.set_right(0);
contents_margins.set_right(contents_margin_for_rounded_corners);
Copy link
Contributor Author

@fallaciousreasoning fallaciousreasoning Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where we remove the margin from the webcontents for vertical tabs. This fixes the issue, but it means we double pad @simonhong (the WebContents and the VerticalTabs sidebar)

} else {
contents_margins.set_left(0);
contents_margins.set_left(contents_margin_for_rounded_corners);
}
}

Expand All @@ -282,9 +304,9 @@ void BraveBrowserViewLayout::UpdateContentsContainerInsets(
// If sidebar is shown in left-side, contents container doens't need its
// left margin.
if (sidebar_container_->sidebar_on_left()) {
contents_margins.set_left(0);
contents_margins.set_left(contents_margin_for_rounded_corners);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same, but for the sidebar

} else {
contents_margins.set_right(0);
contents_margins.set_right(contents_margin_for_rounded_corners);
}
contents_container_bounds.Inset(contents_margins);
}
Expand Down
15 changes: 11 additions & 4 deletions browser/ui/views/frame/brave_contents_view_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,18 @@

#include "brave/browser/ui/views/frame/brave_contents_view_util.h"

#include "brave/browser/ui/brave_browser.h"
#include "chrome/browser/ui/browser.h"
#include "ui/compositor/layer.h"
#include "ui/views/view.h"

namespace {

constexpr ViewShadow::ShadowParameters kShadow{
.offset_x = 0,
.offset_y = 1,
.blur_radius = 4,
.shadow_color = SkColorSetA(SK_ColorBLACK, 0.07 * 255)};

.offset_y = 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the offset_y set the shadow between the location bar and the web contents was hard to see, so I removed it

.blur_radius = BraveContentsViewUtil::kMarginThickness,
.shadow_color = SkColorSetA(SK_ColorBLACK, 0.1 * 255)};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.1 is actually what's in the Figma now, so this might have changed

} // namespace

std::unique_ptr<ViewShadow> BraveContentsViewUtil::CreateShadow(
Expand All @@ -26,3 +27,9 @@ std::unique_ptr<ViewShadow> BraveContentsViewUtil::CreateShadow(
view->layer()->SetIsFastRoundedCorner(true);
return shadow;
}

int BraveContentsViewUtil::GetRoundedCornersWebViewMargin(Browser* browser) {
return BraveBrowser::ShouldUseBraveWebViewRoundedCorners(browser)
? BraveContentsViewUtil::kMarginThickness
: 0;
}
6 changes: 6 additions & 0 deletions browser/ui/views/frame/brave_contents_view_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

#include "brave/browser/ui/views/view_shadow.h"

class Browser;

namespace views {
class View;
}
Expand All @@ -28,6 +30,10 @@ class BraveContentsViewUtil {

// Creates a drop shadow for the specified content area view.
static std::unique_ptr<ViewShadow> CreateShadow(views::View* view);

// If rounded corners are enabled, returns the additional margin required to
// get the shadow to display properly. Otherwise 0.
static int GetRoundedCornersWebViewMargin(Browser* browser);
};

#endif // BRAVE_BROWSER_UI_VIEWS_FRAME_BRAVE_CONTENTS_VIEW_UTIL_H_
41 changes: 30 additions & 11 deletions browser/ui/views/frame/vertical_tab_strip_region_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,12 @@
#include <utility>
#include <vector>

#include "base/functional/bind.h"
#include "brave/app/vector_icons/vector_icons.h"
#include "brave/browser/ui/brave_browser.h"
#include "brave/browser/ui/color/brave_color_id.h"
#include "brave/browser/ui/tabs/brave_tab_prefs.h"
#include "brave/browser/ui/views/frame/brave_contents_view_util.h"
#include "brave/browser/ui/views/tabs/brave_new_tab_button.h"
#include "brave/browser/ui/views/tabs/brave_tab_search_button.h"
#include "brave/browser/ui/views/tabs/brave_tab_strip_layout_helper.h"
Expand Down Expand Up @@ -656,7 +658,6 @@ VerticalTabStripRegionView::VerticalTabStripRegionView(
base::BindRepeating(
&VerticalTabStripRegionView::OnFloatingModePrefChanged,
base::Unretained(this)));
OnFloatingModePrefChanged();

#if BUILDFLAG(IS_MAC)
show_toolbar_on_fullscreen_pref_.Init(
Expand All @@ -667,9 +668,13 @@ VerticalTabStripRegionView::VerticalTabStripRegionView(

vertical_tab_on_right_.Init(
brave_tabs::kVerticalTabsOnRight, browser()->profile()->GetPrefs(),
base::BindRepeating(
&VerticalTabStripRegionView::OnVerticalTabPositionChanged,
base::Unretained(this)));
base::BindRepeating(&VerticalTabStripRegionView::OnBrowserPanelsMoved,
base::Unretained(this)));

sidebar_side_.Init(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kinda sucks that vertical tabs needs to know about sidebar 😢

prefs::kSidePanelHorizontalAlignment, prefs,
base::BindRepeating(&VerticalTabStripRegionView::OnBrowserPanelsMoved,
base::Unretained(this)));

widget_observation_.Observe(browser_view->GetWidget());

Expand All @@ -680,6 +685,9 @@ VerticalTabStripRegionView::VerticalTabStripRegionView(
BrowserList::GetInstance()->end())
<< "Browser shouldn't be added at this point.";
BrowserList::AddObserver(this);

// Note: This should happen after all the PrefMembers have been initialized.
OnFloatingModePrefChanged();
}

VerticalTabStripRegionView::~VerticalTabStripRegionView() {
Expand Down Expand Up @@ -945,7 +953,7 @@ void VerticalTabStripRegionView::OnShowVerticalTabsPrefChanged() {
UpdateBorder();
}

void VerticalTabStripRegionView::OnVerticalTabPositionChanged() {
void VerticalTabStripRegionView::OnBrowserPanelsMoved() {
UpdateBorder();
PreferredSizeChanged();
}
Expand Down Expand Up @@ -1091,8 +1099,10 @@ void VerticalTabStripRegionView::OnBoundsChanged(
#if DCHECK_IS_ON()
if (auto width = GetContentsBounds().width();
width && !IsBrowserFullscren()) {
CHECK_GE(width, tabs::kVerticalTabMinWidth +
tabs::kMarginForVerticalTabContainers * 2);
CHECK_GE(
width,
tabs::kVerticalTabMinWidth + tabs::kMarginForVerticalTabContainers * 2 -
BraveContentsViewUtil::GetRoundedCornersWebViewMargin(browser_));
}
#endif
}
Expand Down Expand Up @@ -1213,10 +1223,19 @@ void VerticalTabStripRegionView::UpdateBorder() {
state_ == State::kFloating;
};

gfx::Insets border_insets =
(!vertical_tab_on_right_.GetPrefName().empty() && *vertical_tab_on_right_)
? gfx::Insets::TLBR(0, 1, 0, 0)
: gfx::Insets::TLBR(0, 0, 0, 1);
// If the sidebar is on the same side as the vertical tab strip, we shouldn't
// take away the margin on the vertical tabs, because the sidebar will be
// between it and the web_contents.
bool is_on_right =
!vertical_tab_on_right_.GetPrefName().empty() && *vertical_tab_on_right_;
bool sidebar_on_same_side = sidebar_side_.GetValue() == is_on_right;
int inset =
1 -
(sidebar_on_same_side
? 0
: BraveContentsViewUtil::GetRoundedCornersWebViewMargin(browser_));
gfx::Insets border_insets = (is_on_right) ? gfx::Insets::TLBR(0, inset, 0, 0)
: gfx::Insets::TLBR(0, 0, 0, inset);

if (show_visible_border()) {
SetBorder(views::CreateSolidSidedBorder(
Expand Down
3 changes: 2 additions & 1 deletion browser/ui/views/frame/vertical_tab_strip_region_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ class VerticalTabStripRegionView : public views::View,
void UpdateStateAfterDragAndDropFinished(State original_state);

void OnShowVerticalTabsPrefChanged();
void OnVerticalTabPositionChanged();
void OnBrowserPanelsMoved();

void UpdateLayout(bool in_destruction = false);

Expand Down Expand Up @@ -228,6 +228,7 @@ class VerticalTabStripRegionView : public views::View,
fullscreen_observation_{this};

BooleanPrefMember vertical_tab_on_right_;
BooleanPrefMember sidebar_side_;

base::WeakPtrFactory<VerticalTabStripRegionView> weak_factory_{this};
};
Expand Down
6 changes: 5 additions & 1 deletion browser/ui/views/sidebar/sidebar_container_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,11 @@ void SidebarContainerView::ShowSidebar(bool show_side_panel) {
animation_start_width_ = width();
animation_end_width_ = sidebar_control_view_->GetPreferredSize().width();
if (show_side_panel) {
animation_end_width_ += side_panel_->GetPreferredSize().width();
// Note: as margins of |side_panel_| are part of |width()| we need to add
// them when calculating the ideal width of the contents.
animation_end_width_ +=
side_panel_->GetPreferredSize().width() +
side_panel_->GetProperty(views::kMarginsKey)->width();
}

// Don't need event detect widget when sidebar gets visible.
Expand Down
15 changes: 8 additions & 7 deletions browser/ui/views/sidebar/sidebar_control_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "brave/browser/ui/sidebar/sidebar_controller.h"
#include "brave/browser/ui/sidebar/sidebar_service_factory.h"
#include "brave/browser/ui/sidebar/sidebar_utils.h"
#include "brave/browser/ui/views/frame/brave_contents_view_util.h"
#include "brave/browser/ui/views/sidebar/sidebar_item_add_button.h"
#include "brave/browser/ui/views/sidebar/sidebar_items_scroll_view.h"
#include "brave/components/l10n/common/localization_util.h"
Expand All @@ -19,8 +20,10 @@
#include "brave/grit/brave_generated_resources.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/browser_command_controller.h"
#include "chrome/browser/ui/browser_window/public/browser_window_features.h"
#include "chrome/browser/ui/color/chrome_color_id.h"
#include "chrome/browser/ui/singleton_tabs.h"
#include "chrome/browser/ui/views/side_panel/side_panel_ui.h"
#include "chrome/common/pref_names.h"
#include "components/prefs/pref_service.h"
#include "ui/base/l10n/l10n_util.h"
Expand Down Expand Up @@ -86,13 +89,11 @@ void SidebarControlView::UpdateBackgroundAndBorder() {
if (const ui::ColorProvider* color_provider = GetColorProvider()) {
SetBackground(
views::CreateSolidBackground(color_provider->GetColor(kColorToolbar)));
if (!BraveBrowser::ShouldUseBraveWebViewRoundedCorners(browser_)) {
constexpr int kBorderThickness = 1;
SetBorder(views::CreateSolidSidedBorder(
gfx::Insets::TLBR(0, sidebar_on_left_ ? 0 : kBorderThickness, 0,
sidebar_on_left_ ? kBorderThickness : 0),
color_provider->GetColor(kColorToolbarContentAreaSeparator)));
}
int border_thickness =
1 - BraveContentsViewUtil::GetRoundedCornersWebViewMargin(browser_);
SetBorder(views::CreateEmptyBorder(
gfx::Insets::TLBR(0, sidebar_on_left_ ? 0 : border_thickness, 0,
sidebar_on_left_ ? border_thickness : 0)));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,7 @@

void SidePanelWebUIView::AddedToWidget() {
if (base::FeatureList::IsEnabled(features::kBraveWebViewRoundedCorners)) {
// Side panel WebUI views are positioned at the bottom of the side panel. In
// order to maintain rounded corners around the side panel, give the web
// contents native view rounded corners on the bottom.
constexpr auto radius = BraveContentsViewUtil::kBorderRadius;
holder()->SetCornerRadii(gfx::RoundedCornersF(0, 0, radius, radius));
constexpr auto kRadius = BraveContentsViewUtil::kBorderRadius;
holder()->SetCornerRadii(gfx::RoundedCornersF(kRadius));
}
}
Loading