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

Status: VERIFIED FIXED

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

Description

[Bogdan Surd, QA [:bsurd, NI]]

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).

Comment 1

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Not too hard but, depending on investigation (> 1hr), we should put it down to P2.

Comment 2

[Chenxia Liu [:liuche]]

Attachment: Bug 1401179 - Add snackbar for bookmark add/remove actions on newtab.

Review commit: https://reviewboard.mozilla.org/r/182440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182440/

Comment 3

[Michael Comella (:mcomella) [needinfo or I won't see it]]

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.

Comment 4

[Michael Comella (:mcomella) [needinfo or I won't see it]]

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.

Comment 5

[Chenxia Liu [:liuche]]

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 6

[Chenxia Liu [:liuche]]

Comment on attachment 8910970 [details]
Bug 1401179 - Add snackbar for bookmark add/remove actions on newtab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182440/diff/1-2/

Comment 7

[Michael Comella (:mcomella) [needinfo or I won't see it]]

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.

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 8910970 [details]
Bug 1401179 - Add snackbar for bookmark add/remove actions on newtab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182440/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6a4660a684e
Add snackbar for bookmark add/remove actions on newtab. r=mcomella

Comment 10

[Chenxia Liu [:liuche]]

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

Comment 11

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d6a4660a684e

Comment 12

[Sylvestre Ledru [:Sylvestre] back june 18th]

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

Comment 13

[Bogdan Surd, QA [:bsurd, NI]]

Notifications are not properly displayed, marking as verified for Nightly.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fd78928b00d8

Comment 16

[Bogdan Surd, QA [:bsurd, NI]]

Verified in 57.0b3.