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

RESOLVED FIXED in Firefox 51

Status

()

Firefox for Android
Awesomescreen
P3
normal
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: nemo, Assigned: dlim)

Tracking

Trunk
Firefox 51
Points:
---

Firefox Tracking Flags

(relnote-firefox 53+, firefox51 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
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

2 years ago
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?
Flags: needinfo?(dlim)
Flags: needinfo?(alam)
(Reporter)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
(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.
Flags: needinfo?(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?

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.
Flags: needinfo?(alam)
(Assignee)

Updated

2 years ago
Assignee: nobody → dlim
@Daniel: Are you still working on this?
Flags: needinfo?(dlim)
Priority: -- → P3
We talked on IRC.
Assignee: dlim → nobody
Flags: needinfo?(dlim)
(Assignee)

Comment 7

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=46d328899195
(Assignee)

Comment 8

a year ago
Created 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 commit: https://reviewboard.mozilla.org/r/65624/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65624/
Attachment #8772951 - Flags: review?(s.kaspari)
(Assignee)

Comment 9

a year ago
Created attachment 8773091 [details]
Screenshot_20160720-163644.png
Assignee: nobody → dlim
Status: NEW → ASSIGNED
Attachment #8773091 - Flags: feedback?(alam)
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)?
Attachment #8772951 - Flags: review?(s.kaspari)
(Assignee)

Comment 11

a year ago
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
Flags: needinfo?(alam)
(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.
Flags: needinfo?(alam)
Attachment #8773091 - Flags: feedback?(alam) → feedback+
(Assignee)

Comment 13

a year ago
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/
Attachment #8772951 - Flags: review?(s.kaspari)
(Assignee)

Comment 14

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b87f7087da7
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.
Attachment #8772951 - Flags: review?(s.kaspari)
(Assignee)

Comment 16

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=469201cceec2
(Assignee)

Comment 17

a year ago
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/
Attachment #8772951 - Flags: review?(s.kaspari)
(Assignee)

Comment 18

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c93ec0af289
(Assignee)

Comment 19

a year ago
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/
Attachment #8772951 - Flags: review?(s.kaspari)
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?
(Assignee)

Comment 21

a year ago
(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)
Flags: needinfo?(alam)
(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").
(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.
Flags: needinfo?(alam)
Comment hidden (mozreview-request)

Comment 25

a year ago
mozreview-review
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).
Attachment #8772951 - Flags: review?(s.kaspari) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 27

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a414133005b6
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 28

a year ago
Pushed by kwierso@gmail.com:
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
Keywords: checkin-needed

Comment 29

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ffbbf5e2a2b2
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Created attachment 8782841 [details]
Screenshot_20160819-174510.png

Verified as fixed using:
Device: One A2001 (Android 5.1.1)
Build: Firefox for Android 51.0a1 (2016-08-18)
status-firefox51: fixed → verified
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.
relnote-firefox: --- → ?
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.
relnote-firefox: ? → 53+
Flags: needinfo?(s.kaspari)
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. :)
Flags: needinfo?(s.kaspari)
You need to log in before you can comment on or make changes to this bug.