671131 - Add a way to remove history entries from the awesome screen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement, P4)

Description

[CoJaBo]

Long-press on a site you have visited frequently, but will never need again.

Expected: Ability to remove site.
Actual: There is no way to remove it other than clearing all data or waiting a very long time for it to move down off the list.

Comment 1

[Anna (Waverley)]

Seems a good idea to delete from your browsing history only a specific site that you might not need it anymore.

Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:8.0a1) Gecko/20110713 Firefox/8.0a1 Fennec/8.0a1

Device: HTC Desire Z (Android 2.2)

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Madhava - thoughts on this? I think desktop has a way to remove a single page from history too.

Comment 3

[CoJaBo]

"Enhancement" would imply that this is a minor new feature with an easy work-around wouldn't it?

Being able to delete history seems to be a pretty fundamental feature to me, particularly when it is the primary way to navigate to most-visited sites.
There is no work-around- or are we to clear all data every time we stop using a particular site?

Comment 4

[Aaron Train [:aaronmt]]

(In reply to comment #3)
> "Enhancement" would imply that this is a minor new feature with an easy
> work-around wouldn't it?

Enhancement status does not exhibit its weight in value, that's solely up to the component product owners whom establish its priority. As this is a request for implementation, it's more or less an enhancement.

> Being able to delete history seems to be a pretty fundamental feature to me

As do I. Let's see what those who are on the CC list have to add.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Desktop Firefox has "Forget About This Site" (from the History window). "Forget" will remove the site (the domain - "mozilla.org" for example), not the page, from history - but also removes other bits of data:
* Cookies
* Images (in cache)
* Passwords
* Permission
* Downloads
* IndexedDB data
and more

See http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#571

Comment 6

[CoJaBo]

This particular request is more analogous to hitting the delete key in the awesome bar- it removes the currently selected history entry from the list. This is indispensable for removing "junk" entries that are ranked highly but no longer needed. This is trivial in the desktop version, but in Fennec there is currently no means to accomplish this- sites that make the list are essentially semi-permanent.
Having a "forget site" option would be useful too as a standard privacy feature (could probably go in the favicon menu), but isn't directly related to this request.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Not entirely sure why this was moved to the SDK.

Comment 8

[CoJaBo]

Sorry, not sure what happened there- I'm moving my bugs that also affect Native to that product, as the XUL version is no longer being developed.

Comment 10

[:Margaret Leibovic]

I can take this.

Comment 12

[:Margaret Leibovic]

I mentioned this in bug 743464. Now that history/bookmarks are combined in the top sites section of the awesome screen, we'd have to decide exactly what removing an entry from there would do - should it remove both the history entry _and_ the bookmark?

We'll also need a string for this if we're going to make a context menu item for it.

Comment 13

[CoJaBo]

The analogous action on Desktop, hitting delete on a history entry that is also bookmarked, just causes it to reappear lower in the list after it is closed and reopened which in some cases will be exactly where it was before deletion- not sure if this is optimal..

"Remove from Top Sites" or "Remove from History" (or similar wording) could be displayed for removing those history entries, like on desktop.
On sites that are also bookmarked, there could be an additional "Remove from Bookmarks" option (the wording on the Bookmarks tab, then, ought to match this); alternatively, the "Remove from Bookmarks" option could just be omitted, as the user can just go to the bookmarks tab to delete it.
Either way, I don't think deleting something from history should delete it from bookmarks (or vice-versa, as this doesn't happen on Desktop), so it should be clear from wording which is being deleted, the history entry or the bookmark.

I would also suggest some visual to indicate bookmarked history entries (like there is a star on desktop- may need another bug for this, is there one?).

Comment 14

[CoJaBo]

