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.

Sign up for GitHub

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

Jump to bottom

Smooth the transition when closing both reading list panel + sidebar with show sidebar set to never #25382

Closed
stephendonner opened this issue Sep 13, 2022 · 2 comments · Fixed by brave/brave-core#19080
Closed

Smooth the transition when closing both reading list panel + sidebar with show sidebar set to never #25382

stephendonner opened this issue Sep 13, 2022 · 2 comments · Fixed by brave/brave-core#19080
Assignees
@simonhong
Labels
bug feature/sidebar Relating to Brave's Sidebar feature feature/user-interface All UI related OS/Desktop priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass-macOS QA Pass-Win64 QA/Test-Plan-Specified QA/Yes release-notes/include usability
Milestone
1.58.x - Release

Comments

@stephendonner
Copy link

Description

Slightly janky two-stage reading list + sidebar panel closings, when show sidebar is set to never

Credit to @kjozwiak for the find + video (he's on Windows, I'm on macOS, and have reproduced it too).

Steps to Reproduce

  1. install 1.45.44
  2. launch Brave
  3. click on "hamburger menu" -> Show sidebar -> Never
  4. click on the Show side panel button in the toolbar
  5. notice it opens both sidebar + reading list, together
  6. now, click again on the Hide side panel button in the toolbar
  7. notice it closes the reading list panel first, followed by the sidebar panel

Actual result:

Settings.-.Brave.2022-09-13.16-50-28.mp4

Expected result:

Both panels close in one fell swoop (highly preferred), or there should be less jank between transitions.

c.f. #21992

Reproduces how often:

100%

