1278840 - FF for Android offers no way to remove arbitrary sites from history apart from top sites or recent history

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P3)

Description

[nemo]

STR:
①  Type a few letters into urlbar on a profile with a history that goes back a significant amount of time
②  Notice entries in the awesomescreen that you don't want to have.
③  Try longtouch on them.  No remove option.

Using the ⁝ menu and choosing "History" sends to the "History" tab of about:home  - unfortunately this tab has no search bar, and is only recent history, so there's no way to search for and remove old items.

Comment 1

[:Margaret Leibovic]

This seems bad. Since we power the awesomescreen results with the top sites query, I think we should show the same context menu that we show for list items in the top sites panel.

Anthony, what do you think?

Daniel, is this something you would be interested in picking up?

Comment 2

[nemo]

Mildly related, after hunting around for anything in the way of history editors for Android, ran into:
https://addons.mozilla.org/en-US/firefox/addon/zero-footprint/
https://addons.mozilla.org/en-US/firefox/addon/dynamichistory/

From the search link in about:addons - neither of them worked due to broken config on Android.  Margaret said that was probably related to bug #1079466, although having broken addons show up in search at all is not great.  Asking around to see if they can be automatically blacklisted based on their config requirements for now.

Comment 3

[Daniel Lim [:dlim]]

(In reply to :Margaret Leibovic from comment #1)
> This seems bad. Since we power the awesomescreen results with the top sites
> query, I think we should show the same context menu that we show for list
> items in the top sites panel.
> 
> Anthony, what do you think?
> 
> Daniel, is this something you would be interested in picking up?

Sure, I can pick this up when I'm done with my current ticket.

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #1)
> This seems bad. Since we power the awesomescreen results with the top sites
> query, I think we should show the same context menu that we show for list
> items in the top sites panel.
> 
> Anthony, what do you think?

I think the long-press context menu here makes sense. 

But I think we should start being more diligent about the options we offer in these context menus. The same list of options with "Open in New Tab", "Open in Private Tab", etc seems like overkill.

So, to start, I think we should just have a "Remove" option for now.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

@Daniel: Are you still working on this?

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

We talked on IRC.

Comment 7

[Daniel Lim [:dlim]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46d328899195

Comment 8

[Daniel Lim [:dlim]]

Attachment: Bug 1278840 - FF for Android offers no way to remove arbitrary sites from history apart from top sites or recent history.

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

Comment 9

[Daniel Lim [:dlim]]

Attachment: Screenshot_20160720-163644.png

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8772951 [details]
Bug 1278840 - FF for Android offers no way to remove arbitrary sites from history apart from top sites or recent history.

https://reviewboard.mozilla.org/r/65624/#review63124

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:382
(Diff revision 1)
> +
> +
> +                if (bookmarkId != -1) {

nit: two empty lines?

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:421
(Diff revision 1)
> +        // Don't show the context menu for folders.
> +        if (info.isFolder) {
> +            return;
> +        }

Do folders show up in the awesomebar? I've never seen them :)

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:427
(Diff revision 1)
> +        if (info.isFolder) {
> +            return;
> +        }
> +
> +        MenuInflater inflater = new MenuInflater(view.getContext());
> +        inflater.inflate(R.menu.home_contextmenu, menu);

Maybe we should create a separate menu xml file for the search? Just to avoid having to deal with all the other options here.

::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:419
(Diff revision 1)
>                          extra = "bookmark";
>                      }
>  
>                      Telemetry.sendUIEvent(TelemetryContract.Event.UNSAVE, TelemetryContract.Method.CONTEXT_MENU, extra);
>                      mDB.removeBookmarksWithURL(cr, mUrl);
> +                    mDB.removeHistoryEntry(cr, mUrl);

This doesn't seem to be right: This is the branch to remove a bookmark and now we are removing the history for this URL too.

Clicking the remove option from the search should only remove the history - maybe it would be easier to control this using a separate action in a separate menu xml file (with the same string 'remove' though)?

Comment 11

[Daniel Lim [:dlim]]

If an item is both a bookmark and history item, because it's an history item we would show the 'remove' option, but what should be the outcome of pressing 'remove'? Either: 

1) Still have it show as bookmarked in awesomebar results and bookmark panel but no longer in history panel
2) Removed from both bookmark and history panel