(In reply to CoJaBo from comment #13)
> I would also suggest some visual to indicate bookmarked history entries
> (like there is a star on desktop- may need another bug for this, is there
> one?).
Just noticed that is Bug 701330. Also, the title of this bug should probably be changed, as the "All Pages" screen is no longer called that (I'll let someone else pick the optimum title tho).

Comment 15

[:Margaret Leibovic]

(In reply to CoJaBo from comment #14)

> Also, the title of this bug should probably
> be changed, as the "All Pages" screen is no longer called that (I'll let
> someone else pick the optimum title tho).

Taken care of.

Comment 18

[Demetrius S.]

As a user I would like to be able to remove a top site from the start page but keep the top sites on the awesome bar intact.

Comment 19

[Paul [pwd]]

Any chance of upping the priority on this? For my trouble of being a nightly tester, I have more about:home's than I know what to do with.

Comment 20

[:Margaret Leibovic]

(In reply to Paul [sabret00the] from comment #19)
> Any chance of upping the priority on this? For my trouble of being a nightly
> tester, I have more about:home's than I know what to do with.

The duplicate history entry problem should have been fixed by bug 741590. It sounds like you're experiencing bug 742381. Unfortunately, if your profile got messed up from using an early version of sync, you'll just need to start fresh with a new profile. Alternately, you can delete bookmarks from the bookmarks tab of the awesomescreen.

Comment 21

[Paul [pwd]]

I don't get that. I only had a single version of the about:home's and about:firefox's for pretty much the entire time of testing Firefox native. Then I see duplicates recently, file a bug and that's found to be a dupe, but end of the trail appears to be this whole "early version of sync" assertion. Surely if that was the case, I should've seen the duplicate top sites earlier?

Comment 22

[Paul [pwd]]

And just to add further info. I don't have multiple versions of these sites in the AwesomeBar/Screen on either desktop nor XUL further compounding my confusion.

Do I simply uninstall-reinstall to create a new profile or will do I need to do anything special?

Comment 23

[:Margaret Leibovic]

(In reply to Paul [sabret00the] from comment #21)
> I don't get that. I only had a single version of the about:home's and
> about:firefox's for pretty much the entire time of testing Firefox native.
> Then I see duplicates recently, file a bug and that's found to be a dupe,
> but end of the trail appears to be this whole "early version of sync"
> assertion. Surely if that was the case, I should've seen the duplicate top
> sites earlier?

I'm sorry I somehow missed this comment. Have you been using sync all along, or did you just start using sync? There was a similar report in another bug from a user who had used Fennec Native for a long time, but just recently set up sync. Do you remember when you started using sync, and when you noticed this problem?

(In reply to Paul [sabret00the] from comment #22)
> And just to add further info. I don't have multiple versions of these sites
> in the AwesomeBar/Screen on either desktop nor XUL further compounding my
> confusion.
> 
> Do I simply uninstall-reinstall to create a new profile or will do I need to
> do anything special?

Uninstall-reinstall should do the trick. Are you still seeing problems?

Comment 24

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #12)
> I mentioned this in bug 743464. Now that history/bookmarks are combined in
> the top sites section of the awesome screen, we'd have to decide exactly
> what removing an entry from there would do - should it remove both the
> history entry _and_ the bookmark?
> 
> We'll also need a string for this if we're going to make a context menuitem
> for it.

I've been thinking about how to go forward with this bug, and I think one solution would be to have both a "Remove from history" context menuitem and a "Remove bookmark" context menuitem, and just show the relevant option(s) depending on the entry. So, in the bookmarks tab, where we currently have the "Remove" context menuitem, we would update that string to say "Remove bookmark", and we'll also show that menuitem for bookmarks in the top sites tab. Inversely, in the history tab, we could have a new menuitem that says "Remove from history", and then we could show that for history items in the top sites tab.

Ian, what do you think? I'm afraid that just having a "Remove" context menuitem in top sites would be too mysterious (what would that do? what would the user expect it to do?), so having an explicit "Remove from history" would avoid confusion.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

I am opposed to "Remove from history" when in History and "Remove bookmark" when in Bookmarks. We already have "Edit" in Bookmarks and this would lead to "Edit bookmark" and I dislike the redundancy.

For the Top Sites tab, I suggest we have a "Remove this Page" menu.


Note that desktop also has a "Forget this Site" command that does more than remove the page from history. It also clears cookies and other data for the website.

Comment 26

[:Margaret Leibovic]

Attachment: (1/2) Context menu item to remove history (WIP)

We still need to sort out some UX/strings, but for now I decided to just add a different "Remove" menuitem that removes the history entry and follows up with a "Page removed from history" toast (the "Remove" that removes bookmarks follows up with a "Bookmark removed" toast).

Comment 27

[:Margaret Leibovic]

Attachment: (2/2) ContentObserver for history tab

I modeled this after what Brian did in bug 722413. Because the history tab data isn't backed by a cursor, it won't automatically update when a history item is deleted, so we need to do it ourselves.

I decided to rename unregisterBookmarkObserver to unregisterContentObserver, since it does not discriminate about the type of observer, and that way I could just re-use it to unregister the history observer.

Comment 28

[Paul [pwd]]

(In reply to Margaret Leibovic [:margaret] from comment #23)
> I'm sorry I somehow missed this comment. Have you been using sync all along,
> or did you just start using sync? There was a similar report in another bug
> from a user who had used Fennec Native for a long time, but just recently
> set up sync. Do you remember when you started using sync, and when you
> noticed this problem?
I set up sync pretty much as soon as it landed on m-c. I started seeing this problem around a month ago. It hasn't resurfaced since I uninstalled/reinstalled.

> Uninstall-reinstall should do the trick. Are you still seeing problems?
Thank you very much.

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 621207 [details] [diff] [review]
(1/2) Context menu item to remove history (WIP)

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

So, if I visited the removed url, it will show up in the top sites search again. Is that the intended UX? In the case of removing an entry from the top sites search, I wonder if what you want is something more like "down-scoring" it in the results  somehow? Patch looks generally good, giving my f+.

::: mobile/android/base/AwesomeBar.java
@@ +513,5 @@
>                  int keywordCol = cursor.getColumnIndex(URLColumns.KEYWORD);
>                  if (keywordCol != -1)
>                      keyword = cursor.getString(keywordCol);
>  
> +                // Use the bookmark id for the Bookmarks tab and the history id for the Top Sites tab 

Top Sites tab might list unvisited bookmarks, no? i.e. history id would be null or something. Don't you have to handle this case somehow?

@@ +634,5 @@
>                  }).execute();
>                  break;
>              }
> +            case R.id.remove_history: {
> +                (new GeckoAsyncTask<Void, Void, Void>() {

nit: this doesn't look very readable. I'd prefer a separate class. No big deal though.

::: mobile/android/base/AwesomeBarTabs.java
@@ +495,5 @@
>  
>              String url = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL));
>              String title = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.TITLE));
>              byte[] favicon = cursor.getBlob(cursor.getColumnIndexOrThrow(URLColumns.FAVICON));
>              Long bookmarkId = cursor.getLong(cursor.getColumnIndexOrThrow(Combined.BOOKMARK_ID));

Change bookmarkId to be an integer too?

::: mobile/android/base/db/LocalBrowserDB.java
@@ +289,5 @@
>          return new LocalDBCursor(c);
>      }
>  
> +    public void removeHistoryEntry(ContentResolver cr, int id) {
> +        cr.delete(mHistoryUriWithProfile,

Alternatively, you could use the uri with the id here (instead of the History._ID selection). Not a big deal though.

Comment 30

[Ian Barlow (:ibarlow)]

> So, if I visited the removed url, it will show up in the top sites search
> again. Is that the intended UX? In the case of removing an entry from the
> top sites search, I wonder if what you want is something more like
> "down-scoring" it in the results  somehow? 

I think for now we should continue to treat it as though it is just like any other site that hasn't been visited, and if someone visits the site again it should build up its awesomebar ranking in the same as any other site does. Down-scoring starts to feel a little too "clever" IMO.

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Ian Barlow (:ibarlow) from comment #30)
> > So, if I visited the removed url, it will show up in the top sites search
> > again. Is that the intended UX? In the case of removing an entry from the
> > top sites search, I wonder if what you want is something more like
> > "down-scoring" it in the results  somehow? 
> 
> I think for now we should continue to treat it as though it is just like any
> other site that hasn't been visited, and if someone visits the site again it
> should build up its awesomebar ranking in the same as any other site does.
> Down-scoring starts to feel a little too "clever" IMO.

Yeah. Agreed.

Comment 32

[:Margaret Leibovic]

Attachment: (1/2) Context menu item to remove history

I was going to flag Lucas for review, but he just left for a vacation...

Good catch on the null history id. In the combined view, we actually have a history id = -1 for all non-bookmark history items, so I added a check to hide the "Remove" item in cases where the id is < 0.

This has the added benefit of avoiding the confusion of trying to remove an unvisited bookmark in "Top Sites" - you won't be able to do it since we'll be hiding the "Remove" item in that case.

Ian and I discussed the UX of having "Remove" just remove the history entry in "Top Sites", and this is the direction we want to go right now. I also discussed strings we him, and we decided to go with "Page Removed" for the toast notification.

As for the comment about the readability of that GeckoAsyncTask, I was just copying what was done in the remove_bookmark handler. This context menu code feels pretty gross to me in general, especially the way we need to special case things depending on the awesomebar tab. I'd like to see if there's a way to fix all this up, but I think it should go in a different bug.

Comment 33

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 623307 [details] [diff] [review]
(1/2) Context menu item to remove history

Looks good

Comment 34

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95dfc63ba181
https://hg.mozilla.org/integration/mozilla-inbound/rev/a02adac1eaf3

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/95dfc63ba181
https://hg.mozilla.org/mozilla-central/rev/a02adac1eaf3

Comment 36

[Aaron Train [:aaronmt]]

SV, please add a basic smoke-test for this added functionality

Comment 37

[Alec]

Sorry to butt into your discussion,  but a strong vote here for an addition to the history list item long-press popup to have a  "remove from history" option, the same as the bookmark list item popup currently has.
A bug I'd been following which has been duped here had a comment saying remove from history should also remove the bookmark, if existing.  Again, a strong vote that if the function is accessed from the history list it should only affect the history.

Comment 38

[:Margaret Leibovic]

(In reply to Alec from comment #37)
> Sorry to butt into your discussion,  but a strong vote here for an addition
> to the history list item long-press popup to have a  "remove from history"
> option, the same as the bookmark list item popup currently has.
> A bug I'd been following which has been duped here had a comment saying
> remove from history should also remove the bookmark, if existing.  Again, a
> strong vote that if the function is accessed from the history list it should
> only affect the history.

That is what the patch in this bug does. You will see this in Firefox 15.

Comment 39

[Demetrius S.]

Has this functionality been removed in the 16? It doesn't seem possible to remove top sites anymore.

Comment 40

[Wesley Johnston (:wesj)]

Bug 763036.

Comment 41

[Adrian Tamas (:AdrianT)]

Testcase in MozTrap
https://moztrap.mozilla.org/manage/cases/_detail/6288/

The test case was added both in the Smoketest suite and in the BFT suite

Comment 42

[Adrian Tamas (:AdrianT)]

Items can be removed from context menu on Nightly 18.0a1 2012-09-02, Aurora 17.0a2 2012-09-02 and Firefox Mobile 16.0b1 using the Samsung Galaxy Tab 2 7.0 (Android 4.0.4) and Samsung Galaxy R (Android 2.3.4).

Comment 43

[alex_mayorga]

Attachment: Missing option to remove a "top site"

Am I missing something here or did the option to remove a "top site" get lost at some point?

The option just not exists on Nightly anymore so I ended up on this bug when trying to report the problem =(

Comment 44

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to alex_mayorga from comment #43)

> Am I missing something here or did the option to remove a "top site" get
> lost at some point?

Could have gotten lost during the about:home rewrite. In any case, it's a new bug. This one is dead and gone. File a new bug for the current issue.

Comment 45

[Mark Finkle (:mfinkle) (use needinfo?)]

Just setting this bug back to it's old state

Comment 46

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #44)
> (In reply to alex_mayorga from comment #43)
> 
> > Am I missing something here or did the option to remove a "top site" get
> > lost at some point?
> 
> Could have gotten lost during the about:home rewrite. In any case, it's a
> new bug. This one is dead and gone. File a new bug for the current issue.

We got rid of the remove context menu item on about:home thumbnails in bug 837132.

However, this comment in that bug is now wrong, so we could file a new bug about it:
https://bugzilla.mozilla.org/show_bug.cgi?id=837132#c15

Comment 47

[alex_mayorga]

(In reply to :Margaret Leibovic from comment #46)
> (In reply to Mark Finkle (:mfinkle) from comment #44)
> > (In reply to alex_mayorga from comment #43)
> > 
> > > Am I missing something here or did the option to remove a "top site" get
> > > lost at some point?
> > 
> > Could have gotten lost during the about:home rewrite. In any case, it's a
> > new bug. This one is dead and gone. File a new bug for the current issue.
> 
> We got rid of the remove context menu item on about:home thumbnails in bug
> 837132.
> 
> However, this comment in that bug is now wrong, so we could file a new bug
> about it:
> https://bugzilla.mozilla.org/show_bug.cgi?id=837132#c15

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=927249 FWIW.