Brave version (brave://version info)

Brave 1.45.44 Chromium: 105.0.5195.102 (Official Build) nightly (x86_64)
Revision 4c16f5ffcc2da70ee2600d5db77bed423ac03a5a-refs/branch-heads/5195_55@{#4}
OS macOS Version 11.6.8 (Build 20G730)

Version/Channel Information:

  • Can you reproduce this issue with the current release?
  • Can you reproduce this issue with the beta channel?
  • Can you reproduce this issue with the nightly channel?

Other Additional Information:

  • Does the issue resolve itself when disabling Brave Shields?
  • Does the issue resolve itself when disabling Brave Rewards?
  • Is the issue reproducible on the latest version of Chrome?

Miscellaneous Information:

cc @petemill @simonhong @rebron
@simonhong simonhong self-assigned this Sep 20, 2022
@rebron rebron added the priority/P3 The next thing for us to work on. It'll ride the trains. label Sep 23, 2022
@rebron rebron added this to Untriaged backlog in Front End Sep 26, 2022
@simonhong simonhong moved this from Untriaged backlog to In progress in Front End Sep 27, 2022
@rebron rebron moved this from In progress to On Deck in Front End Nov 1, 2022
@DesertBear DesertBear mentioned this issue Feb 25, 2023
Closed
@simonhong simonhong moved this from On Deck to In progress in Front End Jun 23, 2023
simonhong added a commit to brave/brave-core that referenced this issue Jun 28, 2023
@simonhong
Sidebar Slide animation
d5e90a0 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
simonhong added a commit to brave/brave-core that referenced this issue Jun 28, 2023
@simonhong
Sidebar Slide animation
5449ff4 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
simonhong added a commit to brave/brave-core that referenced this issue Jun 28, 2023
@simonhong
Sidebar Slide animation
6cdf7b9 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
@simonhong simonhong mentioned this issue Jun 28, 2023
Merged
25 tasks
simonhong added a commit to brave/brave-core that referenced this issue Jun 29, 2023
@simonhong
Sidebar Slide animation
855da13 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
simonhong added a commit to brave/brave-core that referenced this issue Jun 29, 2023
@simonhong
Sidebar Slide animation
e8f9d1a 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
simonhong added a commit to brave/brave-core that referenced this issue Jun 29, 2023
@simonhong
Sidebar Slide animation
f27961c 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
simonhong added a commit to brave/brave-core that referenced this issue Jun 29, 2023
@simonhong
Sidebar Slide animation
e69133d 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
simonhong added a commit to brave/brave-core that referenced this issue Jun 30, 2023
@simonhong
Sidebar Slide animation
201d075 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
simonhong added a commit to brave/brave-core that referenced this issue Jun 30, 2023
@simonhong
Sidebar Slide animation
0f8a07e 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
simonhong added a commit to brave/brave-core that referenced this issue Jun 30, 2023
@simonhong
Sidebar Slide animation
e57e73b 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
simonhong added a commit to brave/brave-core that referenced this issue Jun 30, 2023
@simonhong
Sidebar Slide animation
eaaf2b6 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
simonhong added a commit to brave/brave-core that referenced this issue Jul 3, 2023
@simonhong
Sidebar Slide animation
1ea6a48 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
simonhong added a commit to brave/brave-core that referenced this issue Jul 3, 2023
@simonhong
Sidebar Slide animation
c85d6c5 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
simonhong added a commit to brave/brave-core that referenced this issue Jul 3, 2023
@simonhong
Sidebar Slide animation
16e0c16 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
simonhong added a commit to brave/brave-core that referenced this issue Jul 3, 2023
@simonhong
Sidebar Slide animation
3dc1bb4 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
simonhong added a commit to brave/brave-core that referenced this issue Jul 3, 2023
@simonhong
Sidebar Slide animation
cf17d47 
fix brave/brave-browser#21992
fix brave/brave-browser#25382
simonhong added a commit to brave/brave-core that referenced this issue Jul 3, 2023
@simonhong
Sidebar Slide animation
035e301 
fix brave/brave-browser#21992
fix brave/brave-browser#25382

Added slide animation for opening/closing sidebar/side panel.
To use slide animation, this PR has some more refactoring/cleanup.

1. Ask to show/hide sidebar/panel when only needed.
2. Use SidePanelUI(SidePanelCoordinator) for starting side panel activating.
   So far, we use both(SidePanelUI/SidebarController) for activating side
   panel item and this causes some methods are called multiple times.
3. Renamed ambiguous`Sidebar::UpdateSidebar()` to `Sidebar::UpdateSidebarItemsState()`
   as it's called only when item's status should be updated.
4. Removed `SidebarController::UpdateSidebarVisibility()`. Instead of asking sidebar's
   visibility update from controller, `SidebarContainerView` updates its child view's
   visibility via active index change notification.
simonhong added a commit to brave/brave-core that referenced this issue Jul 3, 2023
@simonhong
Sidebar Slide animation
f0e60fc 
fix brave/brave-browser#21992
fix brave/brave-browser#25382

Added slide animation for opening/closing sidebar/side panel.
To use slide animation, this PR has some more refactoring/cleanup.

1. Ask to show/hide sidebar/panel when only needed.
2. Use SidePanelUI(SidePanelCoordinator) for starting side panel activating.
   So far, we use both(SidePanelUI/SidebarController) for activating side
   panel item and this causes some methods are called multiple times.
3. Renamed ambiguous`Sidebar::UpdateSidebar()` to `Sidebar::UpdateSidebarItemsState()`
   as it's called only when item's status should be updated.
4. Removed `SidebarController::UpdateSidebarVisibility()`. Instead of asking sidebar's
   visibility update from controller, `SidebarContainerView` updates its child view's
   visibility via active index change notification.
simonhong added a commit to brave/brave-core that referenced this issue Jul 3, 2023
@simonhong
Sidebar Slide animation
182ee7e 
fix brave/brave-browser#21992
fix brave/brave-browser#25382

Added slide animation for opening/closing sidebar/side panel.
To use slide animation, this PR has some more refactoring/cleanup.

1. Ask to show/hide sidebar/panel when only needed.
2. Use SidePanelUI(SidePanelCoordinator) for starting side panel activating.
   So far, we use both(SidePanelUI/SidebarController) for activating side
   panel item and this causes some methods are called multiple times.
3. Renamed ambiguous`Sidebar::UpdateSidebar()` to `Sidebar::UpdateSidebarItemsState()`
   as it's called only when item's status should be updated.
4. Removed `SidebarController::UpdateSidebarVisibility()`. Instead of asking sidebar's
   visibility update from controller, `SidebarContainerView` updates its child view's
   visibility via active index change notification.
simonhong added a commit to brave/brave-core that referenced this issue Jul 4, 2023
@simonhong
Sidebar Slide animation
18d300e 
fix brave/brave-browser#21992
fix brave/brave-browser#25382

Added slide animation for opening/closing sidebar/side panel.
To use slide animation, this PR has some more refactoring/cleanup.

1. Ask to show/hide sidebar/panel when only needed.
2. Use SidePanelUI(SidePanelCoordinator) for starting side panel activating.
   So far, we use both(SidePanelUI/SidebarController) for activating side
   panel item and this causes some methods are called multiple times.
3. Renamed ambiguous`Sidebar::UpdateSidebar()` to `Sidebar::UpdateSidebarItemsState()`
   as it's called only when item's status should be updated.
4. Removed `SidebarController::UpdateSidebarVisibility()`. Instead of asking sidebar's
   visibility update from controller, `SidebarContainerView` updates its child view's
   visibility via active index change notification.
simonhong added a commit to brave/brave-core that referenced this issue Jul 4, 2023
@simonhong
Sidebar Slide animation
d8f5940 
fix brave/brave-browser#21992
fix brave/brave-browser#25382

Added slide animation for opening/closing sidebar/side panel.
To use slide animation, this PR has some more refactoring/cleanup.

1. Ask to show/hide sidebar/panel when only needed.
2. Use SidePanelUI(SidePanelCoordinator) for starting side panel activating.
   So far, we use both(SidePanelUI/SidebarController) for activating side
   panel item and this causes some methods are called multiple times.
3. Renamed ambiguous`Sidebar::UpdateSidebar()` to `Sidebar::UpdateSidebarItemsState()`
   as it's called only when item's status should be updated.
4. Removed `SidebarController::UpdateSidebarVisibility()`. Instead of asking sidebar's
   visibility update from controller, `SidebarContainerView` updates its child view's
   visibility via active index change notification.
simonhong added a commit to brave/brave-core that referenced this issue Jul 7, 2023
@simonhong
Sidebar Slide animation
d748cf3 
fix brave/brave-browser#21992
fix brave/brave-browser#25382

Added slide animation for opening/closing sidebar/side panel.
To use slide animation, this PR has some more refactoring/cleanup.

1. Ask to show/hide sidebar/panel when only needed.
2. Use SidePanelUI(SidePanelCoordinator) for starting side panel activating.
   So far, we use both(SidePanelUI/SidebarController) for activating side
   panel item and this causes some methods are called multiple times.
3. Renamed ambiguous`Sidebar::UpdateSidebar()` to `Sidebar::UpdateSidebarItemsState()`
   as it's called only when item's status should be updated.
4. Removed `SidebarController::UpdateSidebarVisibility()`. Instead of asking sidebar's
   visibility update from controller, `SidebarContainerView` updates its child view's
   visibility via active index change notification.
simonhong added a commit to brave/brave-core that referenced this issue Jul 12, 2023
@simonhong
Sidebar Slide animation
fd1a9a5 
fix brave/brave-browser#21992
fix brave/brave-browser#25382

Added slide animation for opening/closing sidebar/side panel.
To use slide animation, this PR has some more refactoring/cleanup.

1. Ask to show/hide sidebar/panel when only needed.
2. Use SidePanelUI(SidePanelCoordinator) for starting side panel activating.
   So far, we use both(SidePanelUI/SidebarController) for activating side
   panel item and this causes some methods are called multiple times.
3. Renamed ambiguous`Sidebar::UpdateSidebar()` to `Sidebar::UpdateSidebarItemsState()`
   as it's called only when item's status should be updated.
4. Removed `SidebarController::UpdateSidebarVisibility()`. Instead of asking sidebar's
   visibility update from controller, `SidebarContainerView` updates its child view's
   visibility via active index change notification.
@brave-builds brave-builds added this to the 1.58.x - Nightly milestone Jul 12, 2023
@rebron rebron moved this from In progress to Completed in Front End Jul 18, 2023
@stephendonner
Copy link
Author

Verified PASSED using

Brave | 1.58.78 Chromium: 116.0.5845.51 (Official Build) nightly (x86_64)
-- | --
Revision | ca260d83d1ffda3cb35bff4bdc3251f30f9c8ccd
OS | macOS Version 11.7.9 (Build 20G1426)

Followed the original steps to reproduce from #25382 (comment)

Confirmed the sidebar + contents "soft-close" when toggling

sidebar-close-smooth

@stephendonner
Copy link
Author

Verified PASSED using

Brave | 1.58.91 Chromium: 116.0.5845.96 (Official Build) beta (64-bit)
-- | --
Revision | 620fa9c0307b294dd6e7c6b0976bc1f93fecf369
OS | Windows 10 Version 22H2 (Build 19045.3324)

Followed my original steps to reproduce.

Confirmed "soft-close"/smooth closing of sidebar + its open items

smooth-closing

@rebron rebron changed the title Slightly janky two-stage reading list + sidebar panel closings, when show sidebar is set to never Smooth transition and remove pause when closing both reading list panel + sidebar with show sidebar set to never Aug 31, 2023
@rebron rebron changed the title Smooth transition and remove pause when closing both reading list panel + sidebar with show sidebar set to never Smooth transition when closing both reading list panel + sidebar with show sidebar set to never Aug 31, 2023
@rebron rebron changed the title Smooth transition when closing both reading list panel + sidebar with show sidebar set to never Smooth the transition when closing both reading list panel + sidebar with show sidebar set to never Aug 31, 2023
@LaurenWags LaurenWags mentioned this issue Sep 13, 2023
Closed
@rebron rebron removed this from Completed in Front End Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@simonhong simonhong

Labels
bug feature/sidebar Relating to Brave's Sidebar feature feature/user-interface All UI related OS/Desktop priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass-macOS QA Pass-Win64 QA/Test-Plan-Specified QA/Yes release-notes/include usability
Projects
None yet
Milestone
1.58.x - Release
Development

Successfully merging a pull request may close this issue.

Sidebar Slide animation brave/brave-core
4 participants
@stephendonner @rebron @simonhong @brave-builds