Mobile Bookmarks folder should always appear in the places organizer for sync users.

RESOLVED FIXED in Firefox 57

Status

()

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: matt, Assigned: tcsc)

Tracking

54 Branch
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170608105825

Steps to reproduce:

1. Installed Firefox 54
2. Removed bookmarks from Mobile Bookmarks folder.


Actual results:

Previous revisions of Firefox have removed the Mobile Bookmarks folder when emptied. In this version, however, the mobile bookmarks folder persists ONLY IN the bookmarks menu. When all bookmarks are removed from Mobile Bookmarks, the Library utility hides the folder, but it still persists at the top of the bookmarks menu.


Expected results:

The Mobile Bookmarks folder should disappear like it does in Library.
Component: Untriaged → Bookmarks & History
this is by design, afaik
Component: Bookmarks & History → Sync
(Reporter)

Comment 2

a year ago
(In reply to Marco Bonardo [::mak] from comment #1)
> this is by design, afaik

Why would the design be to hide it in the library utility but show it in the bookmarks menu? I understand that not being able to delete root folders from the bookmarks tree is by design, but the past behavior has always been to hide the empty folder, as referenced in bug 769323 before the ticket was closed. If this was intended behavior that was modified for Firefox 54, then it seems that the new behavior should also have been incorporated into the library.
That changed in bug 1295237.

> If this was intended behavior that was modified for Firefox 54,
> then it seems that the new behavior should also have been 
> incorporated into the library.

Yeah, I think that's the actual bug here.
See Also: → bug 1295237
Flags: needinfo?(tchiovoloni)
(Assignee)

Comment 4

a year ago
Yep, it looks like this is a real issue. Specifically, that the behavior is different between the places organizer and the bookmark menus.

This is trivial to fix in either direction, I'm assuming that the "correct" fix is to bring it in line with the bookmarks menu (since that was the more recent decision), but ni?rfeeley just to check.

Going to preemptively attach a patch since it's trivial, but it shouldn't land without rfeeley's feedback.
Assignee: nobody → tchiovoloni
Flags: needinfo?(tchiovoloni) → needinfo?(rfeeley)
Priority: -- → P1
Comment hidden (mozreview-request)
Thom, I expect Ryan might have trouble working out what he is being asked to decide on - could you please upload before and after screenshots for him?
Comment hidden (advocacy)
(In reply to Mark Hammond [:markh] from comment #6)
> Thom, I expect Ryan might have trouble working out what he is being asked to
> decide on - could you please upload before and after screenshots for him?

oops - forgot to ni? you for this :)
Flags: needinfo?(tchiovoloni)
Summary: Mobile Bookmarks folder no longer disappears after it's emptied → Mobile Bookmarks folder should always appear in the places organizer for sync users.

Comment 9

a year ago
mozreview-review
Comment on attachment 8879585 [details]
Bug 1372927 - Show mobile bookmarks folder in places organizer for sync users even if they have no mobile bookmarks.

https://reviewboard.mozilla.org/r/150904/#review159400

tests are failing
Attachment #8879585 - Flags: review?(markh)

Comment 10

a year ago
¡Hola Mark!

Over at https://support.mozilla.org/es/questions/1169364 there's a user a bit annoyed by the fact that this folder is apparently impossible to delete.

Would disabling Sync allow the deletion of it?

¡Gracias!
Alex
Flags: needinfo?(markh)
At this stage there are no plans to allow this folder to be removed.
Flags: needinfo?(markh)
Thom is right. It should behave as the bookmarks menu.
Flags: needinfo?(rfeeley)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

a year ago
mozreview-review
Comment on attachment 8879585 [details]
Bug 1372927 - Show mobile bookmarks folder in places organizer for sync users even if they have no mobile bookmarks.

https://reviewboard.mozilla.org/r/150904/#review155684

::: services/sync/modules/engines/bookmarks.js:1065
(Diff revision 1)
>  
>      let mobile = find(MOBILE_ANNO);
>      let queryURI = Utils.makeURI("place:folder=" + PlacesUtils.mobileFolderId);
>      let title = PlacesBundle.GetStringFromName("MobileBookmarksFolderTitle");
>  
> -    // Don't add OR remove the mobile bookmarks if there's nothing.
> +    if (mobile.length == 0) {

Worth noting this comment was wrong in either case.

::: toolkit/components/places/tests/unit/test_sync_utils.js:2869
(Diff revision 2)
>  
>    do_print("Try creating query after organizer is ready");
>    await PlacesSyncUtils.bookmarks.ensureMobileQuery()
>    let queryGuids = await PlacesSyncUtils.bookmarks.fetchGuidsWithAnno(
>      "PlacesOrganizer/OrganizerQuery", "MobileBookmarks");
> -  equal(queryGuids.length, 0, "Should not create query without any mobile bookmarks");
> +  equal(queryGuids.length, 1, "Should create query without any mobile bookmarks");

The deletions here are because we no longer have separate cases for having vs not having mobile bookmarks. If you'd prefer I keep the "insert one bookmark, ensureMobileBookmarks, then check" case instead, let me know.

Comment 15

a year ago
mozreview-review
Comment on attachment 8879585 [details]
Bug 1372927 - Show mobile bookmarks folder in places organizer for sync users even if they have no mobile bookmarks.

https://reviewboard.mozilla.org/r/150904/#review171510

This looks fine, thanks.
Attachment #8879585 - Flags: review?(markh) → review+
Comment hidden (mozreview-request)

Comment 17

a year ago
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7055f74e50fe
Show mobile bookmarks folder in places organizer for sync users even if they have no mobile bookmarks. r=markh
https://hg.mozilla.org/mozilla-central/rev/7055f74e50fe
Status: UNCONFIRMED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Comment 19

a year ago
Clearing my needinfo on this bug since it's resolved
Flags: needinfo?(tchiovoloni)
You need to log in before you can comment on or make changes to this bug.