Closed Bug 1400825 Opened 2 years ago Closed 2 years ago

Allow dismissing suggested sites from top sites

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 58
Iteration:
1.30
Tracking Status
fennec + ---
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- verified
firefox58 --- fixed

People

(Reporter: hsivonen, Assigned: mcomella)

References

(Depends on 1 open bug)

Details

(Whiteboard: [mobileAS])

Attachments

(6 files, 1 obsolete file)

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.
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.
tracking-fennec: --- → ?
Summary: Can't remove suggested sites → Allow dismissing top sites (and suggested sites) without deleting from history
Whiteboard: [mobileAS]
(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.
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.
tracking-fennec: ? → +
Iteration: --- → 1.30
Priority: -- → P1
Summary: Allow dismissing top sites (and suggested sites) without deleting from history → Allow dismissing suggested sites from top sites
(In reply to Michael Comella (:mcomella) from comment #3)
> we're splitting out removing from top sites without deleting from history.

bug 1402003.
Assignee: nobody → michael.l.comella
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).
Assignee: michael.l.comella → nobody
Flags: needinfo?(s.kaspari)
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Flags: needinfo?(s.kaspari)
Attached image 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.
(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.
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.
Flags: needinfo?(michael.l.comella)
Taking over for Sebastian...
Assignee: s.kaspari → michael.l.comella
Flags: needinfo?(michael.l.comella)
Here's what we look like if we completely collapse top sites.
Attachment #8911269 - Attachment is obsolete: true
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.
fwiw, here's what we used to look like when we dismissed all the top sites.
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 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.
Attachment #8911350 - Flags: review+
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).
Attachment #8911351 - Flags: review?(liuche) → review+
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.`
Pushed by michael.l.comella@gmail.com:
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 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
Attachment #8911350 - Flags: approval-mozilla-beta?
Comment on attachment 8911351 [details]
Bug 1400825: Collapse top sites if there are no sites.

Approval Request Comment: please see comment 25.
Attachment #8911351 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/10241c6a7b3a
https://hg.mozilla.org/mozilla-central/rev/a54290a32fd6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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
Attachment #8911350 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8911351 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed on Beta 57.0b5.
Devices:
Asus ZenPad 8.0 Z380KL (Android 6.0.1)
Huawei MediaPad M2 (Android 5.1.1)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.