921136 - Unify removal behavior for bookmark-history-readinglist items from Home items

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Aaron Train [:aaronmt]]

Currently one can long tap and select 'Remove' and you get a toast saying 'Page Removed', but the result is still there as it's a bookmarked.

STR:

i) Visit http://nytimes.com
ii) Bookmark the page
iii) Tap the address bar and search up 'nytimes'
iv) Long tap on the switch to tab result and hit 'Remove'

Expected: Removes the bookmark
Actual: Removes the page history; the site remains visible because it's bookmarked so it looks like tapping 'Remove' does nothing

Not sure if we want to be more clear what exactly were removing by specifying in the context menu label (i.e, Remove Bookmark or Remove from History)

Comment 1

[Aaron Train [:aaronmt]]

Doesn't need to be a page currently open.

Comment 2

[:Margaret Leibovic]

To anyone who's curious, this isn't a regression, it's what we do on the awesomescreen in release/beta.

Comment 3

[:Margaret Leibovic]

According to ibarlow's mockup in bug 931021, we should make this remove item remove both the history entry and the bookmark.

I'm tempted to dupe this to bug 913457, but let's keep them separate for now. That bug can be about thumbnail context menu items, and this one can be about list items.

Ian, question: What should the toast say in the different remove cases? What about this:

1) Item is only a history entry -> "Page removed"
2) Item is only a bookmark -> "Bookmark removed"
3) Item is both a history entry *and* a bookmark -> "Bookmark removed"

I feel like the bookmark being removed is more important info than the history entry being removed. Do you think users would be surprised if we removed a history item when removing a bookmark? For simplicity's sake, I like having one "Remove" item that removes everything about the site, regardless of what about:home page you're on.

Or maybe the toast should always just say "Page removed"?

Comment 4

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #3)
> According to ibarlow's mockup in bug 931021, we should make this remove item
> remove both the history entry and the bookmark.
> 
> I'm tempted to dupe this to bug 913457, but let's keep them separate for
> now. That bug can be about thumbnail context menu items, and this one can be
> about list items.
> 
> Ian, question: What should the toast say in the different remove cases? What
> about this:
> 
> 1) Item is only a history entry -> "Page removed"
> 2) Item is only a bookmark -> "Bookmark removed"
> 3) Item is both a history entry *and* a bookmark -> "Bookmark removed"
> 
> I feel like the bookmark being removed is more important info than the
> history entry being removed. Do you think users would be surprised if we
> removed a history item when removing a bookmark? For simplicity's sake, I
> like having one "Remove" item that removes everything about the site,
> regardless of what about:home page you're on.
> 
> Or maybe the toast should always just say "Page removed"?

I prefer to keep it simple with "Page removed" everywhere, all the time.

Comment 5

[:Margaret Leibovic]

(In reply to Ian Barlow (:ibarlow) from comment #4)
> (In reply to :Margaret Leibovic from comment #3)
> > According to ibarlow's mockup in bug 931021, we should make this remove item
> > remove both the history entry and the bookmark.
> > 
> > I'm tempted to dupe this to bug 913457, but let's keep them separate for
> > now. That bug can be about thumbnail context menu items, and this one can be
> > about list items.
> > 
> > Ian, question: What should the toast say in the different remove cases? What
> > about this:
> > 
> > 1) Item is only a history entry -> "Page removed"
> > 2) Item is only a bookmark -> "Bookmark removed"
> > 3) Item is both a history entry *and* a bookmark -> "Bookmark removed"
> > 
> > I feel like the bookmark being removed is more important info than the
> > history entry being removed. Do you think users would be surprised if we
> > removed a history item when removing a bookmark? For simplicity's sake, I
> > like having one "Remove" item that removes everything about the site,
> > regardless of what about:home page you're on.
> > 
> > Or maybe the toast should always just say "Page removed"?
> 
> I prefer to keep it simple with "Page removed" everywhere, all the time.

Works for me! Makes writing the patch easier :)

Comment 6

[:Margaret Leibovic]

Attachment: Make the "Remove" context menu remove pages from everywhere

I kept the reading_list_removed and bookmark_removed strings, because they're used in other toast messages from the reader mode toolbar and the app menu star.

I decided to change the entity name to make the code clearer, but also to give localizers a chance to update the string, since the meaning has changed.

Comment 7

[:Margaret Leibovic]

I realized this fix isn't quite totally correct... on the bookmarks page, we only query the bookmarks table, not the combined view, so we won't remove history entries on that page.

Comment 8

[:Margaret Leibovic]

I haven't had time to work on this myself, so making into a mentor bug.

