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

Update cosmetic filtering to be able to override page elements with !important styles #20177

Closed
antonok-edm opened this issue Dec 20, 2021 · 5 comments · Fixed by brave/brave-core#12038

Comments

@antonok-edm
Copy link
Collaborator

Originally found on https://www.foodnetwork.com/recipes/food-network-kitchen/classic-shrimp-scampi-8849846, where #dfp_bigbox_recipe_top and a few other banner ad spaces are not being collapsed. Corresponding filter rules are present in Easylist as generic class/id cosmetic filters.

Minimal example:

<html>
  <head></head>
  <body>
    <div id="dfp_bigbox_recipe_top" style="padding: 100px; background: #ccc; display: block!important"></div>
  </body>
</html>

The gray box should not be shown with Shields in aggressive mode. uBlock Origin is able to handle this correctly.

Of note is that Brave Shields creates a constructed stylesheet rule, which gets overridden by the inline display: block!important style on the element. uBlock Origin creates an injected stylesheet rule, which has higher precedence.

@antonok-edm antonok-edm added bug feature/shields/adblock Blocking ads & trackers with Shields uBO-parity OS/Android Fixes related to Android browser functionality features/shields/cosmetic-filtering OS/Desktop labels Dec 20, 2021
@ryanbr
Copy link

ryanbr commented Dec 20, 2021

Reported from the community; https://community.brave.com/t/foodnetwork-com-ad-boxes/316301

@pes10k
Copy link
Contributor

pes10k commented Dec 22, 2021

I've confirmed that this works:

// New helper function
void HideCssSelectorsInFrame(const blink::WebLocalFrame& frame,
    const base::ListValue& selectors) {
  std::string stylesheet_text;
  for (const auto &selector : selectors.GetList()) {
    stylesheet_text.append(
        selector.GetString() + " {display:none !important;}\n");
  }
  frame.GetDocument().InsertStyleSheet(
      blink::WebString::FromUTF8(stylesheet_text),
      nullptr,
      blink::WebDocument::kUserOrigin); // <-- this is the important part
}

I'm not sure how to best integrate with the hide1pContent setting, but replacing the below:

  if (force_hide_selectors_list &&
      force_hide_selectors_list->GetList().size() != 0) {
    std::string json_selectors;
    if (!base::JSONWriter::Write(*force_hide_selectors_list, &json_selectors) ||
        json_selectors.empty()) {
      json_selectors = "[]";
    }
    // Building a script for stylesheet modifications
    std::string new_selectors_script = base::StringPrintf(
        kForceHideSelectorsInjectScript, json_selectors.c_str());
    web_frame->ExecuteScriptInIsolatedWorld(
        isolated_world_id_, blink::WebString::FromUTF8(new_selectors_script),
        blink::BackForwardCacheAware::kAllow);
  }

with

  base::ListValue* force_hide_selectors_list;
  if (resources_dict->GetList("force_hide_selectors",
                               &force_hide_selectors_list)) {
    HideCssSelectorsInFrame(*web_frame, *force_hide_selectors_list);
  }

Solves the issue.

@ryanbr
Copy link

ryanbr commented Dec 23, 2021

Another example, possibly related empty spaces: https://community.brave.com/t/arstechnica-com-blank-ad-boxes/317201 on https://arstechnica.com/gadgets/2021/10/surface-laptop-studio-review-one-well-built-weird-convertible-pc/

https://www.gearbrain.com/dac-for-apple-music-iphone-2653373190.html

https://odysee.com/@Geeks+Gamers:e9/spider-man-no-way-home-movie-review:7

https://carbuzz.com/cars/dodge/challenger

@antonok-edm antonok-edm added this to the 1.37.x - Nightly milestone Jan 28, 2022
@antonok-edm antonok-edm changed the title Cosmetic filtering uses suboptimal stylesheet injection API Update cosmetic filtering to be able to override page elements with !important styles Jan 28, 2022
@stephendonner
Copy link

stephendonner commented Feb 10, 2022

Verified PASSED using

