Skip to content

Commit

Permalink
Reland "[Read Anything] Set maximum side panel width during resizing."
Browse files Browse the repository at this point in the history
This is a reland of commit 7fc7a9b

What changed: Use content_container rather than content_web_view for
the maximum width calculation of side panel. Content_container also
includes dev tools, while content_web_view does not. Side panel
computation should be based on the entire contents, including dev tools.

Original change's description:
> [Read Anything] Set maximum side panel width during resizing.
>
> Current side panel resizing rules:
> 1. The minimum browser window width is 500
>    (kMainBrowserContentsMinimumWidth).
> 2. The minimum side panel width is 320.
> 3. The side panel can be widened up until it leaves 500 - 320 = 180
>    in the main web contents.
>
> Proposed side panel resizing rules:
> 1. The minimum window width remains at its current 500.
> 2. The minimum main web contents width remains at its current 180.
> 3. At 500px, the side panel is 320 and the main web contents is
>    180px, as before the proposal.
> 4. Between window width = 500 to 820, the side panel remains at
>    320px and the main contents increases in width.
> 5. At window width > 820, side panel can now be widened. It cannot
>    go wider than a width that would keep the main contents at least
>    at 500.
>
> The motivation for this change is that the current behavior, which
> allows side panel to be resized up to leaving the main web contents
> at 180, even when the browser window size is very large:
> https://screenshot.googleplex.com/9Fr9axFE4hfJWFE. For Read Anything
> we would like to remove this possibility.
>
> For more context and alternatives, see
> https://docs.google.com/document/d/1qfuhKHWrveRc5UQ37KdFGMDvt2JypvyU_4h8ejNfips/edit#
>
> Bug: 1329585, 1266555
> Change-Id: I166760e168809db6400e12c5c359ef681c7879d3
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4000980
> Reviewed-by: Peter Boström <pbos@chromium.org>
> Commit-Queue: Abigail Klein <abigailbklein@google.com>
> Cr-Commit-Position: refs/heads/main@{#1076212}

Bug: 1329585, 1266555
Change-Id: Ic15fa8d951da9bb7b74675d33d112131477a5bb5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4098251
Reviewed-by: Peter Boström <pbos@chromium.org>
Commit-Queue: Abigail Klein <abigailbklein@google.com>
Cr-Commit-Position: refs/heads/main@{#1082187}
  • Loading branch information
Abigail Klein authored and Chromium LUCI CQ committed Dec 13, 2022
1 parent 2c353f3 commit 158e380
Show file tree
Hide file tree
Showing 3 changed files with 135 additions and 27 deletions.
18 changes: 15 additions & 3 deletions chrome/browser/ui/views/side_panel/side_panel.cc
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,8 @@ bool SidePanel::IsRightAligned() {
}

gfx::Size SidePanel::GetMinimumSize() const {
const int min_side_panel_contents_width = 320;
const int min_height = 0;
return gfx::Size(min_side_panel_contents_width + kBorderInsets.width(),
return gfx::Size(min_side_panel_contents_width_ + kBorderInsets.width(),
min_height);
}

Expand Down Expand Up @@ -257,8 +256,21 @@ void SidePanel::OnResize(int resize_amount, bool done_resizing) {
starting_width_on_resize_ = -1;
}
const int minimum_width = GetMinimumSize().width();
if (proposed_width < minimum_width) {
// The side panel may be resized up to leaving the main contents at
// kMainBrowserContentsMinimumWidth.
DCHECK_EQ(browser_view_->GetMinimumSize().width(),
BrowserViewLayout::kMainBrowserContentsMinimumWidth);
const int maximum_width = width() +
browser_view_->contents_container()->width() -
BrowserViewLayout::kMainBrowserContentsMinimumWidth;
// Side panel must stay at minimum width if either:
// a) The proposed width is less than the minimum.
// b) There is not enough room for the side panel to be wider than the
// minimum.
if (proposed_width < minimum_width || maximum_width < minimum_width) {
proposed_width = minimum_width;
} else if (proposed_width > maximum_width) {
proposed_width = maximum_width;
}
if (width() != proposed_width) {
SetPanelWidth(proposed_width);
Expand Down
5 changes: 5 additions & 0 deletions chrome/browser/ui/views/side_panel/side_panel.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ class SidePanel : public views::AccessiblePaneView,
HorizontalAlignment GetHorizontalAlignment();
bool IsRightAligned();
gfx::Size GetMinimumSize() const override;
void SetMinimumSidePanelContentsWidthForTesting(int width) {
min_side_panel_contents_width_ = width;
}

// views::ResizeAreaDelegate:
void OnResize(int resize_amount, bool done_resizing) override;
Expand Down Expand Up @@ -72,6 +75,8 @@ class SidePanel : public views::AccessiblePaneView,

// Observes and listens to side panel alignment changes.
PrefChangeRegistrar pref_change_registrar_;

int min_side_panel_contents_width_ = 320;
};

#endif // CHROME_BROWSER_UI_VIEWS_SIDE_PANEL_SIDE_PANEL_H_
139 changes: 115 additions & 24 deletions chrome/browser/ui/views/side_panel/side_panel_coordinator_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,19 @@ class SidePanelCoordinatorTest : public TestWithBrowserView {
return coordinator_->header_combobox_ != nullptr;
}

void SetBrowserViewWidth(const int width) {
// If browser window is maximized then explicitly restore it, as otherwise
// the SetBounds call would be a no-op.
// TODO(crbug.com/1393153): Fix this on lacros builds and remove buildflag.
#if !BUILDFLAG(IS_CHROMEOS_LACROS)
if (browser_view()->IsMaximized())
browser_view()->Restore();
#endif

browser_view()->SetBounds(
gfx::Rect(width, browser_view()->GetBounds().height()));
}

protected:
raw_ptr<SidePanelCoordinator> coordinator_;
raw_ptr<SidePanelRegistry> global_registry_;
Expand All @@ -142,8 +155,22 @@ TEST_F(SidePanelCoordinatorTest, ChangeSidePanelWidth) {
// Set side panel to right-aligned
browser_view()->GetProfile()->GetPrefs()->SetBoolean(
prefs::kSidePanelHorizontalAlignment, true);

// The side panel can only be widened when the browser main web contents width
// is greater than |BrowserViewLayout::kMainBrowserContentsMinimumWidth|,
// which is 500. Therefore the side panel can only be widened when the total
// browser view width is greater than the minimum side panel contents width +
// 500. The browser view maximum width is constrained by the display maximum,
// which is 800 on win32 and ChromeOS.
// Therefore, we set the browser view width to be 800 and we reduce the side
// panel width to 100 so that it can be widened and shrunk.
SetBrowserViewWidth(800);
browser_view()
->unified_side_panel()
->SetMinimumSidePanelContentsWidthForTesting(100);

coordinator_->Toggle();
const int starting_width = 500;
const int starting_width = 200;
browser_view()->unified_side_panel()->SetPanelWidth(starting_width);
views::test::RunScheduledLayout(browser_view());
EXPECT_EQ(browser_view()->unified_side_panel()->width(), starting_width);
Expand All @@ -168,8 +195,21 @@ TEST_F(SidePanelCoordinatorTest, ChangeSidePanelWidth) {
}

TEST_F(SidePanelCoordinatorTest, ChangeSidePanelWidthMaxMin) {
// The side panel can only be widened when the browser main web contents width
// is greater than |BrowserViewLayout::kMainBrowserContentsMinimumWidth|,
// which is 500. Therefore the side panel can only be widened when the total
// browser view width is greater than the minimum side panel contents width +
// 500. The browser view maximum width is constrained by the display maximum,
// which is 800 on win32 and ChromeOS.
// Therefore, we set the browser view width to be 800 and we reduce the side
// panel width to 100 so that it can be widened and shrunk.
SetBrowserViewWidth(800);
browser_view()
->unified_side_panel()
->SetMinimumSidePanelContentsWidthForTesting(100);

coordinator_->Toggle();
const int starting_width = 500;
const int starting_width = 200;
browser_view()->unified_side_panel()->SetPanelWidth(starting_width);
views::test::RunScheduledLayout(browser_view());
EXPECT_EQ(browser_view()->unified_side_panel()->width(), starting_width);
Expand All @@ -184,22 +224,32 @@ TEST_F(SidePanelCoordinatorTest, ChangeSidePanelWidthMaxMin) {

browser_view()->unified_side_panel()->OnResize(-large_increment, true);
views::test::RunScheduledLayout(browser_view());
BrowserViewLayout* layout_manager =
static_cast<BrowserViewLayout*>(browser_view()->GetLayoutManager());
const int min_web_contents_width =
layout_manager->GetMinWebContentsWidthForTesting();
EXPECT_EQ(browser_view()->contents_web_view()->width(),
min_web_contents_width);
EXPECT_GT(browser_view()->unified_side_panel()->width(), starting_width);
EXPECT_EQ(browser_view()->contents_container()->width(),
BrowserViewLayout::kMainBrowserContentsMinimumWidth);
}

TEST_F(SidePanelCoordinatorTest, ChangeSidePanelWidthRTL) {
// The side panel can only be widened when the browser main web contents width
// is greater than |BrowserViewLayout::kMainBrowserContentsMinimumWidth|,
// which is 500. Therefore the side panel can only be widened when the total
// browser view width is greater than the minimum side panel contents width +
// 500. The browser view maximum width is constrained by the display maximum,
// which is 800 on win32 and ChromeOS.
// Therefore, we set the browser view width to be 800 and we reduce the side
// panel width to 100 so that it can be widened and shrunk.
SetBrowserViewWidth(800);
browser_view()
->unified_side_panel()
->SetMinimumSidePanelContentsWidthForTesting(100);

// Set side panel to right-aligned
browser_view()->GetProfile()->GetPrefs()->SetBoolean(
prefs::kSidePanelHorizontalAlignment, true);
// Set UI direction to LTR
base::i18n::SetRTLForTesting(false);
coordinator_->Toggle();
const int starting_width = 500;
const int starting_width = 200;
browser_view()->unified_side_panel()->SetPanelWidth(starting_width);
views::test::RunScheduledLayout(browser_view());
EXPECT_EQ(browser_view()->unified_side_panel()->width(), starting_width);
Expand All @@ -223,38 +273,79 @@ TEST_F(SidePanelCoordinatorTest, ChangeSidePanelWidthRTL) {
}

TEST_F(SidePanelCoordinatorTest, ChangeSidePanelWidthWindowResize) {
// The side panel can only be widened when the browser main web contents width
// is greater than |BrowserViewLayout::kMainBrowserContentsMinimumWidth|,
// which is 500. Therefore the side panel can only be widened when the total
// browser view width is greater than the minimum side panel contents width +
// 500. The browser view maximum width is constrained by the display maximum,
// which is 800 on win32 and ChromeOS.
// Therefore, we set the browser view width to be 800 and we reduce the side
// panel width to 100 so that it can be widened and shrunk.
SetBrowserViewWidth(800);
browser_view()
->unified_side_panel()
->SetMinimumSidePanelContentsWidthForTesting(100);

coordinator_->Toggle();
const int starting_width = 500;
const int starting_width = 200;
browser_view()->unified_side_panel()->SetPanelWidth(starting_width);
views::test::RunScheduledLayout(browser_view());
EXPECT_EQ(browser_view()->unified_side_panel()->width(), starting_width);

// Shrink browser window enough that side panel should also shrink in
// observance of web contents minimum width.
gfx::Rect original_bounds(browser_view()->GetBounds());
gfx::Size new_size(starting_width, starting_width);
gfx::Rect new_bounds(original_bounds);
new_bounds.set_size(new_size);
// Explicitly restore the browser window on ChromeOS, as it would otherwise
// be maximized and the SetBounds call would be a no-op.
#if BUILDFLAG(IS_CHROMEOS_ASH)
browser_view()->Restore();
#endif
browser_view()->SetBounds(new_bounds);
EXPECT_LT(browser_view()->unified_side_panel()->width(), starting_width);
BrowserViewLayout* layout_manager =
const int original_width = browser_view()->GetBounds().width();
SetBrowserViewWidth(starting_width);
EXPECT_EQ(browser_view()->unified_side_panel()->width(),
browser_view()->unified_side_panel()->GetMinimumSize().width());
BrowserViewLayout* const layout_manager =
static_cast<BrowserViewLayout*>(browser_view()->GetLayoutManager());
const int min_web_contents_width =
layout_manager->GetMinWebContentsWidthForTesting();
EXPECT_EQ(browser_view()->contents_web_view()->width(),
EXPECT_EQ(browser_view()->contents_container()->width(),
min_web_contents_width);

// Return browser window to original size, side panel should also return to
// size prior to window resize.
browser_view()->SetBounds(original_bounds);
SetBrowserViewWidth(original_width);
EXPECT_EQ(browser_view()->unified_side_panel()->width(), starting_width);
}

TEST_F(SidePanelCoordinatorTest, ChangeSidePanelWidthSmallWindow) {
SetBrowserViewWidth(500);
browser_view()
->unified_side_panel()
->SetMinimumSidePanelContentsWidthForTesting(100);
coordinator_->Toggle();
const int starting_width = 200;
const int min_side_panel_width =
browser_view()->unified_side_panel()->GetMinimumSize().width();
browser_view()->unified_side_panel()->SetPanelWidth(starting_width);
views::test::RunScheduledLayout(browser_view());
EXPECT_EQ(browser_view()->unified_side_panel()->width(),
min_side_panel_width);

// Attempt to resize the side panel, side panel should not be able to resized.
const int increment = 50;
browser_view()->unified_side_panel()->OnResize(increment, true);
views::test::RunScheduledLayout(browser_view());
EXPECT_EQ(browser_view()->unified_side_panel()->width(),
min_side_panel_width);
BrowserViewLayout* const layout_manager =
static_cast<BrowserViewLayout*>(browser_view()->GetLayoutManager());
const int min_web_contents_width =
layout_manager->GetMinWebContentsWidthForTesting();
EXPECT_EQ(browser_view()->contents_container()->width(),
min_web_contents_width);

browser_view()->unified_side_panel()->OnResize(-increment, true);
views::test::RunScheduledLayout(browser_view());
EXPECT_EQ(browser_view()->unified_side_panel()->width(),
min_side_panel_width);
EXPECT_EQ(browser_view()->contents_container()->width(),
min_web_contents_width);
}

TEST_F(SidePanelCoordinatorTest, ChangeSidePanelAlignment) {
browser_view()->GetProfile()->GetPrefs()->SetBoolean(
prefs::kSidePanelHorizontalAlignment, true);
Expand Down

0 comments on commit 158e380

Please sign in to comment.