Closed
Bug 1401179
Opened 5 years ago
Closed 5 years ago
[Activity Stream] - No notifications are displayed when adding a bookmark
Categories
(Firefox for Android Graveyard :: Theme and Visual Design, defect, P1)
Tracking
(fennec+, firefox55 unaffected, firefox56 unaffected, firefox57 verified, firefox58 verified)
VERIFIED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
fennec | + | --- |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | --- | verified |
firefox58 | --- | verified |
People
(Reporter: bsurd, Assigned: liuche)
References
Details
(Whiteboard: [MobileAS])
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
mcomella
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
Steps to reproduce: 1. Open Fennec; 2. Log tap on any item from the top sites tile 3. Add any item as a bookmark. Expected result: A notification is displayed at the bottom of the screen displaying "Bookmark added" in white text and a blue "OPTIONS" button on the right. Actual result: No notification is displayed. Notes: The same issue happens when removing a bookmark or removing any other item (history items).
Not too hard but, depending on investigation (> 1hr), we should put it down to P2.
tracking-fennec: ? → +
Priority: -- → P1
Iteration: --- → 1.30
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → liuche
Comment hidden (mozreview-request) |
Comment 3•5 years ago
|
||
mozreview-review |
Comment on attachment 8910970 [details] Bug 1401179 - Add snackbar for bookmark add/remove actions on newtab. https://reviewboard.mozilla.org/r/182440/#review187754 ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/menu/ActivityStreamContextMenu.java:242 (Diff revision 1) > db.addBookmark(context.getContentResolver(), item.getTitle(), item.getUrl()); > + bookmarkAction = Tabs.TabEvents.BOOKMARK_ADDED; > } > + > + final Tabs tabsInstance = Tabs.getInstance(); > + tabsInstance.notifyListeners(tabsInstance.getSelectedTab(), bookmarkAction); We're not bookmarking the selected tab here, we're bookmarking the top sites item the user has selected, so I don't think this is entirely correct. As we investigated irl, it looks like this makes the edit dialog use the wrong page when bookmarking.
Attachment #8910970 -
Flags: review?(michael.l.comella) → review-
To be explicit, it's important that we have this Snackbar so the user can edit the bookmark via the options button. Notes: - The old top sites doesn't have a "bookmark" or "remove bookmark" option - As Bogdan alludes to, no other items in top sites display a snackbar when pressed (In reply to Michael Comella (:mcomella) from comment #1) > Not too hard but, depending on investigation (> 1hr), we should put it down > to P2. Unfortunately, it looks like we'll have to extract: - Showing a bookmarks snackbar from the BookmarkStateChangeDelegate which requires... - Making SnackbarBuilder not reliant on having an Activity (it looks like it could be substituted for a View which is currently in the view hierarchy) Which, while perhaps possible to do in an hour, is likely to be too complex to uplift.
Assignee | ||
Comment 5•5 years ago
|
||
Okay, thanks for the research - I think that the more important part right now is to be able to see feedback that you've bookmarked a page (because it's one of two items that gives no visual feedback, the other being "copy address", but we don't provide visual feedback there anywhere). I'd propose that we just show a simple snackbark that says "Bookmark added" and "Bookmark removed" without the option to edit the site. If people want to edit it, they can go to bookmarks and do it, or maybe they don't even care. (I even wonder why we added that in the first place - do enough people edit their bookmarks immediately upon bookmarking that they needed a shortcut?)
Comment hidden (mozreview-request) |
Comment 7•5 years ago
|
||
mozreview-review |
Comment on attachment 8910970 [details] Bug 1401179 - Add snackbar for bookmark add/remove actions on newtab. https://reviewboard.mozilla.org/r/182440/#review188122 ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/menu/ActivityStreamContextMenu.java:237 (Diff revision 2) > final BrowserDB db = BrowserDB.from(context); > > if (item.isBookmarked()) { > db.removeBookmarksWithURL(context.getContentResolver(), item.getUrl()); > + > + // XXX: Bug 1402521 for adding "options" to the snackbar. nit: I think XXX will get highlighted by our editors and I don't think this comment needs to be especially highlighted.
Attachment #8910970 -
Flags: review?(michael.l.comella) → review+
Comment hidden (mozreview-request) |
Pushed by cliu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d6a4660a684e Add snackbar for bookmark add/remove actions on newtab. r=mcomella
Assignee | ||
Comment 10•5 years ago
|
||
Comment on attachment 8910970 [details] Bug 1401179 - Add snackbar for bookmark add/remove actions on newtab. Approval Request Comment [Feature/Bug causing the regression]: Missing feature highlighted in QA review as P1, parity with all other bookmarking actions across Fennec [User impact if declined]: No feedback that a bookmark was added or removed from newtab page [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: verified locally [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: Using standard Android call to show a notification [String changes made/needed]: none
Attachment #8910970 -
Flags: approval-mozilla-beta?
![]() |
||
Comment 11•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d6a4660a684e
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Updated•5 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
Comment 12•5 years ago
|
||
Comment on attachment 8910970 [details] Bug 1401179 - Add snackbar for bookmark add/remove actions on newtab. polish a new feature and making it consistent with the rest of the UX, taking it Should be in 57b3
Attachment #8910970 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reporter | ||
Comment 13•5 years ago
|
||
Notifications are not properly displayed, marking as verified for Nightly.
Comment 14•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/fd78928b00d8
Duplicate of this bug: 1340536
Updated•2 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•