Closed Bug 1401179 Opened 2 years ago Closed 2 years ago

[Activity Stream] - No notifications are displayed when adding a bookmark

Categories

(Firefox for Android :: Theme and Visual Design, defect, P1)

57 Branch
ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 58
Iteration:
1.30
Tracking Status
fennec + ---
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- verified
firefox58 --- verified

People

(Reporter: bsurd, Assigned: liuche)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MobileAS])

Attachments

(1 file)

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
Assignee: nobody → liuche
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.
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?)
Blocks: 1402521
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+
Pushed by cliu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6a4660a684e
Add snackbar for bookmark add/remove actions on newtab. r=mcomella
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?
https://hg.mozilla.org/mozilla-central/rev/d6a4660a684e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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+
Notifications are not properly displayed, marking as verified for Nightly.
Verified in 57.0b3.
Status: RESOLVED → VERIFIED
Depends on: 1410221
You need to log in before you can comment on or make changes to this bug.