Brave-browser: [Desktop] Top sites on New Tab Page are missing

[MikeScotty]

Test plan

See https://github.com/brave/brave-core/pull/6584

Description

The top sites disappear after closing and opening brave

Steps to Reproduce

1. Browse a website until it comes up in the top sites list
2. Pin the site in the top sites list
3. Close then reopen brave

Actual result:

Top sites are gone

Expected result:

Top sites remain where they #were

Reproduces how often:

Easily reproduced

Brave version (brave://version info)

1.13.82 Chromium: 85.0.4183.83 (Official Build) (64-bit)

Version/Channel Information:

• Can you reproduce this issue with the current release? Yes
• Can you reproduce this issue with the beta channel? N/A
• Can you reproduce this issue with the nightly channel? N/A

Other Additional Information:

• Does the issue resolve itself when disabling Brave Shields? N/A
• Does the issue resolve itself when disabling Brave Rewards? N/A
• Is the issue reproducible on the latest version of Chrome? Yes

Miscellaneous Information:

I also have brave set to delete history after closing the app but that never caused the top sites to disappear before

[themew]

Seeing this as well in 1 out of 3 desktop linux machines. Top sites, even toggled on and off have disappeared.

[rebron]

cc: @simonhong

[simonhong]

@MikeScotty The fix for https://github.com/brave/brave-browser/issues/9929 is landed in stable(1.13).
With this fix, sites that cleared from history will be deleted from top site in NTP because top sites data is generated from history service.
I think we need to provide the way to add sites to top sites tiles in NTP manually and these manually added sites should be managed differently from sites comes from top sites history.

[bsclifton]

We definitely have some work to do on new tab page ☹️ A while back, @karenkliu had worked with @rebron, @bradleyrichter, and others to put together a spec for new tab page enhancements, specifically with top site tiles (specifically being able to add)

I think the right solution here might be to match the logic Chromium has:

• New profile is in My shortcuts mode
• By default My shortcuts mode will query topsites API and show those results. If there are less than 6 (or whatever maximum number is), show an Add site button
• If user never touches the tiles (ex: deletes or moves), they will continue to match the output from the topsites API
• As soon as user touches the tiles, topsites API is not called anymore
• Allow an option to switch back to the topsites API (Most visited sites)

We might be able to get all of this logic for free. You can demo yourself by visiting brave://new-tab-page which is the Chromium new tab page and compare this to what Brave is showing. Since our page is React, we might have some implementation work to do- but I think the polymer code is storing the custom tiles as a profile preference. We can make those same calls and eliminate the janky code that we have in place (using Redux)

@rebron @karenkliu WDYT? 😄

[karenkliu]

@bsclifton Yep - this is pretty much what we had planned in top sites spec and it's just been waiting to be implemented. I agree with most of what you said, except these two points:

• By default My shortcuts mode will query topsites API and show those results. If there are less than 6 (or whatever maximum number is), show an Add site button
• If user never touches the tiles (ex: deletes or moves), they will continue to match the output from the topsites API

In our spec, we've defined that the default top sites behavior is shown by frecency until the user indicates they would like to customize their shortcuts themselves instead of having it be shown automatically by Brave (this is the way it works in Chrome as well).

While the top sites are shown by frecency, there is a maximum of 6 tiles displayed. But no matter how many displayed, the Add site function is available to the user. Once they add a site, the top sites behavior switches from frecency to user-curated.

Along with that user action, these are the other actions that causes the top sites behavior to switch from frecency to user-curated:

• Adds a new site with their own URL or picks from auto-populated suggestions. The user can add unlimited top site tiles; we have UI to handle this.
• Edits a frequently visited site and puts in a site of their own choosing
• Deletes any of the frequently visited sites
• Reorders a site by dragging and dropping

[bsclifton]

Working through a proof of concept on this. I currently have the NTP showing the same results as Chromium (deleting all of the bug ridden custom code we have for maintaining this list and allowing pinning, etc). Fully matching the Chromium behavior should be quite easy and would be a nice cleanup (will fix a LOT of bugs)

Very early preview work in progress branch viewable here:
https://github.com/brave/brave-core/compare/bsc-top-sites-rework

Will continue to check this out. Thanks for your patience folks 😄

[bsclifton]

If I finish the work on this branch (above), it should fix:

• Top/Pinned Sites Can't be Removed (Reappear after refresh)
• "Undo" delete of a top site tile does not restore the tile to its previous position
• Top site pinning

and then it makes future work easier

• Makes adding/editing tiles VERY easy (https://github.com/brave/brave-browser/issues/7493)
• chrome://new-tab-page supports custom backgrounds; we could tie into this fairly easily (https://github.com/brave/brave-browser/issues/5838)
• we could access colors from the chrome://new-tab-page or from the browser theme if we'd like (https://github.com/brave/brave-browser/issues/404)

[MikeScotty]

So you're saying that in the next version of brave will those problems fixed?

[bsclifton]

@MikeScotty I can't promise a version, but I am working through this and will try to finish ASAP

[MikeScotty]

@bsclifton ok

[GeetaSarvadnya]

Verification passed on

Brave | 1.17.53 Chromium: 86.0.4240.111 (Official Build) dev (64-bit)
-- | --
Revision | b8c36128a06ebad76af51591bfec980224db5522-refs/branch-heads/4240@{#1290}
OS | Windows 10 OS Version 1903 (Build 18362.1139)

• Verified the STR from the description in both clean and upgraded profile
• Verified the test plan from https://github.com/brave/brave-core/pull/6584
• Testing is covered in https://github.com/brave/brave-browser/issues/9457#issuecomment-712306639
• Encountered https://github.com/brave/brave-browser/issues/12354 and https://github.com/brave/brave-browser/issues/12378

---

Verified passed with

Brave   1.17.69 Chromium: 87.0.4280.60 (Official Build) (x86_64)
Revision    12697cfeb273d7de95cf9b18350d2c457f58224c-refs/branch-heads/4280@{#1352}
OS  macOS Version 10.14.6 (Build 18G6042)

Verified test plan from https://github.com/brave/brave-core/pull/6584
Testing notes can be found under https://github.com/brave/brave-browser/issues/9457#issuecomment-712306639
Additional notes that may be helpful: there is no more pinning top site tiles with 1.17.x. Additionally, modifications to top site tiles made with 1.16.x (and before) are not retained when upgrading to 1.17.x.

[SXMacdara]

Has this actually been fixed?

[MikeScotty]

@SXMacdara Nope. The problem is still there.

[SXMacdara]

@MikeScotty why is this closed if the issue is not fixed?

[MikeScotty]

@SXMacdara I have no idea. Maybe because it's been added to the to-do list?

[bsclifton]

@MikeScotty this will roll out with the 1.17 release which is due to be released today. It's already fixed in Beta and Nightly

[SXMacdara]

Excellent

On Thu, Nov 19, 2020 at 10:42 AM Brian Clifton [e-mail]
wrote:

@MikeScotty https://github.com/MikeScotty this will roll out with the
1.17 release which is due to be released today. It's already fixed in Beta
and Nightly

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/brave/brave-browser/issues/11500#issuecomment-730564407,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ARXAIANVRYPS35HZF7L5TP3SQVRKJANCNFSM4QOODNEA
.

[MikeScotty]

I have version 1.17.73 and the problem is STILL occurring. JSYK I'm using linux mint 20.

[SXMacdara]

@bsclifton I am hoping you are the right person to address about this. I guess I am unsure as to why this is closed. I have the latest version of Brave V1.17.73. Top Sites is not fixed. In this latest version you can't even pin or rearrange your Top Sites anymore. Whatever was done in the way of improvement didn't work. At also appears as if the reviewer above has stated that from their perspective/experience this has not been fixed on their end either.

[bsclifton]

@SXMacdara the not being able to move will be addressed with https://github.com/brave/brave-browser/issues/7493, which also will allow for adding your own top sites. This is the next work I'll be looking at. Are you experiencing a problem with sites missing?

@MikeScotty I can create a new issue for the problem you're experiencing. After updating to 1.17.73, you're seeing the top sites shown on new tab page be removed every time you quit/relaunch? If this is the case, can you please check to see if you have browsing history clearing on exit? You can see this on brave://settings/clearBrowserData under On exit tab. If history is cleared, I am not sure if there is a way to keep the top sites

Basically, the behavior should be matching Chromium completely now (well, except that Brave is missing customize feature which Chromium has, captured with https://github.com/brave/brave-browser/issues/7493). You can test which tiles Chrome would show by visiting brave://new-tab-page and you should see the same tiles there

[MikeScotty]

@bsclifton I have brave set to delete history when the app closes. And as for that new tab, I don't use google. Maybe make something like that except with duckduckgo or qwant?

[bsclifton]

@MikeScotty you don't have to use Google - you can open brave://new-tab-page in Brave and just verify the same thing shows for you there (should be the same as brave://newtab)