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

Add a block this element feature to the brave-extension (cosmetic adblock filters) #94

Closed
30 of 32 tasks
bbondy opened this issue Mar 13, 2018 · 16 comments
Closed
30 of 32 tasks

Comments

@bbondy
Copy link
Member

bbondy commented Mar 13, 2018

The intention here would be for users to be able to block elements on a given page and basically have those elements hidden or removed.

  • This change would persist between sessions (ex: saved to state / local storage)
  • they should be able to remove any rules that have been created (in case it was done on accident or if they wanted it back)

Milestones

  • Add a context menu which shows on right click / cmd + click
  • Have the element blocked on the page (display:none or remove from DOM)
  • Persist rules to store (ex: utilize the existing local storage used elsewhere)
  • Add a context menu item for removing rules on a given domain
  • Unit tests
Unit Tests Progress
  • test
    • app
      • actions
        • webNavigationActionsTest
          • onCommitted
      • background
        • actions
          • cosmeticFilterActionsTest
        • api
          • cosmeticFilterAPITest
            • addSiteCosmeticFilter
            • removeSiteFilter
            • removeAllFilters
            • applySiteFilters
        • events
          • cosmeticFilterEventsTest
            • contextMenu
        • reducers
          • cosmeticFilterReducerTest
            • should handle initial state
            • ON_BEFORE_NAVIGATION
            • ON_COMMITTED
            • WINDOW_REMOVED
            • WINDOW_FOCUS_CHANGED
            • TAB_DATA_CHANGED
            • TAB_CREATED
            • SHIELDS_PANEL_DATA_UPDATED
            • SHIELDS_TOGGLED
            • SITE_COSMETIC_FILTER_REMOVED
            • ALL_COSMETIC_FILTERS_REMOVED
            • SITE_COSMETIC_FILTER_ADDED
      • state
        • chrome.storage.local
  • Clean up / refactor code
  • Fix webpack dev environment

Taking this to the next level

After this functionality is up and running, we can create a new issue for altering the behavior. For example, we might want to show a red box around the item that's selected and have the user acknowledge they would like to add a rule. Perhaps they could change the selector. That work should be captured in a different issue

  • highlight corresponding element with candidate selector
  • DOM observer
  • remove via JS
  • no flicker on load

We will also need a page where the rules can be visible (ex. about:adblock) and perhaps editable (possibly broken down by domain). That work should be captured in a different issue

@cezaraugusto cezaraugusto added the feature/shields The overall Shields feature in Brave. label May 30, 2018
@bbondy bbondy added this to the Milestone 3: June-July milestone Jun 6, 2018
@bbondy bbondy added feature/shields/adblock Blocking ads & trackers with Shields enhancement labels Jun 10, 2018
@bsclifton bsclifton changed the title Add a block this element feature to the brave-extension Add a block this element feature to the brave-extension (cosmetic adblock filters) Jul 2, 2018
@LWGShane
Copy link

This is the only thing that's preventing me from using Brave as my default browser as I use this to remove social media annoyances (who to follow/friend, trending, etc.).

I also hope that the blocked elements will be able to be synced to the cloud; so when we have to reinstall Brave we don't have to re-block the elements we blocked before.

@Snuupy
Copy link

Snuupy commented Jul 12, 2018

Something to keep in mind: the things you block on desktop may be different from the things you block on mobile.

From a design standpoint, the ad placements (or whatever you desire to block) may also be located in different locations (leading to different CSS selectors, xpaths, etc). I don't plan to solve that in the first release as there is not a simple, elegant solution.

cc: @DVSlayer42

@jonathansampson
Copy link
Contributor

@Snuupy How are we blocking? Do we plan to inject a custom style element with rules that target the element(s)?

@Snuupy
Copy link

Snuupy commented Jul 12, 2018

@jonathansampson

The current code will inject custom user style sheets. This is just a display:none on that element (specifically via CSS selector).

A benefit to doing it via CSS is that any element that triggers the filter list will be blocked, regardless of when it actually loads (via fetch/ajax calls since many ads are 3rd party hosted).

Unfortunately, in-line styles always have priority over user-style sheets. There are 2 ways to fix this problem:

  1. Create a DOM watcher/observer like what gorhill does, which removes DOM elements that match the CSS selector on the fly, or
  2. Modify the browser where only our extension can set user-style sheets that override the in-line elements on the C++/brave-core side.

As a note, uBO uses the DOM watcher model, and I think that's the right direction to take this. I understand there will be some overhead, but this will allow for faster iteration and better web compatibility. Changing of how the browser renders pages is definitely a much larger change than writing javascript that operates on top of a page.

@chris1000
Copy link