Brave 1.37.37 Chromium: 98.0.4758.87 (Official Build) nightly (x86_64)
Revision e4cd00f135fb4d8edc64c8aa6ecbe7cc79ebb3b2-refs/branch-heads/4758@{#1002}
OS macOS Version 12.3 (Build 21E5196i)

Steps:

  1. installed 1.37.37
  2. saved the minimal HTML testcase to my desktop
  3. started a webserver locally via python3 -m http.server
  4. loaded the file over the webserver: http://localhost:8000/Desktop/issue20177/issue20177-testcase.html
  5. confirmed issue in 1.35.101 build, in both aggressive and standard Shields modes
  6. switched back to 1.37.37

Confirmed fix in both aggressive and standard Shields modes: no gray box showing.
Confirmed gray box shows when allow all trackers & ads is set or Shields are Down/Off.

webserver 1.35.101, aggressive 1.35.101, standard 1.37.7, aggressive 1.37.7, standard 1.377 .7, allow all trackers & ads
Screen Shot 2022-02-09 at 6 04 50 PM Screen Shot 2022-02-09 at 6 10 26 PM Screen Shot 2022-02-09 at 6 10 39 PM Screen Shot 2022-02-09 at 6 14 57 PM Screen Shot 2022-02-09 at 6 14 51 PM Screen Shot 2022-02-09 at 6 15 23 PM

Verified PASSED using

Brave 1.37.40 Chromium: 98.0.4758.87 (Official Build) nightly (64-bit)
Revision e4cd00f135fb4d8edc64c8aa6ecbe7cc79ebb3b2-refs/branch-heads/4758@{#1002}
OS Linux

Steps:

  1. installed 1.37.40
  2. saved the minimal HTML testcase to my desktop
  3. started a webserver locally via python3 -m http.server
  4. loaded the file over the webserver: http://localhost:8000/issue20177-testcase.html
  5. confirmed issue in 1.35.101 build, in both aggressive and standard Shields modes
  6. switched back to 1.37.40

Confirmed fix in both aggressive and standard Shields modes: no gray box showing.
Confirmed gray box shows when allow all trackers & ads is set or Shields are Down/Off.

webserver 1.37.7, aggressive 1.37.7, standard 1.377 .7, allow all trackers & ads
Screen Shot 2022-02-10 at 6 17 12 PM Screen Shot 2022-02-10 at 6 13 34 PM Screen Shot 2022-02-10 at 6 13 40 PM Screen Shot 2022-02-10 at 6 13 57 PM

Verified PASSED using

Brave 1.37.46 Chromium: 98.0.4758.87 (Official Build) nightly (64-bit)
Revision e4cd00f135fb4d8edc64c8aa6ecbe7cc79ebb3b2-refs/branch-heads/4758@{#1002}
OS Windows 10 Version 21H2 (Build 19044.1526)

Steps:

  1. installed 1.37.46
  2. saved the minimal HTML testcase to my desktop
  3. started a webserver locally via python3 -m http.server
  4. loaded the file over the webserver: http://localhost:8000/issue20177-testcase.html
  5. confirmed issue in 1.35.101 build, in both aggressive and standard Shields modes
  6. switched back to 1.37.46

Confirmed fix in both aggressive and standard Shields modes: no gray box showing.
Confirmed gray box shows when allow all trackers & ads is set or Shields are Down/Off.

webserver 1.37.7, aggressive 1.37.7, standard 1.377 .7, allow all trackers & ads
20177-1 20177-2 20177-3 20177-4

@Uni-verse
Copy link
Contributor

Uni-verse commented Mar 18, 2022

Verified using Beta 1.37.98, Chromium 99.0.4844.74 on Samsung GS 21 running Android 12, Samsung Galaxy Tab S7 running Android 11

1 2 3 4
Screen Shot 2022-03-18 at 12 42 06 PM Screen Shot 2022-03-18 at 12 42 17 PM Screen Shot 2022-03-18 at 12 42 25 PM Screen Shot 2022-03-18 at 12 42 36 PM

Tablet:

1 2 3 4
Screen Shot 2022-03-18 at 1 00 19 PM Screen Shot 2022-03-18 at 1 00 30 PM Screen Shot 2022-03-18 at 1 00 39 PM Screen Shot 2022-03-18 at 1 01 06 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment