Brave-browser: Desktop Bookmark Platform Alignment

[jhreis]

Description

Please carefully read this in its entirety, and if any issues are known, please raise them before starting work. If any of the below are discovered to be impossible during development, please raise concerns before completing the rest of the items, as these are all tightly coupled together.

Currently each Brave platform has variations to bookmarks that can often lead to unexpected syncing behavior. The following changes are outlined to help align desktop with the mobile platforms and how bookmarks are presented to the user.

The main change surrounds the function of “Other Bookmarks” on Chromium. Most changes outlined below related solely to UI modifications. The main exception to this is a migration process to move some bookmarks to a new folder.

UI Menu Changes

The menu items below are currently always present, and should be hidden from the user completely.

1. Hide/Delete “Other Bookmarks” from “Bookmark Manager”:

2. Hide/Delete “Other Bookmarks” from Bookmark Edit menu:

3. Hide/Delete “Other Bookmarks” from Bookmark Ad menu:

Other Changes

4. Migration Bookmarks inside “Other Bookmarks” to new folder.

The new folder will share the _exact same title_ as “Other Bookmarks”. It should be noted that the title of this folder is localized, and should not just be titled “Other Bookmarks”, but should be titled with the literal string.

The newly created “Other Bookmarks” folder should be _inside_ the “Bookmarks Bar” folder (see step 5 below for additional note on this).

All user bookmarks from the Chromium “Other Bookmarks” should be moved into this folder. This folder has no special permissions, it is the same as a user-created folder. e.g. the user can rename it, delete it, move it, etc…

See screenshot for expected outcome. Notice that "Other Bookmarks" is no longer a root node, and is nested under the only present root node.

5. Remove "Bookmarks Bar" node in Bookmark Manager (or rename)

Ideally "Bookmarks Bar" would also be removed from the Bookmark Manager, and bookmarks just listed linearly:

If this is not possible then it would be tremendous if we could at least rename it to simply "Bookmarks" (see step 6 for additional info about this)

6. Rename "Bookmarks Bar" to just "Bookmark" on edit / add menu for bookmarks (if possible)

Rename the two menus below:

---

Miscellaneous Information:

The outcome of this issue is that the user has one, and only one root level node/“folder”. This will be titled “Bookmarks” and _all_ bookmarks will be placed inside of this.

Please have @jhreis review any associated PRs to verify behavior.

Reproduces how often:

N/A

