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

Conversation

fallaciousreasoning
Copy link
Contributor

Resolves brave/brave-browser#41313

Before:
image

After:
image

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@fallaciousreasoning
Copy link
Contributor Author

fallaciousreasoning commented Oct 1, 2024

Hey @simonhong and @zenparsing I'm not sure if this is the best approach as it makes everything very interconnected. Do you have any ideas about a better approach?

The solution here is basically:

  1. Darken the shadow
  2. Add some left/right margins to the webcontents so the shadow has somewhere to draw on the sides


.offset_y = 0,
.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

// View::ReorderChildView() didn't work for us. So remove child views and
// add them again.
base::ranges::for_each(children(),
base::ranges::for_each(children_copy,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

without this, I was encountering crashes - I've made a separate PR for this as it's not really related, and I don't know if this PR will land 😆

#25737

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 😢

// 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)

@@ -282,9 +291,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

.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

@simonhong
Copy link
Member

After I cherry picked this PR's commits to latest master and enabled rounded corner feature, I got startup crash with this.
Curious this only happens to me.

[49481:259:1002/111318.455856:FATAL:vertical_tab_strip_region_view.cc(1105)] Check failed: width >= tabs::kVerticalTabMinWidth + tabs::kMarginForVerticalTabContainers * 2 (4 vs. 40)
0   libbase.dylib                       0x000000010f674e92 base::debug::CollectStackTrace(base::span<void const*, 18446744073709551615ul, void const**>) + 18
1   libbase.dylib                       0x000000010f6558aa base::debug::StackTrace::StackTrace(unsigned long) + 106
2   libbase.dylib                       0x000000010f52e69e logging::LogMessage::Flush() + 190
3   libbase.dylib                       0x000000010f52e56d logging::LogMessage::~LogMessage() + 29
4   libbase.dylib                       0x000000010f5087d0 logging::(anonymous namespace)::CheckLogMessage::~CheckLogMessage() + 112
5   libbase.dylib                       0x000000010f50882e logging::(anonymous namespace)::CheckLogMessage::~CheckLogMessage() + 14
6   libbase.dylib                       0x000000010f508463 logging::CheckError::~CheckError() + 35
7   libbase.dylib                       0x000000010f5084b9 logging::CheckError::~CheckError() + 9
8   libchrome_dll.dylib                 0x000000011d740721 VerticalTabStripRegionView::OnBoundsChanged(gfx::Rect const&) + 609
9   libui_views.dylib                   0x000000012c52c563 views::View::SetBoundsRect(gfx::Rect const&) + 1763
10  libui_views.dylib                   0x000000012c518529 views::LayoutManagerBase::ApplyLayout(views::ProposedLayout const&) + 425
11  libui_views.dylib                   0x000000012c518361 views::LayoutManagerBase::LayoutImpl() + 49
12  libui_views.dylib                   0x000000012c517b74 views::LayoutManagerBase::Layout(views::View*) + 132
13  libui_views.dylib                   0x000000012c52fb72 views::View::Layout(base::NonCopyablePassKey<views::View>) + 82
14  libui_views.dylib                   0x000000012c52ca95 views::View::LayoutImmediately() + 165
15  libui_views.dylib                   0x000000012c52c635 views::View::SetBoundsRect(gfx::Rect const&) + 1973
16  libui_views.dylib                   0x000000012c518529 views::LayoutManagerBase::ApplyLayout(views::ProposedLayout const&) + 425
17  libui_views.dylib                   0x000000012c518361 views::LayoutManagerBase::LayoutImpl() + 49
18  libui_views.dylib                   0x000000012c517b74 views::LayoutManagerBase::Layout(views::View*) + 132
19  libui_views.dylib                   0x000000012c52fb72 views::View::Layout(base::NonCopyablePassKey<views::View>) + 82
20  libui_views.dylib                   0x000000012c52ca95 views::View::LayoutImmediately() + 165
21  libui_views.dylib                   0x000000012c54f46b views::Widget::SetContentsView(views::View*) + 75
22  libui_views.dylib                   0x000000012c54caaa views::Widget::Init(views::Widget::InitParams) + 2090
23  libchrome_dll.dylib                 0x000000011d74604d VerticalTabStripWidgetDelegateView::Create(BrowserView*, views::View*) + 333
24  libchrome_dll.dylib                 0x000000011d6c8b36 BraveBrowserView::AddedToWidget() + 118
25  libui_views.dylib                   0x000000012c539fda views::View::PropagateAddNotifications(views::ViewHierarchyChangedDetails const&, bool) + 202
26  libui_views.dylib                   0x000000012c539136 views::View::AddChildViewAtImpl(views::View*, unsigned long) + 870
27  libui_views.dylib                   0x000000012c56f31b views::NonClientView::ViewHierarchyChanged(views::ViewHierarchyChangedDetails const&) + 75
28  libui_views.dylib                   0x000000012c539ca4 views::View::ViewHierarchyChangedImpl(views::ViewHierarchyChangedDetails const&) + 36
29  libui_views.dylib                   0x000000012c539fc4 views::View::PropagateAddNotifications(views::ViewHierarchyChangedDetails const&, bool) + 180
30  libui_views.dylib                   0x000000012c539136 views::View::AddChildViewAtImpl(views::View*, unsigned long) + 870
31  libui_views.dylib                   0x000000012c546851 views::internal::RootView::SetContentsView(views::View*) + 209
32  libui_views.dylib                   0x000000012c54cb65 views::Widget::Init(views::Widget::InitParams) + 2277
33  libchrome_dll.dylib                 0x000000012014c21b BrowserFrame::InitBrowserFrame() + 571
34  libchrome_dll.dylib                 0x000000012019578e BrowserWindow::CreateBrowserWindow(std::__Cr::unique_ptr<Browser, std::__Cr::default_delete<Browser>>, bool, bool) + 94
35  libchrome_dll.dylib                 0x000000011fcc5329 Browser::Browser(Browser::CreateParams const&) + 2169
36  libchrome_dll.dylib                 0x000000011d647722 BraveBrowser::BraveBrowser(Browser::CreateParams const&) + 18
37  libchrome_dll.dylib                 0x000000011fcc4a27 Browser::Create(Browser::CreateParams const&) + 55
38  libchrome_dll.dylib                 0x000000011e4fc333 SessionRestoreImpl::CreateRestoredBrowser(BrowserWindowInterface::Type, gfx::Rect, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>> const&, bool, ui::WindowShowState, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>> const&, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>> const&, std::__Cr::map<std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>, std::__Cr::less<std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>>, std::__Cr::allocator<std::__Cr::pair<std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>> const, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>>>> const&, int) + 467
39  libchrome_dll.dylib                 0x000000011e4fb970 SessionRestoreImpl::ProcessSessionWindows(std::__Cr::vector<std::__Cr::unique_ptr<sessions::SessionWindow, std::__Cr::default_delete<sessions::SessionWindow>>, std::__Cr::allocator<std::__Cr::unique_ptr<sessions::SessionWindow, std::__Cr::default_delete<sessions::SessionWindow>>>>*, SessionID, std::__Cr::vector<SessionRestoreDelegate::RestoredTab, std::__Cr::allocator<SessionRestoreDelegate::RestoredTab>>&, int*, int*) + 528
40  libchrome_dll.dylib                 0x000000011e4f9e9e SessionRestoreImpl::ProcessSessionWindowsAndNotify(std::__Cr::vector<std::__Cr::unique_ptr<sessions::SessionWindow, std::__Cr::default_delete<sessions::SessionWindow>>, std::__Cr::allocator<std::__Cr::unique_ptr<sessions::SessionWindow, std::__Cr::default_delete<sessions::SessionWindow>>>>*, SessionID) + 62
41  libchrome_dll.dylib                 0x000000011e4f7544 SessionRestoreImpl::Restore() + 292

@fallaciousreasoning
Copy link
Contributor Author

Huh, weird it doesn't work for you. I'll dig into it tomorrow. Out of interest, does it work if you disable the check?

@simonhong
Copy link
Member

simonhong commented Oct 2, 2024

vertical_tab_strip_region_view.cc

It works w/o that check.
This doesn't happen with your branch. Latest master with your commit has this check failure.
Enabled vertical tab and restart. and seeing this failure.

browser/ui/brave_browser.cc Outdated Show resolved Hide resolved
browser/ui/views/sidebar/sidebar_control_view.cc Outdated Show resolved Hide resolved
browser/ui/views/sidebar/sidebar_control_view.cc Outdated Show resolved Hide resolved
@fallaciousreasoning
Copy link
Contributor Author

@simonhong could you take another look?

@simonhong
Copy link
Member

@fallaciousreasoning This PR really made browser window beautiful! 👍🏼

Sorry but I can see another startup crash after enabling vertical tab.

[15092:26488:1003/132004.072:FATAL:pref_member.h(119)] Check failed: !pref_name_.empty(). 
        base::debug::CollectStackTrace [0x00007FF8E65D8AB5+21] (C:\PRJ\brave-browser\src\base\debug\stack_trace_win.cc:332)
        base::debug::StackTrace::StackTrace [0x00007FF8E65B9434+100] (C:\PRJ\brave-browser\src\base\debug\stack_trace.cc:240)
        logging::LogMessage::Flush [0x00007FF8E6462264+244] (C:\PRJ\brave-browser\src\base\logging.cc:740)
        logging::LogMessage::~LogMessage [0x00007FF8E6462109+25] (C:\PRJ\brave-browser\src\base\logging.cc:728)
        logging::`anonymous namespace'::DCheckLogMessage::~DCheckLogMessage [0x00007FF8E6436A20+144] (C:\PRJ\brave-browser\src\base\check.cc:146)
        logging::CheckError::~CheckError [0x00007FF8E643632C+44] (C:\PRJ\brave-browser\src\base\check.cc:328)
        subtle::PrefMemberBase::VerifyPref [0x00007FF8E4E6F2B9+185] (C:\PRJ\brave-browser\src\components\prefs\pref_member.cc:83)
        PrefMember<bool>::GetValue [0x00007FF8C83116EF+31] (C:\PRJ\brave-browser\src\components\prefs\pref_member.h:241)
        VerticalTabStripRegionView::UpdateBorder [0x00007FF8CA7863A9+121] (C:\PRJ\brave-browser\src\brave\browser\ui\views\frame\vertical_tab_strip_region_view.cc:1230)
        VerticalTabStripRegionView::SetState [0x00007FF8CA785F0F+495] (C:\PRJ\brave-browser\src\brave\browser\ui\views\frame\vertical_tab_strip_region_view.cc:817)   
        VerticalTabStripRegionView::VerticalTabStripRegionView [0x00007FF8CA7849BD+4621] (C:\PRJ\brave-browser\src\brave\browser\ui\views\frame\vertical_tab_strip_region_view.cc:650)
        VerticalTabStripWidgetDelegateView::VerticalTabStripWidgetDelegateView [0x00007FF8CA78C791+289] (C:\PRJ\brave-browser\src\brave\browser\ui\views\frame\vertical_tab_strip_widget_delegate_view.cc:87)
        VerticalTabStripWidgetDelegateView::Create [0x00007FF8CA78C4A6+102] (C:\PRJ\brave-browser\src\brave\browser\ui\views\frame\vertical_tab_strip_widget_delegate_view.cc:52)
        BraveBrowserView::AddedToWidget [0x00007FF8CA70C609+137] (C:\PRJ\brave-browser\src\brave\browser\ui\views\frame\brave_browser_view.cc:888)
        views::View::PropagateAddNotifications [0x00007FF8E60F77B1+241] (C:\PRJ\brave-browser\src\ui\views\view.cc:3169)
        views::View::AddChildViewAtImpl [0x00007FF8E60F66DC+1164] (C:\PRJ\brave-browser\src\ui\views\view.cc:3042)
        views::NonClientView::ViewHierarchyChanged [0x00007FF8E6136F59+89] (C:\PRJ\brave-browser\src\ui\views\window\non_client_view.cc:329)
        views::View::ViewHierarchyChangedImpl [0x00007FF8E60F7403+67] (C:\PRJ\brave-browser\src\ui\views\view.cc:3189)
        views::View::PropagateAddNotifications [0x00007FF8E60F7796+214] (C:\PRJ\brave-browser\src\ui\views\view.cc:3167)
        views::View::AddChildViewAtImpl [0x00007FF8E60F66DC+1164] (C:\PRJ\brave-browser\src\ui\views\view.cc:3042)
        views::internal::RootView::SetContentsView [0x00007FF8E6107AF6+230] (C:\PRJ\brave-browser\src\ui\views\widget\root_view.cc:303)
        views::Widget::Init [0x00007FF8E610E925+2533] (C:\PRJ\brave-browser\src\ui\views\widget\widget.cc:507)
        BrowserFrame::InitBrowserFrame [0x00007FF8CC3E6750+656] (C:\PRJ\brave-browser\src\chrome\browser\ui\views\frame\browser_frame.cc:190)
        BrowserWindow::CreateBrowserWindow [0x00007FF8CD6A6933+163] (C:\PRJ\brave-browser\src\chrome\browser\ui\views\frame\browser_window_factory.cc:83)
        Browser::Browser [0x00007FF8CC5471B2+2322] (C:\PRJ\brave-browser\src\chrome\browser\ui\browser.cc:643)
        BraveBrowser::BraveBrowser [0x00007FF8CA677D5F+15] (C:\PRJ\brave-browser\src\brave\browser\ui\brave_browser.cc:60)
        Browser::Create [0x00007FF8CC546800+64] (C:\PRJ\brave-browser\src\chrome\browser\ui\browser.cc:532)

and I'm still seeing sidebar UI flickering when toggled. It would be great if we can avoid it!

@fallaciousreasoning
Copy link
Contributor Author

Interestingly, I can reduce that error when I commit out the Init call on a PrefMember - I wonder it its because the pref isn't registered when I attempt to use it?

@simonhong
Copy link
Member

simonhong commented Oct 4, 2024

Interestingly, I can reduce that error when I commit out the Init call on a PrefMember - I wonder it its because the pref isn't registered when I attempt to use it?

I'm not sure why but it seems empty during the startup?
How about handling empty condition sidebar_side_.GetPrefName().empty() and if it's empty, just use default(maybe right?) value. I think it would work?

@github-actions github-actions bot added the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Oct 4, 2024
@fallaciousreasoning
Copy link
Contributor Author

fallaciousreasoning commented Oct 4, 2024

I found it - it was because OnFloatingModePrefChanged was triggering a code path which used the variable! Think its ready for another look.

Thanks @simonhong for your help!

@github-actions github-actions bot removed the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Oct 4, 2024
@simonhong
Copy link
Member

Really nice! I think this is the last comment from me :) sorry for many complaints 🙏🏼
When vertical tab and sidebar has on the same side, there is a vertical line between them only on left side.
Curious what is our design? maybe both have same(have separator ot not on both).
Screenshot 2024-10-04 at 12 06 57 PM
Screenshot 2024-10-04 at 12 06 35 PM

Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

++ this touch is really nice 👍🏼

My last comment is optional as it's existing behavior. It's up to you!

@fallaciousreasoning
Copy link
Contributor Author

fallaciousreasoning commented Oct 4, 2024

Would it be okay if I fixed that as a follow up? This seems to be in latest Nightly already:

On Right:
image

On Left:
image

edit: too speedy!

@fallaciousreasoning fallaciousreasoning merged commit 74b1dfa into master Oct 7, 2024
17 checks passed
@fallaciousreasoning fallaciousreasoning deleted the webui-rounded-borders branch October 7, 2024 01:01
@github-actions github-actions bot added this to the 1.72.x - Nightly milestone Oct 7, 2024
@fallaciousreasoning
Copy link
Contributor Author

@rebron do we want to uplift this to 1.71? I think we can leave it unless we're going to launch rounded corners, but might be worth having a look at the borders in 1.71 if not

@brave-builds
Copy link
Collaborator

Released in v1.72.66

@rebron
Copy link
Collaborator

rebron commented Oct 7, 2024

@fallaciousreasoning We want to uplift this into 1.71.x. We still need to discuss if we'll do rounded corners at all for release.

@kjozwiak
Copy link
Member

kjozwiak commented Oct 8, 2024

@fallaciousreasoning We want to uplift this into 1.71.x. We still need to discuss if we'll do rounded corners at all for release.

Closed #25856 (review) 👍

iefremov added a commit that referenced this pull request Oct 8, 2024
kjozwiak pushed a commit that referenced this pull request Oct 8, 2024
…) (#25880)

Revert "[Rounded Corners]: Fix WebContents shadow in light mode (#25738)"

This reverts commit 74b1dfa.
fallaciousreasoning added a commit that referenced this pull request Oct 10, 2024
…h fixes (#25895)

* Reapply "[Rounded Corners]: Fix WebContents shadow in light mode (#25738) (#25880)

This reverts commit 6b7124e.

* [Vertical Tabs]: Fix crash caused by uninitialized pref
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants