Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Desktop] Top site behavior seems wrong / unpredictable #11551

Closed
BartAgterbosch opened this issue Sep 2, 2020 · 3 comments · Fixed by brave/brave-core#6584
Closed

[Desktop] Top site behavior seems wrong / unpredictable #11551

BartAgterbosch opened this issue Sep 2, 2020 · 3 comments · Fixed by brave/brave-core#6584
Assignees
@bsclifton
Labels
feature/new-tab OS/Desktop priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass-macOS QA Pass-Win64 QA/Test-Plan-Specified QA/Yes release-notes/exclude
Milestone
1.17.x - Release #1

Comments

@BartAgterbosch
Copy link

BartAgterbosch commented Sep 2, 2020 •
edited by bsclifton
Loading

Test plan

See brave/brave-core#6584

Description

Top site pinning is still broken, doesn't do anything and get's regularly replaced by random sites, this has been an issue for well over 3 to 4 years now, on both windows 10 and linux, is it ever going to get fixed? Or has it simply been given up on?
This was referenced Sep 4, 2020
Closed
Merged
@ikeman32
Copy link

ikeman32 commented Sep 8, 2020

Oh this is definitely broken but I have only just experienced any problems and posted about it in the community:
https://community.brave.com/t/top-sites-has-a-potential-bug/159826

This started happening immediately after the last update release for Linux, before that there were no issues. What happened is that the pinned top sites completely vanished and had to be repinned as they would appear on the top sites, but they continue to randomly vanish or be replaced by an unpinned site.

@bsclifton
Copy link
Member

Thanks for the report @ikeman32 - I'm working on a fix along with @simonhong and @petemill, which can be tracked here:
brave/brave-core#6584

It gets rid of the need for pinning; once a site is there, it'll stay there. You can optionally switch to a "most visited" mode

@bsclifton bsclifton self-assigned this Sep 10, 2020
@bsclifton bsclifton changed the title [Desktop] Top site pinning [Desktop] Top site behavior seems wrong / unpredictable Sep 10, 2020
bsclifton added a commit to brave/brave-core that referenced this issue Sep 21, 2020
@bsclifton
Fixing a few remaining items:
e2ef755 
- Move gridSiteState to sessionStorage (not localStorage) and default value for "use custom links".
  Includes cleanup of previous localStorage value (if user has one).
  sessionStorage seems to be persisted between tabs and should be cleaned
  up when browser is closed.
- X to remove tiles is now right aligned
- Update top sites mode/visible setting when NTP gets updates
- Fixed disabled icon showing when tiles are movable (condition was reversed)
- Title case the new preference (to match existing preference)

And of course, closing keywords for this PR:
Fixes brave/brave-browser#11500
Fixes brave/brave-browser#9788
Fixes brave/brave-browser#9457
Fixes brave/brave-browser#11551
bsclifton added a commit to brave/brave-core that referenced this issue Sep 22, 2020
@bsclifton
Fixing a few remaining items:
6a4f80e 
- Move gridSiteState to sessionStorage (not localStorage) and default value for "use custom links".
  Includes cleanup of previous localStorage value (if user has one).
  sessionStorage seems to be persisted between tabs and should be cleaned
  up when browser is closed.
- X to remove tiles is now right aligned
- Update top sites mode/visible setting when NTP gets updates
- Fixed disabled icon showing when tiles are movable (condition was reversed)
- Title case the new preference (to match existing preference)

And of course, closing keywords for this PR:
Fixes brave/brave-browser#11500
Fixes brave/brave-browser#9788
Fixes brave/brave-browser#9457
Fixes brave/brave-browser#11551
bsclifton added a commit to brave/brave-core that referenced this issue Sep 24, 2020
@bsclifton
Fixing a few remaining items:
8ed0745 
- Move gridSiteState to sessionStorage (not localStorage) and default value for "use custom links".
  Includes cleanup of previous localStorage value (if user has one).
  sessionStorage seems to be persisted between tabs and should be cleaned
  up when browser is closed.
- X to remove tiles is now right aligned
- Update top sites mode/visible setting when NTP gets updates
- Fixed disabled icon showing when tiles are movable (condition was reversed)
- Title case the new preference (to match existing preference)

And of course, closing keywords for this PR:
Fixes brave/brave-browser#11500
Fixes brave/brave-browser#9788
Fixes brave/brave-browser#9457
Fixes brave/brave-browser#11551
@rebron rebron added the priority/P3 The next thing for us to work on. It'll ride the trains. label Oct 2, 2020
bsclifton added a commit to brave/brave-core that referenced this issue Oct 6, 2020
@bsclifton
Fixing a few remaining items:
98e093e 
- Move gridSiteState to sessionStorage (not localStorage) and default value for "use custom links".
  Includes cleanup of previous localStorage value (if user has one).
  sessionStorage seems to be persisted between tabs and should be cleaned
  up when browser is closed.
- X to remove tiles is now right aligned
- Update top sites mode/visible setting when NTP gets updates
- Fixed disabled icon showing when tiles are movable (condition was reversed)
- Title case the new preference (to match existing preference)

And of course, closing keywords for this PR:
Fixes brave/brave-browser#11500
Fixes brave/brave-browser#9788
Fixes brave/brave-browser#9457
Fixes brave/brave-browser#11551
@bsclifton bsclifton added this to the 1.17.x - Nightly milestone Oct 7, 2020
@LaurenWags
Copy link
Member

LaurenWags commented Nov 17, 2020 •
edited by btlechowski
Loading

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 brave/brave-core#6584
Testing notes can be found under #9457 (comment)
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.

Verification passed on

Brave 1.17.69 Chromium: 87.0.4280.60 (Official Build) (64-bit)
Revision 12697cfeb273d7de95cf9b18350d2c457f58224c-refs/branch-heads/4280@{#1352}
OS Windows 7 Service Pack 1 (Build 7601.24544)

Verified test plan from brave/brave-core#6584
Testing notes can be found under #9457 (comment)

@LaurenWags LaurenWags mentioned this issue Nov 18, 2020
Closed
@LaurenWags LaurenWags mentioned this issue Nov 18, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bsclifton bsclifton

Labels
feature/new-tab OS/Desktop priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass-macOS QA Pass-Win64 QA/Test-Plan-Specified QA/Yes release-notes/exclude
Projects
None yet
Milestone
1.17.x - Release #1
Development

Successfully merging a pull request may close this issue.

Make top sites work like Chromium brave/brave-core
6 participants
@bsclifton @rebron @ikeman32 @LaurenWags @btlechowski @BartAgterbosch