Closed Bug 1029891 Opened 7 years ago Closed 7 years ago
Collections and bookmarks is very slow
When tapping on the X icon to delete smartCollections and bookmarks, the deletion dialog often takes a few seconds to be displayed. X to delete for normal apps is fast.
QA Wanted for a video.
It is not ideal but it is normal due to the implementation. To delete apps the confirm message is displayed in the homescreen app but smartCollections and bookmarks are controlled by other apps via a MozActivity. Nothing to do here from homescreen point of view
I think its problematic for the UI to be unresponsive for several seconds, so we need to find some UX trick to provide feedback to the user while that loads.
Severity: normal → blocker
Priority: -- → P1
Whiteboard: [c=progress p=3 s= u=]
blocking-b2g: 2.0? → 2.0+
Whiteboard: [c=progress p=3 s= u=] → [c=progress p=3 s= u=2.0]
According to our goals for meeting responsiveness , we want to show this interaction within 140ms. If we cannot, we need to display some kind of indicator that there is some action in progress within 1000ms, e.g. with a spinner, etc.  https://developer.mozilla.org/en-US/Apps/Build/Performance/Firefox_OS_app_responsiveness_guidelines#User_stories
IMO this can be very simple... we just delete the icon after the confirm (without really deleting it) and delete the thing asynchronously.
The problem is that currently we surface the confirm() inside of the collection or bookmark app, and surface that using a MozActivity. Should we move this confirm into the homescreen app instead?
Gregor, do you have anyone that can take ownership of this issue? (FxOS Performance Triage)
(In reply to Eli Perelman, :Eli from comment #7) > Gregor, do you have anyone that can take ownership of this issue? (FxOS > Performance Triage) Gregor is on PTO, so I'm redirecting this to Candice.
Flags: needinfo?(anygregor) → needinfo?(cserran)
A reasonable approach to me seems to: - Move the dialogs to the homescreen (or gaia_grid that is another issue) - Use datastore to remove the collections, bookmarks asynchronously - Live with the very-very-very edge case of unsuccessful downloads
I can provide a patch today to fix this.
QA Contact: jlal
For best performance we need to move strings around... there is no way to workaround this and get the performance we need (with the same strings).... The ids will move between apps but the strings themselves should remain the same.
Hrm- on second thought we can do this without l10n changes if we simply reuse the same string for apps/collections which should not be the end of the world for near-instant downloads. <name> and all of its data will be deleted.
Needs tests for bookmark/collection removal (we already have some for app installs).
Attachment #8446240 - Flags: feedback?(kgrandon)
Comment on attachment 8446240 [details] [review] https://github.com/mozilla-b2g/gaia/pull/21000 Makes sense to me, thanks.
Attachment #8446240 - Flags: feedback?(kgrandon) → feedback+
Assignee: nobody → jlal
Status: NEW → ASSIGNED
QA Contact: jlal
Comment on attachment 8446240 [details] [review] https://github.com/mozilla-b2g/gaia/pull/21000 Ready for review... I fixed the test failures (in one case removing the unit test in favor of the better integration test) and adding tests for bookmark removal / collection removal.
Attachment #8446240 - Flags: feedback+ → review?(kgrandon)
Please be careful because the message is different when you are removing an app or deleting a bookmark and you are doing the same thing in your pull request. See http://i.imgur.com/itHxCCe.png Bookmarks: Remove <name> Remove <name> form homescreen? Cancel | Remove With your implementation here you could close this bug 1029477 because I think that you are fixing it too seeing your code in github
I chatted with UX today about this (the bookmark case at least) changing the strings is less then ideal at this point but we can try something. @jsavory Are you okay with the above screenshot ? (Thanks Cristian)
Comment on attachment 8446240 [details] [review] https://github.com/mozilla-b2g/gaia/pull/21000 Hey, you basically have an R+ here, but I do think there is a legit failing test for pinning items to smart collections. I haven't looked into why yet, but can help dig in tomorrow. Nice job.
Attachment #8446240 - Flags: review?(kgrandon) → review+
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?] → [VH-FL-blocking-][VH-FC-blocking+]
Whiteboard: [c=progress p=3 s= u=2.0] → [c=progress p=3 s= u=2.0][systemsfe]
Ugh - should not attempt to read images at night ... the image above is the current images not what this patch does.
:D I will fix this right now since I have an hour... So happy we have those tests ++
Should be fixed now... There was a typo in collection additions and a missing name change in the collections app.
Assignee: jlal → kgrandon
Target Milestone: --- → 2.0 S5 (4july)
Landed: https://github.com/mozilla-b2g/gaia/commit/a62db8f1a3c52546d30fbd08b5e539676c5fc6c0 There is some UX discussion happening in bug 1029477, but because it appears there are no l10n changes here I think this is a win and we should take it for 2.0. I will also close bug 1029477, but UX can follow-up here or there. Thanks!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
This issue has been successfully verified on Flame 2.0: Gaia-Rev 8d1e868864c8a8f1e037685f0656d1da70d08c06 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3 Build-ID 20141202000201 Version 32.0 Device-Name flame FW-Release 4.4.2 This issue has been successfully verified on Flame 2.1: Gaia-Rev ccb49abe412c978a4045f0c75abff534372716c4 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22 Build-ID 20141202001201 Version 34.0 Device-Name flame FW-Release 4.4.2
You need to log in before you can comment on or make changes to this bug.