search

brave / brave-browser

Brave browser for Android, iOS, Linux, macOS, Windows.
https://brave.com
Mozilla Public License 2.0
17.72k stars 2.31k forks source link

Desktop Bookmark Platform Alignment #5158

Closed jhreis closed 5 years ago

jhreis commented 5 years ago

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”:

1

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

2

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

3

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.

4

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)

5

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

Rename the two menus below:

6

7

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:

Other Additional Information:

btlechowski commented 4 years ago

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 image image image

Other Bookmarks on clean profile

Other Bookmarks on upgraded profile without sync

Other Bookmarks with sync - both devices on old Brave

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

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

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)

Other Bookmarks with sync disabled - Old Brave 1.1.23

Other bookmarks in bookmarks bar image Other bookmarks in bookmarks Manager image Other bookmarks in bookmarks Add image Other bookmarks in bookmarks Edit image Other bookmarks in Hamburger menu image

Launch New brave (Bravebeta 1.2.39 ) with old profile 1.1.23

Other bookmarks in bookmarks bar image Other bookmarks in bookmarks Manager image Other bookmarks in bookmarks Add image Other bookmarks in bookmarks Edit image Other bookmarks in Hamburger menu image 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

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

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

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

Screen Shot 2020-01-06 at 2 10 14 PM Screen Shot 2020-01-06 at 2 07 04 PM Screen Shot 2020-01-06 at 2 08 40 PM Screen Shot 2020-01-06 at 2 13 52 PM Screen Shot 2020-01-06 at 2 15 13 PM Screen Shot 2020-01-06 at 2 18 08 PM

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

Screen Shot 2020-01-06 at 2 36 24 PM Screen Shot 2020-01-06 at 2 38 43 PM Screen Shot 2020-01-06 at 2 39 30 PM Screen Shot 2020-01-06 at 2 40 14 PM Screen Shot 2020-01-06 at 2 46 10 PM Screen Shot 2020-01-06 at 2 45 32 PM

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

Screen Shot 2020-01-06 at 3 05 56 PM Screen Shot 2020-01-06 at 3 07 10 PM Screen Shot 2020-01-06 at 3 08 11 PM Screen Shot 2020-01-06 at 3 09 19 PM Screen Shot 2020-01-06 at 3 09 59 PM

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

Screen Shot 2020-01-06 at 3 43 30 PM Screen Shot 2020-01-06 at 3 43 57 PM Screen Shot 2020-01-06 at 3 44 37 PM Screen Shot 2020-01-06 at 3 45 11 PM
rob4226 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

bridiver commented 4 years ago

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 commented 4 years ago

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

nero120 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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

antonycourtney commented 4 years ago

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

bridiver commented 4 years ago

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 notifications@github.com 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 commented 4 years ago

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

On Jan 8, 2020, at 4:07 PM, Rich notifications@github.com 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 commented 4 years ago

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

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

bridiver commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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