The patch in here is likely bitrotted by now, but it also needs work to make it correct.

Comment 10

[Chenxia Liu [:liuche]]

This should be implemented per comment #3.

Comment 11

[Chenxia Liu [:liuche]]

Ian, I took a look at the mocks, and I think that removing a history/bookmark item => removing the item from the reading list seems a little unexpected. What do you think?

Comment 12

[Ian Barlow (:ibarlow)]

Mm, I think I disagree. It strikes me that if I have a thing that I saved, and I hit "remove", I really mean "remove" it from Firefox. 

The less-than-ideal alternative is leaving users to hunt and peck around the app, wondering if they have actually removed the page from everywhere...

Comment 13

[Chenxia Liu [:liuche]]

Attachment: Patch: Unify removing bookmark/history/reading list

Unfortunately, this will still double notify if removing a bookmark/history item that also happens to be a reading list item because Reading List removal waits for a Gecko message (which gets handled in BrowserApp).

Comment 14

[:Margaret Leibovic]

Comment on attachment 8422832 [details] [diff] [review]
Patch: Unify removing bookmark/history/reading list

Review of attachment 8422832 [details] [diff] [review]:
-----------------------------------------------------------------

The reading list message dance looks like it has some problems, but let's file a follow-up bug to address that.

::: mobile/android/base/home/HomeFragment.java
@@ +206,5 @@
>              if (info.hasBookmarkId()) {
> +                new RemoveBookmarkTask(context, info.bookmarkId, !notifyQueued).execute();
> +                notifyQueued = true;
> +            } else {
> +                new RemoveBookmarkTask(context, url, false).execute();

This could be confusing, because it will remove any bookmark with that URL (e.g. ones in different folders), but that's what the bookmark star does now, so it's not unprecedented.

It's hard to have a perfect solution here, but at least we can be consistent if we just go with the "I want this thing gone from Firefox" approach.

@@ +345,3 @@
>  
>              GeckoEvent e = GeckoEvent.createBroadcastEvent("Reader:Remove", mUrl);
>              GeckoAppShell.sendEventToGecko(e);

I think we should just move the notification to here. The only reason we need to send a message to gecko is to remove the article from the cache. In fact, the current logic is kinda wrong, because even if we fail to remove the article from the cache, we're still removing the item from the list, so as far as the user is concerned, it's removed. Actually, looking at the current logic, we call BrowserDB.removeReadinglistItem twice in this case!

I want us to clean up this logic, but let's do it in a follow-up bug, since it looks like there could be a few things at play here.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +280,5 @@
>  <!ENTITY pref_scroll_title_bar "Scroll title bar">
>  
> +<!-- Localization note (page_removed): This string appears in a toast message when
> +     any page is removed frome about:home. This includes pages that are in history,
> +     bookmarks, or reading list. -->

Nice l10n note.

Comment 15

[Chenxia Liu [:liuche]]

> ::: mobile/android/base/home/HomeFragment.java
> @@ +206,5 @@
> >              if (info.hasBookmarkId()) {
> > +                new RemoveBookmarkTask(context, info.bookmarkId, !notifyQueued).execute();
> > +                notifyQueued = true;
> > +            } else {
> > +                new RemoveBookmarkTask(context, url, false).execute();
> 
> This could be confusing, because it will remove any bookmark with that URL
> (e.g. ones in different folders), but that's what the bookmark star does
> now, so it's not unprecedented.
> 
> It's hard to have a perfect solution here, but at least we can be consistent
> if we just go with the "I want this thing gone from Firefox" approach.

Right, I was following ibarlow's comment #12 on removing everything everywhere so a user doesn't need to peck through all their home panels to make sure something is actually gone. I suppose the "bookmarks in different folders" could be a problem though, but we can talk about that later, I suppose.
> 
> @@ +345,3 @@
> >  
> >              GeckoEvent e = GeckoEvent.createBroadcastEvent("Reader:Remove", mUrl);
> >              GeckoAppShell.sendEventToGecko(e);
> 
> I think we should just move the notification to here. The only reason we
> need to send a message to gecko is to remove the article from the cache. In
> fact, the current logic is kinda wrong, because even if we fail to remove
> the article from the cache, we're still removing the item from the list, so
> as far as the user is concerned, it's removed. Actually, looking at the
> current logic, we call BrowserDB.removeReadinglistItem twice in this case!
> 
> I want us to clean up this logic, but let's do it in a follow-up bug, since
> it looks like there could be a few things at play here.
> 
Filed: bug 1011714.

Comment 16

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/53ba60264695

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/53ba60264695