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)
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.
Updated•8 years ago
|
Component: Untriaged → Bookmarks & History
Reporter | ||
Comment 2•8 years 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.
Comment 3•8 years ago
|
||
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
Updated•8 years ago
|
Flags: needinfo?(tchiovoloni)
Assignee | ||
Comment 4•8 years 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) |
Comment 6•8 years ago
|
||
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) |
Comment 8•8 years ago
|
||
(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)
Updated•8 years ago
|
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•8 years 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•8 years 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)
Comment 11•8 years ago
|
||
At this stage there are no plans to allow this folder to be removed.
Flags: needinfo?(markh)
Comment 12•8 years ago
|
||
Thom is right. It should behave as the bookmarks menu.
Flags: needinfo?(rfeeley)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•8 years 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•8 years 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•8 years 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
![]() |
||
Comment 18•8 years ago
|
||
bugherder |
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Assignee | ||
Comment 19•8 years 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.
Description
•