Desktop Bookmark Platform Alignment #5158

jhreis · 2019-07-05T21:40:00Z

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 · 2019-11-27T01:22:30Z

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 brave/brave-core#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 Follow up to #5158 - Bookmark from Other Bookmarks moved to Bookmark bar after sync #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 Hide other bookmarks brave-core#3620

2.

3.

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 · 2020-01-07T07:33:23Z

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 · 2020-01-07T21:35:07Z

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 · 2020-01-07T22:40:44Z

@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

nero120 · 2020-01-07T23:18:00Z

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 · 2020-01-07T23:54:34Z

@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 · 2020-01-08T04:32:29Z

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 · 2020-01-08T20:59:32Z

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 · 2020-01-08T21:02:20Z

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 · 2020-01-08T21:12:22Z

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

bridiver · 2020-01-08T21:32:25Z

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 · 2020-01-08T21:55:20Z

The active issue is #7639

nero120 · 2020-01-08T22:02:56Z

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 · 2020-01-08T22:25:08Z

@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 · 2020-01-08T22:29:52Z

@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 · 2020-01-08T23:06:04Z

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 · 2020-01-08T23:07:35Z

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

antonycourtney · 2020-01-09T04:09:45Z

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

bridiver · 2020-01-09T15:55:24Z

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 ***@***.***> 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 · 2020-01-09T15:58:46Z

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

…

On Jan 8, 2020, at 4:07 PM, Rich ***@***.***> 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 · 2020-01-09T16:47:04Z

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

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

bridiver · 2020-01-09T16:57:54Z

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 · 2020-01-09T20:08:15Z

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:

_locales
 |
 ------ 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 · 2020-01-09T20:14:12Z

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:
_locales
 |
 ------ 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 · 2020-01-09T20:18:08Z

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 · 2020-01-09T23:43:26Z

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 · 2020-01-10T00:05:23Z

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_; }
```

srirambv added the feature/bookmarks label Jul 7, 2019

darkdh self-assigned this Sep 27, 2019

darkdh added the feature/sync label Sep 27, 2019

This was referenced Oct 3, 2019

Consecutive sync record CREATE, UPDATE won't be applied on receiving end #6310

Closed

Bookmarks sync to wrong folder (Other bookmarks to Mobile bookmarks) brave/sync#312

Closed

Hide other bookmarks brave/brave-core#3620

Merged

darkdh added 1.0 QA/Yes labels Oct 8, 2019

darkdh closed this as completed in brave/brave-core#3620 Oct 16, 2019

darkdh added this to the 0.73.x - Nightly milestone Oct 16, 2019

AlexeyBarabash mentioned this issue Oct 22, 2019

Create bookmarks from chrome API in 'Bookmarks bar' instead 'Other bookmarks' #6578

Closed

darkdh removed the 1.0 label Oct 28, 2019

btlechowski mentioned this issue Nov 27, 2019

Follow up to #5158 - Bookmark from Other Bookmarks moved to Bookmark bar after sync #7115

Closed

btlechowski added the QA Pass-Linux label Nov 27, 2019

This was referenced Dec 24, 2019

Manual test run on OS X for 1.2.x - Beta #7519

Closed

Manual test run on Windows x64 for 1.2.x - Beta #7520

Closed

GeetaSarvadnya added the QA Pass-Win64 label Jan 2, 2020

LaurenWags added the release-notes/include label Jan 6, 2020

LaurenWags mentioned this issue Jan 6, 2020

Release Notes for 1.2.x (Release Channel) #7615

Closed

kjozwiak added the QA Pass-macOS label Jan 6, 2020

rob4226 mentioned this issue Jan 7, 2020

Brave Browser Support xbrowsersync/app#100

Closed

fhoshino mentioned this issue Jan 7, 2020

Fail to fetch bookmarks in brave 1.2.41 ChrisNZL/Fauxbar#55

Closed

bsclifton mentioned this issue Jan 7, 2020

Other bookmarks is missing, breaking bookmarks API #7639

Closed

frbaxter mentioned this issue Jan 13, 2020

Tabli not working with Brave browser after latest update antonycourtney/tabli#138

Closed

srirambv mentioned this issue Apr 6, 2020

Sites getting classified as untargeted #9013

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop Bookmark Platform Alignment #5158

Desktop Bookmark Platform Alignment #5158

jhreis commented Jul 5, 2019

btlechowski commented Nov 27, 2019 •

edited by kjozwiak

Loading

rob4226 commented Jan 7, 2020 •

edited

Loading

Hardtarget24 commented Jan 7, 2020

bsclifton commented Jan 7, 2020

nero120 commented Jan 7, 2020

Hardtarget24 commented Jan 7, 2020

antonycourtney commented Jan 8, 2020

bridiver commented Jan 8, 2020

bridiver commented Jan 8, 2020

bridiver commented Jan 8, 2020

bridiver commented Jan 8, 2020

darkdh commented Jan 8, 2020

nero120 commented Jan 8, 2020

bridiver commented Jan 8, 2020

nero120 commented Jan 8, 2020

bridiver commented Jan 8, 2020

nero120 commented Jan 8, 2020

antonycourtney commented Jan 9, 2020

bridiver commented Jan 9, 2020 via email

bridiver commented Jan 9, 2020 via email

nero120 commented Jan 9, 2020

bridiver commented Jan 9, 2020

nero120 commented Jan 9, 2020

bridiver commented Jan 9, 2020

nero120 commented Jan 9, 2020

bridiver commented Jan 9, 2020 •

edited

Loading

bridiver commented Jan 10, 2020

Desktop Bookmark Platform Alignment #5158

Desktop Bookmark Platform Alignment #5158

Comments

jhreis commented Jul 5, 2019

Description

UI Menu Changes

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.

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

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

Miscellaneous Information:

Reproduces how often:

Brave version (brave://version info)

Version/Channel Information:

Other Additional Information:

btlechowski commented Nov 27, 2019 • edited by kjozwiak Loading

Renaming Bookmarks Bar --> Bookmarks

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

rob4226 commented Jan 7, 2020 • edited Loading

Hardtarget24 commented Jan 7, 2020

bsclifton commented Jan 7, 2020

nero120 commented Jan 7, 2020

Hardtarget24 commented Jan 7, 2020

antonycourtney commented Jan 8, 2020

bridiver commented Jan 8, 2020

bridiver commented Jan 8, 2020

bridiver commented Jan 8, 2020

bridiver commented Jan 8, 2020

darkdh commented Jan 8, 2020

nero120 commented Jan 8, 2020

bridiver commented Jan 8, 2020

nero120 commented Jan 8, 2020

bridiver commented Jan 8, 2020

nero120 commented Jan 8, 2020

antonycourtney commented Jan 9, 2020

bridiver commented Jan 9, 2020 via email

bridiver commented Jan 9, 2020 via email

nero120 commented Jan 9, 2020

bridiver commented Jan 9, 2020

nero120 commented Jan 9, 2020

bridiver commented Jan 9, 2020

nero120 commented Jan 9, 2020

bridiver commented Jan 9, 2020 • edited Loading

bridiver commented Jan 10, 2020

btlechowski commented Nov 27, 2019 •

edited by kjozwiak

Loading

Renaming `Bookmarks Bar` --> `Bookmarks`

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

rob4226 commented Jan 7, 2020 •

edited

Loading

bridiver commented Jan 9, 2020 •

edited

Loading