Closed
Bug 1400825
Opened 3 years ago
Closed 3 years ago
Allow dismissing suggested sites from top sites
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect, P1)
Firefox for Android Graveyard
Awesomescreen
Tracking
(fennec+, firefox55 unaffected, firefox56 unaffected, firefox57 verified, firefox58 fixed)
VERIFIED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
fennec | + | --- |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | --- | verified |
firefox58 | --- | fixed |
People
(Reporter: hsivonen, Assigned: mcomella)
References
Details
(Whiteboard: [mobileAS])
Attachments
(6 files, 1 obsolete file)
120.34 KB,
image/png
|
Details | |
90.52 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
mcomella
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
liuche
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
34.23 KB,
image/png
|
Details | |
44.67 KB,
image/png
|
Details |
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.
Assignee | ||
Comment 1•3 years ago
|
||
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]
Reporter | ||
Comment 2•3 years ago
|
||
(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.
Assignee | ||
Comment 3•3 years ago
|
||
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
Assignee | ||
Comment 4•3 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #3) > we're splitting out removing from top sites without deleting from history. bug 1402003.
Assignee | ||
Updated•3 years ago
|
Assignee: nobody → michael.l.comella
Assignee | ||
Comment 5•3 years ago
|
||
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
Updated•3 years ago
|
Flags: needinfo?(s.kaspari)
Updated•3 years ago
|
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Flags: needinfo?(s.kaspari)
Comment 6•3 years ago
|
||
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.
Assignee | ||
Comment 7•3 years ago
|
||
(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 hidden (mozreview-request) |
Comment 9•3 years ago
|
||
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)
Assignee | ||
Comment 10•3 years ago
|
||
Taking over for Sebastian...
Assignee: s.kaspari → michael.l.comella
Flags: needinfo?(michael.l.comella)
Assignee | ||
Comment 11•3 years ago
|
||
Here's what we look like if we completely collapse top sites.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•3 years ago
|
Attachment #8911269 -
Attachment is obsolete: true
Comment 14•3 years ago
|
||
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.
Assignee | ||
Comment 15•3 years ago
|
||
Assignee | ||
Comment 16•3 years ago
|
||
fwiw, here's what we used to look like when we dismissed all the top sites.
Assignee | ||
Comment 17•3 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Comment 19•3 years ago
|
||
mozreview-review |
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 20•3 years ago
|
||
mozreview-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+
Assignee | ||
Comment 21•3 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 24•3 years ago
|
||
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
Assignee | ||
Comment 25•3 years ago
|
||
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?
Assignee | ||
Comment 26•3 years ago
|
||
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?
![]() |
||
Comment 27•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/10241c6a7b3a https://hg.mozilla.org/mozilla-central/rev/a54290a32fd6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Updated•3 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
status-firefox57:
--- → affected
Comment 28•3 years ago
|
||
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+
Updated•3 years ago
|
Attachment #8911351 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 29•3 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/2ac4b62c58a1 https://hg.mozilla.org/releases/mozilla-beta/rev/c7907ac20968
Comment 30•3 years ago
|
||
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
Updated•1 month ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•