Or something else

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Daniel Lim [:dlim] from comment #11)
> 2) Removed from both bookmark and history panel

this! 

I don't want to overthink the issue here. Removing it from History and Bookmarks makes the most sense.

Comment 13

[Daniel Lim [:dlim]]

Comment on attachment 8772951 [details]
Bug 1278840 - FF for Android offers no way to remove arbitrary sites from history apart from top sites or recent history.

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

Comment 14

[Daniel Lim [:dlim]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b87f7087da7

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8772951 [details]
Bug 1278840 - FF for Android offers no way to remove arbitrary sites from history apart from top sites or recent history.

https://reviewboard.mozilla.org/r/65624/#review64432

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:424
(Diff revision 2)
> +    public void onCreateContextMenu(ContextMenu menu, View view, ContextMenuInfo menuInfo) {
> +        if (!(menuInfo instanceof BrowserSearchContextMenuInfo)) {
> +            return;
> +        }
> +
> +        BrowserSearchContextMenuInfo info = (BrowserSearchContextMenuInfo) menuInfo;

This seems to be a new class and it's not included in this patch?

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:427
(Diff revision 2)
> +        }
> +
> +        BrowserSearchContextMenuInfo info = (BrowserSearchContextMenuInfo) menuInfo;
> +
> +        // Show contextmenu only if it's a history item
> +        if(info.historyId == -1) {

nit: Space after 'if'.

Running 'mach gradle checkstyle' should find such style nits.

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:451
(Diff revision 2)
> +
> +        final int itemId = item.getItemId();
> +
> +        if (itemId == R.id.home_remove) {
> +            // Position for Top Sites grid items, but will always be -1 since this is only for BrowserSearch result
> +            final int position =  -1;

nit: Two spaces after '='

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:453
(Diff revision 2)
> +
> +        if (itemId == R.id.home_remove) {
> +            // Position for Top Sites grid items, but will always be -1 since this is only for BrowserSearch result
> +            final int position =  -1;
> +
> +            (new RemoveItemByUrlTask(context, info.url, info.itemType, position)).execute();

nit: Technically the braces are not needed and you can write:
> new Task().execute();

::: mobile/android/branding/unofficial/res/menu/browser_contextmenu.xml:1
(Diff revision 2)
> +<?xml version="1.0" encoding="utf-8"?>

This should be in mobile/android/base/resources/..

Otherwise this will only be picked up by local builds.

Comment 16

[Daniel Lim [:dlim]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=469201cceec2

Comment 17

[Daniel Lim [:dlim]]

Comment on attachment 8772951 [details]
Bug 1278840 - FF for Android offers no way to remove arbitrary sites from history apart from top sites or recent history.

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

Comment 18

[Daniel Lim [:dlim]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c93ec0af289

Comment 19

[Daniel Lim [:dlim]]

Comment on attachment 8772951 [details]
Bug 1278840 - FF for Android offers no way to remove arbitrary sites from history apart from top sites or recent history.

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

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8772951 [details]
Bug 1278840 - FF for Android offers no way to remove arbitrary sites from history apart from top sites or recent history.

https://reviewboard.mozilla.org/r/65624/#review64832

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:425
(Diff revision 4)
> +        // Show contextmenu only if it's a history item
> +        if (info.historyId == -1) {
> +            return;
> +        }

You added code to handle bookmarks and "combined" items too. Why do we only show the context menu for history items then?

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearchContextMenuInfo.java:13
(Diff revision 4)
> +import android.view.View;
> +
> +/**
> + * A ContextMenuInfo for BrowserSearch
> + */
> +public class BrowserSearchContextMenuInfo extends HomeContextMenuInfo{

If we only extend but don't add anything, can't we just use HomeContextMenuInfo?

Comment 21

[Daniel Lim [:dlim]]

(In reply to Anthony Lam (:antlam) from comment #12)
> (In reply to Daniel Lim [:dlim] from comment #11)
> > 2) Removed from both bookmark and history panel
> 
> this! 
> 
> I don't want to overthink the issue here. Removing it from History and
> Bookmarks makes the most sense.


Sebastian pointed this out: From a user perspective, would it be weird to see the "remove" option only showing for certain bookmarks (in this case a bookmark item which is also a history item) and not other bookmarks (bookmark only item)

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Daniel Lim [:dlim] from comment #21)
> (In reply to Anthony Lam (:antlam) from comment #12)
> > (In reply to Daniel Lim [:dlim] from comment #11)
> > > 2) Removed from both bookmark and history panel
> > 
> > this! 
> > 
> > I don't want to overthink the issue here. Removing it from History and
> > Bookmarks makes the most sense.
> 
> 
> Sebastian pointed this out: From a user perspective, would it be weird to
> see the "remove" option only showing for certain bookmarks (in this case a
> bookmark item which is also a history item) and not other bookmarks
> (bookmark only item)

To clarify:

An item can be:
* Bookmark
* History
* History and bookmark

The current implementation only shows 'remove' in the context menu for:
* History
* History and bookmark (removes both)

I think we should show it for "just bookmark" too. It's more consistent and a user can't really distinguish between "bookmark with history = visited bookmark" and "bookmark without history = unvisited bookmark").

Comment 23

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Sebastian Kaspari (:sebastian) from comment #22)
> I think we should show it for "just bookmark" too. It's more consistent and
> a user can't really distinguish between "bookmark with history = visited
> bookmark" and "bookmark without history = unvisited bookmark").

I'd agree. 

I didn't even think about this as a separate case originally TBH. To a user, typing in the search bar will just pull up and filter the urls in the Awesomescreen. So, long-pressing anything there should offer this "Remove" function.

Comment 24

[Daniel Lim [:dlim]]

Comment on attachment 8772951 [details]
Bug 1278840 - FF for Android offers no way to remove arbitrary sites from history apart from top sites or recent history.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65624/diff/4-5/

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8772951 [details]
Bug 1278840 - FF for Android offers no way to remove arbitrary sites from history apart from top sites or recent history.

https://reviewboard.mozilla.org/r/65624/#review68016

r+: After rebasing and switching to the new snackbar API we can land this.

::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:428
(Diff revision 5)
> +            SnackbarHelper.showSnackbar((Activity) mContext,
> +                    mContext.getString(R.string.page_removed),
> +                    Snackbar.LENGTH_LONG);

This call seems to use an old API. After rebasing your patch this should fail to build (See new SnackbarBuilder class).

Comment 26

[Daniel Lim [:dlim]]

Comment on attachment 8772951 [details]
Bug 1278840 - FF for Android offers no way to remove arbitrary sites from history apart from top sites or recent history.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65624/diff/5-6/

Comment 27

[Daniel Lim [:dlim]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a414133005b6

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ffbbf5e2a2b2
FF for Android offers no way to remove arbitrary sites from history apart from top sites or recent history. r=sebastian

Comment 29

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

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

Comment 30

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_20160819-174510.png

Verified as fixed using:
Device: One A2001 (Android 5.1.1)
Build: Firefox for Android 51.0a1 (2016-08-18)

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

Nice little feature (being able to delete history by long pressing something while searching in the URLbar). It's a bit late. 51.0 is already released. But we haven't rolled it out to all users - so maybe worth adding.

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Sebastian, how can we tell that it hasn't been rolled out to all users? Is something different particularly for 53, here?   I could note it as "Long press on search results to remove from Awesomebar history" but without a link to an explanation that seems a bit confusing.

Comment 33

[Sebastian Kaspari (:sebastian; :pocmo)]

Oh, all users with 51+ should have it. At that time we had 51 build and in the play store but in staged rollout (10%) so I thought we could still add it to the release notes. :)