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

Use side-panel's bookmark part as sidebar's bookmark panel #20500

Closed
simonhong opened this issue Jan 14, 2022 · 4 comments · Fixed by brave/brave-core#11897
Closed

Use side-panel's bookmark part as sidebar's bookmark panel #20500

simonhong opened this issue Jan 14, 2022 · 4 comments · Fixed by brave/brave-core#11897
Assignees

Comments

@simonhong
Copy link
Member

simonhong commented Jan 14, 2022

Load below side panel's bookmark part to sidebar bookmarks
Screen Shot 2022-01-14 at 10 42 38 AM

@simonhong simonhong self-assigned this Jan 14, 2022
@simonhong
Copy link
Member Author

side-panel page is loaded in sidebar like below.
However, that page doesn't work properly.
Ex) Add current tab's state is not updated. Context menu is not rendered.
image

@simonhong
Copy link
Member Author

simonhong commented Jan 17, 2022

Found the reason why some features of read-later page(brave://read-later.top-chrome) doesn't work in the sidebar panel
after loading its url in the sidebar panel's webcontents.
chromium uses customized webview for sidepanel(ReadLaterSidePanelWebView) that monitors active tab changing,
providing custom context menu and set BubbleContentsWrapper::Host toReadLaterUI.
However, sidebar uses one WebView instance with different WebContents for each builtin panel.
To make sidepanel's read-later page works at sidebar, sidebar bookmarks panel should provide similar logic what ReadLaterSidePanelWebView does.

@simonhong simonhong changed the title Use side-panel's page as sidebar's bookmark page Use side-panel's bookmark part as sidebar's bookmark panel Jan 18, 2022
@simonhong
Copy link
Member Author

One more thing - we may want to display side panel's bookmark part at sidebar regardless of upstream's side-panel feature flag state.

@rebron rebron added priority/P3 The next thing for us to work on. It'll ride the trains. priority/P4 Planned work. We expect to get to it "soon". and removed priority/P3 The next thing for us to work on. It'll ride the trains. labels Jan 18, 2022
@rebron rebron added priority/P3 The next thing for us to work on. It'll ride the trains. and removed priority/P4 Planned work. We expect to get to it "soon". labels Jan 25, 2022
simonhong added a commit to brave/brave-core that referenced this issue Jan 31, 2022
fix brave/brave-browser#20500

Applied bookmark part of chromium's side panel to sidebar bookmark panel.
sidebar.mojom is used for interacting between sidebar's bookmarks panel
with SidebarBookmarksPageHandler.
Most of webui impls in browser/resources/sidebar/bookmarks are copied from upstream
and modified to show bookmark part only.
(side panel's page shows read later and bookmark part both.)
Bookmarks page uses custom context menu by asking to native via
Sidebar:ShowCustomContextMenu interface.
@simonhong simonhong added this to the 1.37.x - Nightly milestone Jan 31, 2022
muliswilliam pushed a commit to brave/brave-core that referenced this issue Feb 7, 2022
fix brave/brave-browser#20500

Applied bookmark part of chromium's side panel to sidebar bookmark panel.
sidebar.mojom is used for interacting between sidebar's bookmarks panel
with SidebarBookmarksPageHandler.
Most of webui impls in browser/resources/sidebar/bookmarks are copied from upstream
and modified to show bookmark part only.
(side panel's page shows read later and bookmark part both.)
Bookmarks page uses custom context menu by asking to native via
Sidebar:ShowCustomContextMenu interface.
muliswilliam pushed a commit to brave/brave-core that referenced this issue Feb 8, 2022
fix brave/brave-browser#20500

Applied bookmark part of chromium's side panel to sidebar bookmark panel.
sidebar.mojom is used for interacting between sidebar's bookmarks panel
with SidebarBookmarksPageHandler.
Most of webui impls in browser/resources/sidebar/bookmarks are copied from upstream
and modified to show bookmark part only.
(side panel's page shows read later and bookmark part both.)
Bookmarks page uses custom context menu by asking to native via
Sidebar:ShowCustomContextMenu interface.
@stephendonner
Copy link

stephendonner commented Feb 9, 2022

Verified PASSED using

Brave 1.37.36 Chromium: 98.0.4758.87 (Official Build) nightly (64-bit)
Revision e4cd00f135fb4d8edc64c8aa6ecbe7cc79ebb3b2-refs/branch-heads/4758@{#1002}
OS Windows 10 Version 21H2 (Build 19044.1466)

Steps:

  1. new profile
  2. open brave://flags/
  3. set Enable Sidebar to Enabled
  4. restart Brave
  5. click on the Bookmarks icon in Sidebar

Tested the following:

Context-menu

  • Open in New Tab
  • Open in New Window
  • Open in Private Window

  • Edit...

  • Cut
  • Copy
  • Paste

  • Delete

  • Add Page...
  • Add Folder...

  • Bookmark Manager

drag and drop from brave://bookmarks <-> Bookmarks in Sidebar
drag and drop from desktop -> Bookmarks in Sidebar

example example example example example
20500-1 20500-2 20500-3 20500-4 20500-5
example example example example example
20500-6 20500-7 20500-8 20500-9 20500-10

Also tested multi-selection context menu items (Open all (9))

Filed:
#20995
#20996
#21000
#21004

And still testing 😉


Verification passed on

Brave 1.37.86 Chromium: 99.0.4844.51 (Official Build) beta (64-bit)
Revision d537ec02474b5afe23684e7963d538896c63ac77-refs/branch-heads/4844@{#875}
OS Ubuntu 18.04 LTS

Verified the following:

  • Open in New Tab
  • Open in New Window
  • Open in Private Window
  • Edit...
  • Cut
  • Copy
  • Paste
  • Delete
  • Add Page...
  • Add Folder...

Example:
image

Drag and drop within bookmark panel
image
image

Drag and drop between bookmark panel and bookmark manager


Verified PASSED using the above testcases with

Brave 1.37.91 Chromium: 99.0.4844.51 (Official Build) beta (x86_64)
Revision d537ec02474b5afe23684e7963d538896c63ac77-refs/branch-heads/4844@{#875}
OS macOS Version 12.3 (Build 21E230)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants