Closed Bug 1372927 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Sync, defect, P1)

54 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: matt, Assigned: tcsc)

References

Details

Attachments

(1 file)

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
(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: → 1295237
Flags: needinfo?(tchiovoloni)
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
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?
(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 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)
¡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)
See Also: → 769323
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 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+
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
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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.

Attachment

General

Created:
Updated:
Size: