1400825 - Allow dismissing suggested sites from top sites

Status: VERIFIED FIXED

(Firefox for Android :: Awesomescreen, defect, P1)

Description

[Henri Sivonen (:hsivonen)]

Steps to reproduce:
1) Install Fennec
2) Open a new tab
3) Tap and hold on the Facebook suggested site.

Actual results:
A menu opens. The menu has no option for remove the sites from the Top Sites view.

Expected results:
Expected to be able to remove suggested sites the way history items can be removed.

Comment 1

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

Relatedly, the only way to remove a site from your top sites is to delete that site from your history, which sucks (and which is why you can't delete suggested sites - they're not in your history).

bug 1393178 is for Pocket items and it seems we already have "Remove" for Highlights.

fwiw, if you don't visit the suggested sites, they're quickly replaced with the sites you do visit which is why I'm assuming we never considered this bug a priority in the past.

Comment 2

[Henri Sivonen (:hsivonen)]

(In reply to Michael Comella (:mcomella) from comment #1)
> fwiw, if you don't visit the suggested sites, they're quickly replaced with
> the sites you do visit which is why I'm assuming we never considered this
> bug a priority in the past.

The situation makes for a bad first impression, so this is important even if the unwanted icons went away after prolonged use. A new user sees a bunch of stuff they don't want and they can't immediately get rid of what they don't want, which gives a feeling of sponsor placement overriding the user being in control.

Comment 3

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

We want to be able to support this functionality because it existed before - we're splitting out removing from top sites without deleting from history.

Comment 4

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

(In reply to Michael Comella (:mcomella) from comment #3)
> we're splitting out removing from top sites without deleting from history.

bug 1402003.

Comment 5

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

Won't be able to get to this until tomorrow so unassigning so maybe Sebastian can take a stab.

My thoughts on approach here:
1) We could do it in the DB: this is nice because we'll get exactly the right number of top sites in our Cursor. I think this is the way we used to do it so maybe it'd be easy to re-use/revive the code.

2) We could store "hidden" suggested sites in shared prefs and skip over hidden suggested sites in the Cursor.

N.B.:
- You can re-use R.id.dismiss and the String "Remove" as we do for highlights (we swapped "Dismiss" -> "Remove" for accessibility reasons because "dismiss" felt like "dismiss this dialog").
- In telemetry, suggested sites are reported as "suggested" if they've never been visited but "top" (i.e. a history item) if they've been visited once or more: I assume this is how tiles are considered on the back-end so the code to decide how to dismiss top sites may also need to take this into account (for visited sites, a user may need to 1) Delete from History and 2) Remove a suggested site or we can make Delete from History also remove from suggested sites).

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: no-topsites.png

I started to write a patch for this. However this introduces a new problem: We do not have an "empty state" in the case there are no top sites. Right now we'd show an empty single row. Also the divider at the bottom is not at the correct position.

Comment 7

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

