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

Top/Pinned Sites Can't be Removed (Reappear after refresh) #9788

Closed
jonathansampson opened this issue May 14, 2020 · 7 comments · Fixed by brave/brave-core#6584
Closed

Top/Pinned Sites Can't be Removed (Reappear after refresh) #9788

jonathansampson opened this issue May 14, 2020 · 7 comments · Fixed by brave/brave-core#6584
Assignees
@bsclifton
Labels
bug 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

@jonathansampson
Copy link
Contributor

jonathansampson commented May 14, 2020 •
edited by bsclifton
Loading

Test plan

See brave/brave-core#6584

Description

I spoke with a user last night who was finding it impossible to remove a pinned site from their New Tab Page. Removing it would appear to work, but refreshing the NTP would show the same site again. I took a look at the grid-sites-data-v1 in localStorage, and found that the removedSites array had over 700 entries. Additionally, the entries in the array were extremely redundant, with identical objects repeated.

Steps to Reproduce

N/A

Actual result:

Users may not be able to remove pinned sites.

Expected result:

Users can remove pinned sites.

Reproduces how often:

N/A

Brave version (brave://version info)

1.8.96

Other Additional Information:

Wiping out grid-sites-data-v1 from localStorage appears to have resolved the issue.

Miscellaneous Information:

image
@rebron rebron added the priority/P3 The next thing for us to work on. It'll ride the trains. label May 14, 2020
@cezaraugusto cezaraugusto removed their assignment May 18, 2020
@adamwinn
Copy link

I see this same issue. I'm on Version 1.10.90 Chromium: 83.0.4103.97 (Official Build) (64-bit) but it's been happening for a couple of months now

Screenflick Movie 93

@norsehealer
Copy link

I'd be ok with a local file for them so that I can easily add/remove the ones I want.

right now I have 2 icons for youtube. one for https://www.youtube.com and youtube.com. unpinning one and clicking the X doesn't remove it.

@jonathansampson
Copy link
Contributor Author

Would you (@adamwinn or @norsehealer) be available for a Zoom call? I'd love to take a more direct look at the issue you're experiencing. If so, please make sure your browser is fully up-to-date, and let me know when would be most convenient for you 🙂

@adamwinn
Copy link

@jonathansampson I've already deleted my grid-sites-data-v1 file to fix it

@jonathansampson
Copy link
Contributor Author

@adamwinn Thank you for the update; I'm sorry it came to that. We'll work on identifying the root issue, and fixing it in a future build.

@norsehealer
Copy link

Brave is up to date
Version 1.10.93 Chromium: 83.0.4103.106 (Official Build) (64-bit)

I've literally removed the same 3-5 sites and just hit refresh and they're back. I've unpinned 1 and both youtube icons, removed the one and both, refresh and they're back. Even the pinned sites aren't being shown until I remove a few sites.

This was referenced Sep 4, 2020
Closed
Merged
@bsclifton bsclifton self-assigned this 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
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. Modified STR to check removing a tile was confirmed as per the test notes in "Removing sites and Most Visited mode" case.

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
Modified STR to check removing a tile was confirmed as per the test notes in "Removing sites and Most Visited mode" case of #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
bug 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
8 participants
@jonathansampson @adamwinn @cezaraugusto @bsclifton @rebron @norsehealer @LaurenWags @btlechowski