Brave version (brave://version info)

N/A

Version/Channel Information:

• Can you reproduce this issue with the current release? N/A
• Can you reproduce this issue with the beta channel? N/A
• Can you reproduce this issue with the dev channel? N/A
• Can you reproduce this issue with the nightly channel? N/A

Other Additional Information:

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

[btlechowski]

Verified passed with

Brave | 1.2.11 Chromium: 78.0.3904.108 (Official Build) dev (64-bit)
-- | --
Revision | 4b26898a39ee037623a72fcfb77279fce0e7d648-refs/branch-heads/3904@{#889}
OS | Ubuntu 18.04 LTS

Verified test plan from https://github.com/brave/brave-core/pull/3620

Old Brave:

Brave | 1.0.1 Chromium: 78.0.3904.108 (Official Build) (64-bit)
-- | --
Revision | 4b26898a39ee037623a72fcfb77279fce0e7d648-refs/branch-heads/3904@{#889}
OS | Windows 7 Service Pack 1 (Build 7601.24530)

Brave | 1.0.1 Chromium: 78.0.3904.108 (Official Build) (64-bit)
-- | --
Revision | 4b26898a39ee037623a72fcfb77279fce0e7d648-refs/branch-heads/3904@{#889}
OS | Ubuntu 18.04 LTS

Bookmarks Bar to Bookmarks

Other Bookmarks on clean profile

• Bookmark Add/Edit Menu in the Address bar
  

• Hamburger menu
  

• Edit bookmark in Hamburger menu and bookmark bar
  

• brave://bookmarks and Bookmark bar
  

Other Bookmarks on upgraded profile without sync

• Old Brave
  

• Verified that Other Bookmarks folder is moved to Bookmarks Bar (verified with @darkdh)

• New Brave - Bookmark Add/Edit Menu in the Address bar
  

• New Brave - Hamburger menu
  

• New Brave - Edit bookmark in Hamburger menu and bookmark bar
  

• New Brave - brave://bookmarks and Bookmark bar
  

Other Bookmarks with sync - both devices on old Brave

• Verified that upon upgrade Other Bookmarks folder is moved to Bookmarks Bar on Device A(New Brave)
  

• Verified that on Device B(Old Brave) upon upgrade all bookmarks are moved from Other Bookmarks to Bookmark Bar\Other Bookmarks
  

• Verified Other Bookmarks in Bookmark Add/Edit Menu in the Address bar

• Verified Other Bookmarks in Hamburger menu
• Verified Other Bookmarks in Edit bookmark in Hamburger menu and bookmark bar
• Verified Other Bookmarks in brave://bookmarks and Bookmark bar

Other Bookmarks with sync - Device A(New Brave), Device B(Old Brave)

• Logged https://github.com/brave/brave-browser/issues/7115

Other Bookmarks with sync - Device A(New Brave), Device B(New Brave)

• Verified Other Bookmarks in Bookmark Add/Edit Menu in the Address bar
• Verified Other Bookmarks in Hamburger menu
• Verified Other Bookmarks in Edit bookmark in Hamburger menu and bookmark bar
• Verified Other Bookmarks in brave://bookmarks and Bookmark bar
• Verified on clean and upgrade profile

Verification passed on

Brave | 1.2.39 Chromium: 79.0.3945.88 (Official Build) beta (64-bit)
-- | --
Revision | c2a58a36b9411c80829b4b154bfcab97e581f1f3-refs/branch-heads/3945@{#954}
OS | Windows 10 OS Version 1803 (Build 17134.1006)

• Verified test plan from https://github.com/brave/brave-core/pull/3620
• 
  
• 
  
• 
  

Other Bookmarks with sync disabled - Old Brave 1.1.23

Other bookmarks in bookmarks bar

Other bookmarks in bookmarks Manager

Other bookmarks in bookmarks Add

Other bookmarks in bookmarks Edit

Other bookmarks in Hamburger menu

Launch New brave (Bravebeta 1.2.39 ) with old profile 1.1.23

• Verified that all existing bookmarks in "Other Bookmarks" are moved to the same name folder at the end of "Bookmarks Bar"

Other bookmarks in bookmarks bar

Other bookmarks in bookmarks Manager

Other bookmarks in bookmarks Add

Other bookmarks in bookmarks Edit

Other bookmarks in Hamburger menu

Other Bookmarks with sync - Device A(Old Brave), Device B(Old Brave)
Device A - Windows 10 running Brave 1.1.23
Device B - windows 8 running Brave 1.1.23

• After sync, other bookmarks displayed in same position in Device (A, B )
  

Verified Other Bookmarks in Bookmark Add/Edit Menu in the Address bar

• Verified Other Bookmarks in Hamburger menu
• Verified Other Bookmarks in Edit bookmark in Hamburger menu and bookmark bar
• Verified Other Bookmarks in brave://bookmarks and Bookmark bar

Other Bookmarks with sync - Device A(New Brave), Device B(Old Brave)
Device A - Windows 10 running Bravebeta 1.2.39

• Verified that upon upgrade Other Bookmarks folder is moved to Bookmarks Bar on Device A(New Brave)
• Verified Other Bookmarks in Bookmark Add/Edit Menu in the Address bar
• Verified Other Bookmarks in Hamburger menu
• Verified Other Bookmarks in Edit bookmark in Hamburger menu and bookmark bar
• Verified Other Bookmarks in brave://bookmarks and Bookmark bar
• Verified on clean and upgrade profile

Verification PASSED on macOS 10.15.2 x64 using the following build:

Brave | 1.2.41 Chromium: 79.0.3945.88 (Official Build) (64-bit)
-- | --
Revision | c2a58a36b9411c80829b4b154bfcab97e581f1f3-refs/branch-heads/3945@{#954}
OS | macOS Version 10.15.2 (Build 19C57)

Renaming Bookmarks Bar --> Bookmarks

Hide Other Bookmarks while Sync is disabled while upgrading from 1.1.23 --> 1.2.41**

Other Bookmarks appearing under 1.1.23 CR: 79.0.3945.88 before upgrading

Other Bookmarks moved under Bookmarks after upgrading to 1.2.41 Chromium: 79.0.3945.88 with Sync being disabled

Other Bookmarks moved under Bookmarks after upgrading to 1.2.41 Chromium: 79.0.3945.88 with Sync being enabled

[Rob4226]

This update on Windows 10 just broke a bookmark extension xBrowserSync and I had no idea why all day until I finally looked here. Is there any way to change it back (with syncing off), unnesting the Other Bookmarks folder? Thank you.

[Hardtarget24]

This broke having "Other Bookmarks" being on the right side in a separate area of the bookmarks bar. how do we get this functionality restored?

[bsclifton]

@Hardtarget24 there's an issue up at https://github.com/brave/brave-browser/issues/7639 if you want to leave a comment (and subscribe for updates)

I had closed as wontfix for the moment, but @rebron is discussing with a few others

[nero120]

As this change potentially breaks compatibility with any extension that uses the bookmarks api, what's the plan for dealing with that? Will you be launching your own extensions store shortly since extension authors that use the bookmarks api will now need to provide a separate build for brave...?

I suppose you'll need to update this page from:

Brave offers support for nearly all extensions that are compatible with chromium.

to something like

Brave offers support for some extensions that are compatible with chromium (up to you to work out which though)

Any guidance would be appreciated!

[Hardtarget24]

@Hardtarget24 there's an issue up at #7639 if you want to leave a comment (and subscribe for updates)

I had closed as wontfix for the moment, but @rebron is discussing with a few others

I've done that but I think this issue has larger complications than you guys realized based on nero and rob's replies.

[antonycourtney]

I'm the author of the Tabli tab manager extension for Chrome (https://chrome.google.com/webstore/detail/tabli/igeehkedfibbnhbfponhjjplpkeomghi).
In addition to window and tab switching, Tabli allows the user to save windows and tabs, which are stored as bookmarks under "Other Bookmarks/Tabli Saved Windows".

I never tested with Brave, but users reported successfully using Tabli with Brave, until today...

Is there anything like a compatibility guide for extension authors, providing some guidance on how to deal with bookmarks stored under "Other Bookmarks" to support both Chrome and Brave in the presence of this change?

Was there any staging of this change, such as an interim release before making the breaking change that would log and warn about extensions with this implicit dependency on "Other Bookmarks", or an attempt to warn users or extension of authors of impending breakage?

[bridiver]

if you are trying to create bookmarks in the "Other Bookmarks" folder you should just leave the parentId blank. That will work correctly in Chrome and Brave.

[bridiver]

by "correctly" I mean that it will go in the default folder which is "Other Bookmarks" on Chrome and the bookmarks bar on Brave

[bridiver]

another option would be to use children.length - 1 as the index

[bridiver]

Also relying on undocumented fixed bookmark node ids seems dangerous to me. We could potentially make changes to the bookmarks api to create a "shadow" node to have the same number of children as chrome, but this issue is closed and a new issue should be opened for this so it can be appropriately triaged and assigned a priority.

[darkdh]

The active issue is https://github.com/brave/brave-browser/issues/7639

[nero120]

if you are trying to create bookmarks in the "Other Bookmarks" folder you should just leave the parentId blank. That will work correctly in Chrome and Brave.

Thanks for the tip, I'll test this and report back as soon as I can.

Also relying on undocumented fixed bookmark node ids seems dangerous to me. We could potentially make changes to the bookmarks api to create a "shadow" node to have the same number of children as chrome, but this issue is closed and a new issue should be opened for this so it can be appropriately triaged and assigned a priority.

Could not agree with you more, it is a real shame that the chromium devs didn't include methods to retrieve container nodes when they originally implemented the bookmarks API.

[bridiver]

@nero120 I was surprised they didn't have constants for the bookmarks bar and other bookmarks node. I expected to find them in the api docs and use it as a potential fix inside the api

[nero120]

@bridiver there have been instances whereby a few of my Chrome users have reported issues in the past that lead to the discovery that their Chrome bookmarks data file has different ids for other bookmarks and bookmarks bar. Could be that the ids are not fixed for these nodes by design, hence no constant.

I don't know how the internal browser code identifies these nodes, but it can't be by id obviously. Though as I said before, shame they didn't include API methods that use the same logic to return the node(s).

[bridiver]

well, there are different ways they could handle the constant in the c++ code so it works even if the actual ids are not deterministic, but in any case it does seem odd that the only apparent way to accurately identify them is by folder name

[nero120]

@bridiver you can't even do that as the container node title values are different for each language 😭

[antonycourtney]

I moved my comments over to open issue #7639. Propose we continue the discussion there.

[bridiver]

I guess in that case maybe array index is the best option, but I think length - 1 may not work correctly on Chrome if you have mobile bookmarks from android

On Jan 8, 2020, at 4:07 PM, Rich [e-mail] wrote:


@bridiver you can't even do that as the container node title values are different for each language 😭

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

[bridiver]

Actually you could use the l10n apis to handle that, no?

On Jan 8, 2020, at 4:07 PM, Rich [e-mail] wrote:


@bridiver you can't even do that as the container node title values are different for each language 😭

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

[nero120]

Actually you could use the l10n apis to handle that, no?

Thought you could only access your own locale strings using that API?

[bridiver]

Actually you could use the l10n apis to handle that, no?

Thought you could only access your own locale strings using that API?

Right, but you would search based on the localized string so it should always match.

[nero120]

Right, but you would search based on the localized string so it should always match.

Sorry I meant that the extension would need to package the strings itself for every locale, like so:

 |
 ------ en
 |       |
 |       ------ messages.json
 |               |
 |               ------ "bookmarksbar_title": { "message": "Bookmarks bar" }
 |               |
 |               ------ "otherbookmarks_title": { "message": "Other bookmarks" }
 |
 ------ es
         |
         ------ messages.json
                 |
                 ------ "bookmarksbar_title": { "message": "Barra de marcadores" }
                 |
                 ------ "otherbookmarks_title": { "message": "Otros marcadores" }
 |
 [etc]

Possible but horrid! Would be far better for the bookmarks API to present a way to request container nodes.

[bridiver]

Right, but you would search based on the localized string so it should always match.

Sorry I meant that the extension would need to package the strings itself for every locale, like so:

 |
 ------ en
 |       |
 |       ------ messages.json
 |               |
 |               ------ "bookmarksbar_title": { "message": "Bookmarks bar" }
 |               |
 |               ------ "otherbookmarks_title": { "message": "Other bookmarks" }
 |
 ------ es
         |
         ------ messages.json
                 |
                 ------ "bookmarksbar_title": { "message": "Barra de marcadores" }
                 |
                 ------ "otherbookmarks_title": { "message": "Otros marcadores" }
 |
 [etc]

Possible but horrid! Would be far better for the bookmarks API to present a way to request container nodes.

Ok, I assumed getMessage would just work because the string is already localized for the UI

[nero120]

Ok, I assumed getMessage would just work because the string is already localized for the UI

Yeah that would have been useful, but I believe the API only access your own implemented I18n strings, not internal browser ones. There are some predefined strings but nothing that would help us.

[bridiver]

Ok, I assumed getMessage would just work because the string is already localized for the UI

Yeah that would have been useful, but I believe the API only access your own implemented I18n strings, not internal browser ones. There are some predefined strings but nothing that would help us.

Haha, that's great, so you have either 2 or 3 root nodes (depending on whether or not you're syncing with mobile) and the only way to identify them is to hope they're in the order you expect them to be in? I guess you could create a bookmark without specifying the parentId (so it goes in "other bookmarks") and then check the parentId value in the callback, but you still have no documented reliable way to differentiate between bookmarks bar and mobile bookmarks if there are 3 root nodes. I also love how the example uses bookmarkBar.id, but doesn't bother to tell you how they got the bookmarkBar in the first place :)

chrome.bookmarks.create({'parentId': bookmarkBar.id,
                               'title': 'Extension bookmarks'},
                              function(newFolder) {
        console.log("added folder: " + newFolder.title);
      });

[bridiver]

and fyi - in the c++ code there are several ways to get the nodes

const BookmarkPermanentNode* BookmarkModel::PermanentNode(
    BookmarkNode::Type type) {
  DCHECK(loaded_);
  switch (type) {
    case BookmarkNode::BOOKMARK_BAR:
      return bookmark_bar_node_;
    case BookmarkNode::OTHER_NODE:
      return other_node_;
    case BookmarkNode::MOBILE:
      return mobile_node_;
    default:
      NOTREACHED();
      return nullptr;
  }
}

and
````
// Returns the root node. The 'bookmark bar' node and 'other' node are
// children of the root node.
const BookmarkNode* root_node() const { return root_; }

// Returns the 'bookmark bar' node. This is NULL until loaded.
const BookmarkNode* bookmark_bar_node() const { return bookmark_bar_node_; }

// Returns the 'other' node. This is NULL until loaded.
const BookmarkNode* other_node() const { return other_node_; }

// Returns the 'mobile' node. This is NULL until loaded.
const BookmarkNode* mobile_node() const { return mobile_node_; }
```