(In reply to Sebastian Kaspari (:sebastian) from comment #6)
> Also the divider at the bottom is not at the correct position.

I think Chenxia is looking into this - there was a patch in bug 1402145 and I encouraged Chenxia to file a new bug.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1400825 - Show option for removing suggested sites from top sites.

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

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

I've to leave now. I attached my patch if you want to pick it up. It's not super nice but working - as a workaround maybe enough. However we might want to hide top sites completely if there are none.

Comment 10

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

Taking over for Sebastian...

Comment 11

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

Attachment: Screenshot: collapsed top sites

Here's what we look like if we completely collapse top sites.

Comment 12

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

Attachment: Bug 1400825 - Show option for removing suggested sites from top sites.

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

Comment 13

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

Attachment: Bug 1400825: Collapse top sites if there are no sites.

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

Comment 14

[Chenxia Liu [:liuche]]

Okay, but can you also add a screenshot of "Pocket and Highlights disabled"? Since we're talking about empty states here, what did we do before? Please flag bbell for feedback on the totally empty state.

Comment 15

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

Attachment: Screenshot: totally empty state (pocket, highlights disabled)

Comment 16

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

Attachment: Screenshot (56): old top sites empty state

fwiw, here's what we used to look like when we dismissed all the top sites.

Comment 17

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

Spoke with bbell on Slack: 

mcomella [13:59] 
Hey! We’re adding the ability to dismiss suggested sites because we had that functionality in the old top sites. However, this leads to an edge case where users can dismiss *all* suggested sites and have no history so it’ll be empty. I don’t think it’s worth adding a lot of code to handle this case but I just wanted to fly by the UI I have right now for that case: https://bug1400825.bmoattachments.org/attachment.cgi?id=8911349 (93kB)

[14:00] 
I think collapsing the top margin any more will be difficult because it’s outside the bounds of the top sites view (it’s probably the top margin of the pocket title)

[14:00] 
So I think this is good enough for a really rare edge case (and yes, we’re aware of the divider through the highlights empty state)

bryanbell [14:01] 
it’s good enough

mcomella [14:01] 
Cool, ty! :slightly_smiling_face:

bryanbell [14:01] 
a left over message about there not being any top-sites would be best

[14:01] 
but. for now this is ok

mcomella [14:03] 
Chenxia, in review, also mentioned the top sites, pocket, and highlights disabled case: https://bug1400825.bmoattachments.org/attachment.cgi?id=8911352 (35kB)

[14:03] 
Is this also okay for the unlikely edge case?

[14:03] 
(at this point, I don’t know why this person won’t bother just disabling the panel…)

bryanbell [14:03] 
you don’t want to leave the impression it’s broken

mcomella [14:04] 
So caveat: we can’t add/uplift strings if we want to get this in 57 (edited)

[14:04] 
But if this is too broken to get into 57 the way it is, that’s okay too

bryanbell [14:04] 
blank is ok

mcomella [14:04] 
Okay, I’ll file a bug to fix this in 58

[14:05] 
ty

bryanbell [14:05] 
it’ll fill in eventually (edited)

[14:05] 
right?

mcomella [14:05] 
If the user visits a site, it’ll add the site

[14:05] 
However, they may clear history on exit, or something similar, so they could end up seeing this a lot if they have that work flow

Comment 18

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

Comment on attachment 8911351 [details]
Bug 1400825: Collapse top sites if there are no sites.

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

Comment 19

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

Comment on attachment 8911350 [details]
Bug 1400825 - Show option for removing suggested sites from top sites.

https://reviewboard.mozilla.org/r/182828/#review188062

Sebastian authored this patch, despite me pushing the commits, so I'm going to review it.

This looks good. I like that it uses a lot of existing code so it's a small change and low risk.

Comment 20

[Chenxia Liu [:liuche]]

Comment on attachment 8911351 [details]
Bug 1400825: Collapse top sites if there are no sites.

https://reviewboard.mozilla.org/r/182830/#review188074

::: commit-message-d8fe6:1
(Diff revision 2)
> +Bug 1400825: Collapse top sites if there are no sites. r=liuche

Before you push the other patch you should add a reviewer to the commit message.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/TopPanelRow.java:69
(Diff revision 2)
> +
> +        // The cursor is null when the view is first created. The view will layout with the number of rows we
> +        // set initially (while we load the Cursor) and these rows (with no content) will be briefly visible
> +        // to users. Since we expect 2 rows to be the most common for users, we show 2 rows at this point;
> +        // the RecyclerView will gracefully animate a collapse if we display fewer.
> +        if (cursor == null || cursor.getCount() > 4) {

This 4 is a little hard-coded - do we reference "number of tiles wide" somewhere else? It would be good to use the same reference (so when we update the dynamic width bug for rotation or tablets this doesn't get overlooked).

Comment 21

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

Comment on attachment 8911351 [details]
Bug 1400825: Collapse top sites if there are no sites.

https://reviewboard.mozilla.org/r/182830/#review188074

> This 4 is a little hard-coded - do we reference "number of tiles wide" somewhere else? It would be good to use the same reference (so when we update the dynamic width bug for rotation or tablets this doesn't get overlooked).

I swapped in `TopSitesPage.NUM_COLUMNS.`

Comment 22

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

Comment on attachment 8911350 [details]
Bug 1400825 - Show option for removing suggested sites from top sites.

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

Comment 23

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

Comment on attachment 8911351 [details]
Bug 1400825: Collapse top sites if there are no sites.

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10241c6a7b3a
Show option for removing suggested sites from top sites. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/a54290a32fd6
Collapse top sites if there are no sites. r=liuche

Comment 25

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

Comment on attachment 8911350 [details]
Bug 1400825 - Show option for removing suggested sites from top sites.

Approval Request Comment
[Feature/Bug causing the regression]: Activity Stream
[User impact if declined]: Users cannot remove the default top sites ("suggested sites") such as Facebook and Youtube. This is a regression from the old top sites experience.

[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Ideally - Bogdan can test.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not very.

[Why is the change risky/not risky?]: The first patch adds an option on suggested sites tiles to display "Remove" and adds a callback to hide the top site on press. This uses existing code to handle the complex portions (e.g. hiding top sites, adding click handlers) so is only a few code branches and one-line statements to enable/disable views or take an action. The second patch allows us to show 0 rows if there are zero top sites (rather than being stuck on 1 row) and its changes can only really affect the height of the top sites panel, which is a small area to affect. The core change is also simple (if rows is 0, use height 0, otherwise use the existing equation). Overall, it seems low risk.

[String changes made/needed]: None

Comment 26

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

Comment on attachment 8911351 [details]
Bug 1400825: Collapse top sites if there are no sites.

Approval Request Comment: please see comment 25.

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/10241c6a7b3a
https://hg.mozilla.org/mozilla-central/rev/a54290a32fd6

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8911350 [details]
Bug 1400825 - Show option for removing suggested sites from top sites.

Fix a bad UX, taking it.
Should be in 57b3

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2ac4b62c58a1
https://hg.mozilla.org/releases/mozilla-beta/rev/c7907ac20968

Comment 30

[Oana Horvath]

Verified as fixed on Beta 57.0b5.
Devices:
Asus ZenPad 8.0 Z380KL (Android 6.0.1)
Huawei MediaPad M2 (Android 5.1.1)