Would it be possible to simply build in uBlock? At least as a temporary fix while a first party solution is being developed? Possibly a sub branch of the main code?

@bbondy
Copy link
Member Author

bbondy commented Jul 25, 2018

That wouldn't lead to the best UX. Particularly we have a faster way to do network filtering because we never leave the IO thread and our engine is in C++.

@chris1000
Copy link

I see, thank you for the input and best of luck with the project. Once it's out you'll certainly have me as a permanent user!

@dri94
Copy link

dri94 commented Jul 31, 2018

Is there a commit so we can cherry pick the code?

@Snuupy
Copy link

Snuupy commented Aug 2, 2018

@dri94
Copy link

dri94 commented Aug 6, 2018

@Snuupy thanks, code looks good with comments in PR. However cant cherry pick due ot restricted access to branch. Looking forward to a merge.

@bbondy
Copy link
Member Author

bbondy commented Aug 16, 2018

This has landed so closing.

@srirambv
Copy link
Contributor

srirambv commented Sep 21, 2018

Verification Passed on

Brave 0.55.5 Chromium: 70.0.3538.16 (Official Build) dev (64-bit)
Revision 16ed95b41bb05e565b11fb66ac33c660b721f778-refs/branch-heads/3538@{#306}
OS Linux
  • Verified right click context menu shows Brave-> Block element via selector
  • Verified Clear CSS rules for this site and reload unblocks all elements on a particular site
  • Verified Clear CSS rules for all sites and reload unblocks all elements blocked on all sites
Brave 0.55.6 Chromium: 70.0.3538.16 (Official Build) dev (64-bit)
Revision 16ed95b41bb05e565b11fb66ac33c660b721f778-refs/branch-heads/3538@{#306}
OS Windows 7

Verified passed with

Brave 0.55.10 Chromium: 70.0.3538.22 (Official Build) beta(64-bit)
Revision ac9418ba9c3bd7f6baaffa0b055dfe147e0f8364-refs/branch-heads/3538@{#468}
OS Mac OS X
  • Verified right click context menu shows Brave-> Block element via selector
  • Verified Clear CSS rules for this site and reload unblocks all elements on a particular site
  • Verified Clear CSS rules for all sites and reload unblocks all elements blocked on all sites

@Snuupy
Copy link

Snuupy commented Dec 9, 2018

I realize this is overdue, but I wanted to write down my notes here to capture the future features/roadmap in this issue. This is in no particular order, but is aimed to be a comprehensive to do list for the cosmetic filter feature.

I'd be happy to put this in a new issue if that'll fit better in terms of organization.

To do:

  • insert observer only if user defined rules exist
  • add mutation observer (and only if cosmetic filter rules exist for that particular website)
  • add idempotent checker for filters
  • add toggle for cosmetic filtering
  • when mutation observer/cosmetic filter blocks ads, add to counter in brave icon
  • import/export feature
  • add/import existing cosmetic filter rules (easylist, etc.) - bloom filters
  • look into using a webworker to process filter lists and apply cosmetic filtering (see https://github.com/ampproject/worker-dom)
  • improve front-end (including a picker/selector for DOM elements)
    • add a slider for DOM elements
  • improve automatic CSS/xpath selector tool and handle edge cases better (ex. iframes)
  • add tutorial
  • add caching to cosmetic (and maybe) network filter rules (ex. first 8 characters: Style injection? uBlock-LLC/uBlock#161 (comment))
  • add compatibility flags feature to filter lists (ex. 3rd party cookies off for certain websites for compatibility reasons, etc.)
  • add tool to easily find which filter list rules are blocking certain elements
  • compare perf to other extensions/utilities out there

@mihirkumar
Copy link

mihirkumar commented Mar 5, 2019

Is there a way we could have a feature similar to uBO where one could visually select the element they want to remove from the page? Exactly how uBO's element picker mode works. I feel that is a lot more easy to use than finding the correct CSS selector. A casual user might not know anything about CSS selectors but a visual way to block an annoying element would allow them to make use of this feature. Also it is hard to pinpoint exactly which CSS selector would result in the desired element to be blocked properly. A visual tool would take care of all of these limitations/shortcomings.

@bbondy
Copy link
Member Author

bbondy commented Mar 5, 2019

@mihirkumar yes, @bsclifton and @Snuupy will want to post issues for @Snuupy and your suggestions above

@raine
Copy link

raine commented Mar 25, 2019

The injected CSS added with Block element via selector does not work on elements that have display: block directly set as their style. The display: block takes precedence over the injected display: none.

Screenshot 2019-03-25 at 12 58 45

Screenshot 2019-03-25 at 12 55 46

Apologies if this is not the right place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests