|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0 Build ID: 20160813030202 Steps to reproduce: Currently, when a user that has Firefox Sync moves bookmarks from a mobile device running Firefox to a desktop version of Firefox, the bookmarks that have been synchronized do not appear in the “Show your bookmarks” drop down from the tool bar. Actual results: The user has to go into “Show all bookmarks” and look for the “Mobile bookmarks” folder. Nothing tells them to do this, so it can make it look like Sync does not work, or that information has “vanished”. Expected results: To solve this, the "Mobile bookmarks" folder should also appear in the same block as “Bookmarks Toolbar” and "Other bookmarks" in the “Show your bookmarks” drop down from the tool bar.
I agree that we should do this, and I expect it's probably not too difficult (although OTOH, a quick look didn't make it obvious why that folder isn't already being shown)
I'm adding this to a feature card I am working on in our product board. There are a few different improvements we can do around improving mobile bookmark discovery.
Unsure about the approach here. We don't have a "Mobile Bookmarks" query (e.g. it won't show up in the places organizer" until we have successfully completed a bookmark sync once. It seems wrong to show these before then, so I set a pref at the same time we would add the query. It's a bit cludgey to use a pref for this, but AIUI we need to add the menu item during the onpopupshowing event or it won't show up on Mac, which won't update that list on the fly (although it's possible this is not true and I'm just misunderstanding some comments), which means it needs to be synchronous, and it seems like a rather bad place to spin a nested event loop on a sql query. Also worth noting is we don't have a good icon for this. Right now it shows up like a normal bookmark folder. I feel like to land we'd want it to have a special icon? Could be wrong though -- and we might have this and I just don't know where.
Comment on attachment 8803979 [details] Bug 1295237 - Add 'Mobile Bookmarks' root to the bookmark menus if we have it. https://reviewboard.mozilla.org/r/88170/#review87542 Same comments as I added in bug 1296564 :)
The reason I didn't go for something along the lines of checking the services.sync.username and services.sync.engine.bookmarks pref is that if you did that, you wouldn't be able to access any mobile bookmarks you had if you e.g. turned off bookmark sync after having it on for a while.
Created attachment 8804791 [details] screenshot.png :rfeeley, do you think this (the Mobile Bookmarks folder) looks OK? Only shows up for users who have bookmark sync on.
(In reply to Thom Chiovoloni [:tcsc] from comment #6) > The reason I didn't go for something along the lines of checking the > services.sync.username and services.sync.engine.bookmarks pref is that if > you did that, you wouldn't be able to access any mobile bookmarks you had if > you e.g. turned off bookmark sync after having it on for a while. Conversely, if you setup Sync once, never create a mobile bookmark, then disconnect, it would be reasonable to expect that menu to be in the same state as if you never used Sync. How hard would it be to only show it if there are actually items in the folder? I think that aligns closer to some work Kit is doing by making the mobile query a first-class query.
And more to the point, disconnecting sync will reset that pref!
Some thoughts: - What will this look like when iOS is uploading bookmarks? Will mobile merge both? Or will we split it by OS? - I would recommend never hiding the folder. I think it could be useful for the user to know that they can have their mobile bookmarks on their desktop. If the user isn't syncing mobile bookmarks, perhaps we put links to download our apps in the folder?
(In reply to Alex Davis [:adavis] from comment #10) > Some thoughts: > - What will this look like when iOS is uploading bookmarks? Will mobile > merge both? Or will we split it by OS? Good question - I'm not sure what UX iOS will be offering. But ultimately desktop doesn't really case - some other device ends up sticking bookmarks in some folder - desktop doesn't care what folder it uses, not can it (practically) tell what device added it. ie, Android currently only writes to the "mobile" folder currently IIUC, but there's nothing stopping it from growing a full-blown bookmark editing UI where it could do anything to any folder. > - I would recommend never hiding the folder. I think it could be useful for > the user to know that they can have their mobile bookmarks on their desktop. I agree in general, although it's worth noting that the bookmarks sidebar and "show all bookmarks" window do *not* show a mobile folder until Sync creates it. It would seem strange to me for this dropdown to show "mobile" and those other windows to not show it. There's a bug on file for those 2 windows always showing mobile and offering a "sign in to sync" panel, which I agree is where we want to end up, and this dropdown should have the same behaviour. The real question is what do we do in the meantime? Maybe nothing, and simply reprioritize that other bug? > If the user isn't syncing mobile bookmarks, perhaps we put links to download > our apps in the folder? Yeah, but I think we should do that in all 3 places simultaneously.
Agreed. If we change it, I would expect it to appear everywhere. We should look into prioritizing that bug in Q1 or perhaps if it's low hanging fruit, in an upcoming sprint.
Given showing a panel from the menu is tricky, another option is to show a single menu item with text like "Sign in to Sync to blah blah", which just takes you to a signup page.
(In reply to Thom Chiovoloni [:tcsc] from comment #7) > Created attachment 8804791 [details] > screenshot.png > > :rfeeley, do you think this (the Mobile Bookmarks folder) looks OK? Only > shows up for users who have bookmark sync on. Looks like an elegant solution, and I appreciate the initiative!
Hi Just a quick note to say that I really like the look of the screenshot - that is exactly how it was imagined and will help mitigate some of the questions we get asked in SUMO. With regards to the timing and trigger for appearance, I suggest that this be made the same as for the Mobile Bookmarks folder that appears in the Library window. The link from the toolbar is as the folder in the Library, but accessible from a different (easier to find) location.
Comment on attachment 8803979 [details] Bug 1295237 - Add 'Mobile Bookmarks' root to the bookmark menus if we have it. Explicitly re-requesting review even though it's r+, since it's been a while and undergone some discussion. We still use a pref here, but effectively it's only used as a cache for an expensive call -- that is, the pref getting cleared out is expected and doesn't matter.
Comment on attachment 8803979 [details] Bug 1295237 - Add 'Mobile Bookmarks' root to the bookmark menus if we have it. https://reviewboard.mozilla.org/r/88170/#review106504 Looks great, thanks!
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/bd710f6e3fae Add 'Mobile Bookmarks' root to the bookmark menus if we have it. r=markh
:kkumari, I realize you're probably not the right person to ask about this, but since you were doing sync QA earlier, you're the easiest email for me to bring up. We'd like to get some checking that this feature works. What would the correct process be for this?
Created attachment 8835190 [details] Mobile bookmark.PNG Hello Thom,Thank you for reaching out to me. As far as the happy path of this bug fix is concerned, I tested it on latest nightly and found it to be working fine. However, if you are interested in getting more test work done (like regression, etc.) for this bug fix or some new testing effort, and would like to know more about the QA processes then I would like to let you know that our works gets prioritized by our managers I would request you to contact my managers: 1. benhamin (email@example.com) 2. rares (firstname.lastname@example.org) Usually we test features on all related platforms and send a QA sign off on that feature before merging to next channel i.e. from Nightly to Aurora. I would test this bug fix on Latest nightly on Desktop (Mac, Linux and Windows) with Android mobile devices (as iOS doesn't support uploading bookmarks) and will provide test summary/sign off report. Just to clarify, here we have to verify if mobile bookmark folder appear in Bookmark panel? Please refer to attachment.
It should appear in the bookmark panels unless you've never synced before. It should also stay visible after signing out of sync. There are two panels, the menu bar panel and the panel that shows up when you click the list next to the star (sorry, I don't know the real term for either, the code is fairly vague about it) see http://imgur.com/a/gBbvU for screenshots of the two areas (used imgur since attaching multiple things to bugzilla is more tedious and takes multiple steps -- if that's a problem I can reupload to bugzilla). Our goal was mainly to get some limited cross-platform sanity checking, I believe. ni?:adavis, since I'm not sure if that's entirely right or how far we want to go with it (for the record, I think this feature is fairly low risk, and unlikely not to work).
Correct, we just want to make sure that the folder persists disconnects in both locations where it was added. It is certainly also worth verifying if their contents are good too. Though, it is important to note that iOS bookmarks are still not uploading to desktop (so they won't appear here) due to a well known issue that we are all working to resolve.
This bug fix has been tested on the latest nightly(54.0a1) with focus on following areas and no issue was observed: 1. Mobile Bookmark folder is visible under both bookmark panels 2. Mobile Bookmark folder persist through account disconnection and re-connection 3. User Interface 4. Content of Mobile Bookmark folder 5. Cross platform testing between desktop (Mac, Linux, Windows) and Android 6. Bookmark sync verification from desktop to mobile folder and from Android to mobile folder on desktop 7. Regression
Added to Fx54 (Aurora) release notes.