[Bug] Desktop Bookmarks are only visible when signed-in

[grigoryk]

Currently we only display desktop bookmarks in the bookmarks library view if the user is signed-in.

If I sync my desktop bookmarks and then sign-out, I am no longer able to access them unless I sign-in again. At least from that point of view, this clearly reads as a bug to me.

I think display of the Desktop bookmarks folder in the library and edit views should be predicated on the presence of the desktop roots in the tree instead.

This should work fine for editing as well. If the user wants to move a bookmark into one of the desktop folders, there's no problem there as long as we actually display desktop bookmarks in the library. If they re-connect sync, our bookmark merging logic should sensibly merge their local edits with the remote desktop tree. Merging two complete bookmarks trees is one of the benchmark use cases for https://github.com/mozilla/dogear, which is what we use to do the heavy lifting here.

┆Issue is synchronized with this Jira Task

[grigoryk]

Actually, displaying desktop bookmarks folder only if "desktop roots" exists assumes a little too much about the behaviour of underlying storage. Off top of my head, I'm not actually sure if desktop roots aren't inserted when we initially create our bookmark storage. Relying on that undocumented and internal behaviour of the storage library at the UI level seems quite wrong.

Alternative is to always display Desktop bookmarks, and use it as an opportunity to promote Firefox Accounts by displaying some kind of a "sign into FxA to sync your desktop bookmarks" banner inside of Desktop Bookmarks if user doesn't have any bookmarks, or if the user isn't logged-in.

[yoasif]

Alternative is to always display Desktop bookmarks, and use it as an opportunity to promote Firefox Accounts by displaying some kind of a "sign into FxA to sync your desktop bookmarks" banner inside of Desktop Bookmarks if user doesn't have any bookmarks, or if the user isn't logged-in.

👍

[grigoryk]

Off top of my head, I'm not actually sure if desktop roots aren't inserted when we initially create our bookmark storage

Desktop roots are created whenever we initialize our storage for the first time. So, we always have them. It makes sense to also always display them as described in #4046 (comment)

[colintheshots]

We had the opposite bug beforehand suggesting that Desktop Bookmarks should not appear when we were signed out. I think it's important to have clear UX guidelines here. Keeping the Bookmarks could cause syncing problems when they sign in again as another user. #3333

We have a sign in button that appears when the user is not signed in to encourage using accounts, so adding an empty Desktop Bookmarks folder shouldn't be necessary.

I see two options, keep the current behavior or keep a user identifier hash to compare to know to flush the existing Bookmarks in Desktop Bookmarks if the identifier changes. It seems it would not likely be desirable to merge two users' Bookmarks under Desktop Bookmarks and sync them across.

[grigoryk]

I feel pretty strongly that #3333 was misguided, or at least the solution we ended up with didn't really consider UX implications. What we have now doesn't align with what we do in our other browsers.

I agree that there's some subtlety involved here, but we pretty clearly have a broken experience right now, where a lot of the bookmarks are hidden from the user for no apparent reason.

Keeping the Bookmarks could cause syncing problems when they sign in again as another user

That's not how this works. By hiding desktop bookmarks from the UI, sync isn't actually affected in any way. After signing out, all bookmarks (mobile & desktop) remain in the database, on the device. Currently the only thing we're doing is hiding those whose parent is one of the desktop roots from the UI. When user logs in with a different account, we will synchronize those bookmarks against that new account. If that account already has some bookmarks, we will perform a full tree merge. Ability to do this well is one of the "corner-stone' cases of the new bookmark sync implementation.

It seems it would not likely be desirable to merge two users' Bookmarks under Desktop Bookmarks and sync them across.

Let's not re-invent the wheel; we have an established pattern already on other platforms: we warn the user that their data will be merged when they sign-into another account. I can't find an existing ticket for this - it needs to be filed. @ryanfeeley is the person to discuss this with.

Another ticket we need to file is about offering users to wipe their local data when they sign-out of sync. This is what we do on Desktop to protect against accidental data merges.

Just to underscore: the current behaviour is just a UI that hides contents of the database. Actual bookmarks aren't affected by signin-out, and so right now we will upload any already-present desktop bookmarks to any new sync account that comes along. However, the current UI hides that from the user, in a way that may mislead to them to think we've deleted bookmarks after they signed-out!

[topotropic]

What about the following:

• Don't show desktop folder for people who haven't signed in
• Show desktop folder for people who signed in and synced bookmarks –  also show folder after log out
• When someone logs in with a different account, show a warning and merge – like described in above comment [Bug] Desktop Bookmarks are only visible when signed-in #4046 (comment)

[grigoryk]

Re-surfacing this issue, since we're starting to test Fennec bookmark migrations and it'll be useful to see what we're actually migrating :)

Don't show desktop folder for people who haven't signed in
Show desktop folder for people who signed in and synced bookmarks –  also show folder after log out

@topotropic is this distinction really valuable? It will add a bunch of complexity we'll have to maintain. We will need to differentiate between "never signed-in", "was signed-in and synced bookmarks", "was signed-in and didn't sync bookmarks". And once we start handling migrations, we'll need to further differentiate between past Fennec states, as well.

The much simpler alternative would be to always display the desktop folder, and if user is signed-out and looks into it, display a promo banner for Sync, with messaging about how users can synchronize bookmarks between desktop and mobile if they Sign In (with a button below). As a bonus, this will also help us support our FxA user acquisition goals.

[topotropic]

@grigoryk You're right – let's follow your suggestions.

[ekager]

Reopening for QA. New behavior is:

Always display the desktop folder whether or not user is signed in to Sync, and if user is signed-out and looks into it, display a button to log in to Sync.

[AndiAJ]

Hi, verified as fixed on the latest Nightly Build #21130610 from 4/22 using the following devices:
• Google Pixel 3a (Android 10)
• Huawei Mate 20 Lite (Android 9)
• OnePlus A3 (Android 6.0.1)

✔️ Always display the desktop folder whether or not user is signed in to Sync, and if user is signed-out and looks into it, display a button to log in to Sync.

✔️ If the user wants to move a bookmark into one of the desktop folders, the bookmark merging logic with the desktop tree is properly working.

► Video