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.

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

Forbid new empty cosmetic filters from being created via right click menu #8270

Merged
merged 1 commit into from
Mar 18, 2021

Conversation

antonok-edm
Copy link
Collaborator

Resolves brave/brave-browser#14763

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

These are mostly identical to the steps in #8156, but with additional care given to cases which may generate "empty" rules. Differences are highlighted in bold.

Migration

Using a previous browser version (1.22.x or earlier):

  1. Visit brave://adblock in a new tab
  2. Add some text of your choice to the "Custom filters" box
  3. Visit a website of your choice
  4. Right-click on a page element of your choice
  5. Select "Brave > Block element via selector"
  6. Confirm the block
  7. Repeat steps 3-6 a few times to block a variety of items through the legacy system. Block multiple items on the same page at least once. Keep track of what is being blocked. Be sure to close the prompt window, press cancel, or just press OK with an empty rule each at least once.

Then, testing the same profile after the version upgrade:

  1. Visit brave://adblock in a new tab
  2. Verify that any text previously added to the "Custom filters" box is still present
  3. Verify that new lines have been added to the "Custom filters" box, including one corresponding to each item previously blocked using the right click menu, and that no rules were generated that are empty after the ##
  4. Revisit some of the websites and confirm that any previously blocked items are still hidden

Adding new filters

Just on the new version:

  1. Visit a website of your choice
  2. Right-click on a page element of your choice
  3. Select "Brave > Block element via selector"
  4. Confirm the block
  5. Verify that the blocked item is hidden, and remains hidden after a page refresh
  6. Reopen or refresh brave://adblock and verify that a new line has been added corresponding to the newly blocked item
  7. Repeat steps 1-6 at least one more time
  8. Repeat steps 1-6 for each of the following, and ensure that no new lines are created in brave://adblock: closing the prompt window, pressing cancel on the prompt window, and pressing OK with an empty rule in the prompt window

@antonok-edm antonok-edm requested a review from bsclifton March 16, 2021 19:23
@antonok-edm antonok-edm self-assigned this Mar 16, 2021
@antonok-edm
Copy link
Collaborator Author

Recommending @stephendonner for QA here after the excellent job he did on #8156 😄

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and confirmed this works as expected 😄👍 I ran into this when you did the original PR but didn't think anything of it (just deleted the empty lines)

Thanks for coming back and fixing it!

@bsclifton
Copy link
Member

Going to mark the branches which passed as CI/skip and then restart iOS by rebasing/pushing

@bsclifton bsclifton added CI/skip-android Do not run CI builds for Android CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Mar 18, 2021
@bsclifton bsclifton force-pushed the forbid-new-empty-cosmetic-filters branch from 417e85d to 54b258b Compare March 18, 2021 05:53
@bsclifton
Copy link
Member

iOS looks good! Ready to merge 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent empty selectors from being migrated from legacy cosmetic filters
2 participants