brave
Use chromium's side panel inside our side bar container, and insert Reading List
Chromium's side panel becomes controlled by Brave's side bar buttons, which uses chromium's side panel coordinator.
The goal of this PR is to get the best of both worlds - use Chromium's SidePanel related views, models, coordinators, WebContents management, and WebUI resources, whilst still using the awesome Brave SideBar from @simonhong.
There are many things we and chromium would like to use side panels for, and this could set us up to scale well for that.
Another change is that using the toolbar button to open side panel will result in the sidebar (and panel) staying open, even if the sidebar prefs are set to open on mouseover or never. Once the panel closes, then the sidebar will to if the prefs have either of these values.
Note: One consideration is to instead do the inverse of what I have done in this PR: Perhaps we move our SideBar to be a child of browser_view_->right_aligned_side_panel() (like the ComboBox is for Chromium) and have the toolbar button turn on SideBar + SidePanel, with no separation between the two. Some complication will need to be catered to where we want the SideBar to show even when there is no active SidePanel.
TODO
- [x] Use Chromium's SidePanelCoordinator to manage WebContents and panel open / close
- [x] Reading list works
- [x] Bookmarks works, and remove Brave's clone of the panel
- [x] Panel opens and closes when clicking SideBar item
- [x] Design team feedback on padding and border colors
- [ ] Panel gets focus when opening
- [x] Toolbar button controls SideBar, not just panel inside it
- [x] Replace toolbar button icon so that sidebar is on left
- [ ] Use Brave design styles for Reading list panel, not chromium (for follow-up)
- [ ] Have side panel show on right for RTL UI (for follow-up)
Resolves https://github.com/brave/brave-browser/issues/17959
Future considerations:
- [ ] Side search
- [ ] Interaction with vertical tabs, and perhaps Side Bar + Side Panel moving to the right
Submitter Checklist:
- [x] I confirm that no security/privacy review is needed, or that I have requested one
- [x] There is a ticket for my issue
- [x] Used Github auto-closing keywords in the PR description above
- [x] 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/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issue - [ ] Checked the PR locally:
npm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,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:
- [ ] The associated issue milestone is set to the smallest version that the changes has landed on
- [ ] All relevant documentation has been updated, for instance:
- [ ] https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
- [ ] https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
- [ ] https://github.com/brave/brave-browser/wiki/Fingerprinting-Protections
- [ ] https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
- [ ] https://github.com/brave/brave-browser/wiki/Custom-Headers
- [ ] https://github.com/brave/brave-browser/wiki/Web-Compatibility-Exceptions-in-Brave
- [ ] https://github.com/brave/brave-browser/wiki/QA-Guide
- [ ] https://github.com/brave/brave-browser/wiki/P3A
Test Plan:
On https://github.com/brave/brave-browser/issues/17959
I have now pushed all functionality working well with the new toolbar icon. The Sidebar correctly shows active states when the panel is made active from external means (i.e. the side panel toolbar icon).
Another change is that using the toolbar button to open side panel will result in the sidebar (and panel) staying open, even if the sidebar prefs are set to open on mouseover or never. Once the panel closes, then the sidebar will to if the prefs have either of these values.
~The only item left for me at the moment is to update the vector icon from figma.~
I realized that SidebarService doesn't show newly introduced default item.
So existing user should add reading list item via add item bubble.
It should be handled as a separated issue.
When any item is not activated after launching, clicking toolbar button opens always reading list (even it's removed item). When readling list item is removed, I think another (bookmark) item should be activated. If there are no visible items for panel, what should we do when clicks toolbar button?
also there is no border between web contens and panel when panel is opened via toolbar button after launching.
@simonhong I've addressed feedback again and added / amended some tests as appropriate. Please re-review when you can.
I realized that SidebarService doesn't show newly introduced default item. So existing user should add reading list item via add item bubble. It should be handled as a separated issue.
I've handled that in this PR now. I migrated the prefs such that the existing pref only stores user items (not built-in). And I made a new pref which simply contains a list of IDs of built-in items which the user has hidden.
When any item is not activated after launching, clicking toolbar button opens always reading list (even it's removed item). When readling list item is removed, I think another (bookmark) item should be activated. If there are no visible items for panel, what should we do when clicks toolbar button?
This should now work as you describe. And if the user removes all panel items, then the toolbar icon will hide.
also there is no border between web contens and panel when panel is opened via toolbar button after launching.
This is fixed. The issue was the delay between sidebar item getting activated and sidepanel view being shown (due to waiting for load event).
@sangwoo108 @mkarolin @simonhong thanks for the reviews - feedback is addressed again. Changes can be seen here: https://github.com/brave/brave-core/compare/45e5419e2c160c5a7cef1c0e5735a4329f2d670e..4ffd577477
Updated to fix network audit test, review changes can now be seen at https://github.com/brave/brave-core/compare/45e5419e2c160c5a7cef1c0e5735a4329f2d670e..4ffd577477
@sangwoo108 I addressed your welcome nits via https://github.com/brave/brave-core/compare/4ffd577477..650d1c300b - thanks!