brave / brave-browser

Brave browser for Android, iOS, Linux, macOS, Windows.
https://brave.com
Mozilla Public License 2.0
17.01k stars 2.21k forks source link

[Android] Support cosmetic filtering on Android #9955

Closed Oeufhp closed 3 years ago

Oeufhp commented 4 years ago

Description

I enabled cometic filtering in brave://flags, enable ads blocking in setting and update in brave://components

Steps to reproduce

  1. enter brave://flags and enable cosmetic filtering
  2. restart brave 2 times and open brave again
  3. go to https://apkmirror.com and still see the white space

Actual result

screenshot1 screenshot2

Expected result

No white space

Issue happens on

Device details

Brave version

1.8.112

Additional information

I found this link but there's no solution yet

srirambv commented 4 years ago

Cosmetic filtering is not yet applicable for mobile.

cc: @pes10k @antonok-edm

Oeufhp commented 4 years ago

Thanks, hope it will be available soon

antonok-edm commented 4 years ago

@Oeufhp I'll leave this issue open to track the implementation status. Thank you for the report.

antonok-edm commented 4 years ago

After further investigation, it appears that the cosmetic filtering background and content scripts are not used because Brave Android doesn't yet support extensions. Once https://github.com/brave/brave-browser/issues/4493 is resolved this should theoretically be ready immediately.

srirambv commented 4 years ago

Thanks for the confirmation @antonok-edm 👍

srirambv commented 3 years ago

+1 from @chris122380 via https://github.com/brave/brave-browser/issues/12405

chris122380 commented 3 years ago

Why would this need extension support when other mobile browsers with add filtering support this without extensions? Vivaldi for example.

pes10k commented 3 years ago

@chris122380 because the previous desktop implementation used the extension API, and so the original approach was to add the extension interfaces to Android and then use the desktop filtering implementation.

Porting extension support to android ended up being prohibitive, and so whats captured here is the second go at the problem: to reimplement cosmetic filtering w/o using the extension interface, and then port that to desktop / make that the sole implementation

Furax-31 commented 3 years ago

Nightly 1.21.24 (Android 9)

Despite the implementation of cosmetic filtering, there are still sites with large empty spaces.

Example: https://www.01net.com/

antonok-edm commented 3 years ago

Nightly 1.21.24 (Android 9)

Despite the implementation of cosmetic filtering, there are still sites with large empty spaces.

Example: https://www.01net.com/

cc @ryanbr - can you check if we need to add different filters for this site on mobile? Otherwise it might be an issue with the new implementation.

If that's solved we can close this as a duplicate of https://github.com/brave/brave-browser/issues/13070.

ryanbr commented 3 years ago

On the mobile site, the cosmetics applied is causing the most ad-space issues ##.mrf-adv (in Easylist) @antonok-edm

Specifically: ##div.mrf-heightLocked.mad.mrf-preInsertedMad.sRoba.mrf-adv

mrf-ad

SergeyZhukovsky commented 3 years ago

I see the same issue on old implementation on desktop for apkmirror.com Screenshot from 2021-01-25 15-25-27 Most likely it's something that has been missed in the original implementation as well on apkmirror.com. For https://www.01net.com/ I see empty spaces on mobile, but hard to compare with desktop as desktop version of website on mobile looks still different compare to a real desktop version

Oeufhp commented 3 years ago

I see the same issue on old implementation on desktop for apkmirror.com Screenshot from 2021-01-25 15-25-27 Most likely it's something that has been missed in the original implementation as well on apkmirror.com. For https://www.01net.com/ I see empty spaces on mobile, but hard to compare with desktop as desktop version of website on mobile looks still different compare to a real desktop version

For the desktop just enter brave://flags/#brave-adblock-cosmetic-filtering then enable it and restart the browser

SergeyZhukovsky commented 3 years ago

@Oeufhp it is on by default on desktop. It's available on Android on Brave - Nightly. The thing is that apkmirror.com still has gaps.

Oeufhp commented 3 years ago

@SergeyZhukovsky I tried Nightly in Android yesterday and it was as you said.

antonok-edm commented 3 years ago

I think the 01net.com issue is related to there being no aggressive mode on Android yet (https://github.com/brave/brave-browser/issues/9831). If I toggle Pixel 2 XL in the device toolbar when devtools is open on Desktop, the ad spaces are hidden only in aggressive mode.

cc @pes10k - this may be an issue with our 1p/3p heuristics? Those look like pretty standard 3p Google ads when Shields are down.

On closer investigation it looks like there isn't actually anything 3p in there, there's just data inside that's used by some script to populate the ad. This is the full HTML of one of the ad slots:

<div class="mrf-adv sRoba  mad mrf-heightLocked" data-placement="" data-madtype="sRoba" data-sectionid="home" data-virtualpageid="1" data-loadstrategy="immediate" data-refreshfrequency="0" data-layout="layoutRoba_s" data-invalidatelayout="true" data-loadonevent="navigationChange" data-navigationlevel="0" style="height:320px;" data-scrollposition="1" data-madprocessed="true" data-madid="1">
  <div id="mrf-1"></div>
  <script class="mrf-adv__configuration" type="application/json">
    {"type":"crispr","width":"336","height":"320","layout":null,"json":{"targeting":{"tenant_smart_network_id":"13","tenant_smart_site_id":"283961","tenant_smart_page_id":"1307410","tenant_smart_format_id":"98","tenant_smart_domain":"//securite.01net.com"}},"chunkFilename":"CRISPR","block-on-consent":"_till_responded","rtc_config":{"vendors":{"openwrap":{"PROFILE_ID":"1798","PUB_ID":"155979"}}},"chunkFilename":"CRISPR","multi-size":"200x200,300x50,300x100,300x200,300x250,300x300,320x50,320x100,320x240,320x320,336x280","slotName":"/22571134/crispr/01net.com","adNetworkId":"crispr","adNetworks":[{"name":"rubicon","accountId":"242322","adTags":{"*":"1268008"}},{"name":"rubiconMbid","accountId":"242322","adTags":{"*":"1268008"}},{"name":"indexExchange","adTags":{"*":"306219"}},{"name":"smartadserver","accountId":"219890/996537","adTags":{"*":"65224"}},{"name":"appnexus","adTags":{"*":"13996414"}},{"name":"criteo","adTags":{"300x600":"1459698","728x90":"1459701","300x250":"1459697","160x600":"1459700","320x50":"1459699"}},{"name":"openx","adTags":{"*":"540333635"}},{"name":"amazon","accountId":"","adTags":{}},{"name":"appnexusMbid","adTags":{"*":"15746156"}},{"name":"openxMbid","adTags":{"*":"540612291"}},{"name":"improveDigital","adTags":{"*":"22205957"}},{"name":"improveDigitalMbid","adTags":{"*":"22212246"}}],"sellerID":"892607997","currencyExchange":{"month":1,"year":2021,"rates":{"USD":1.2155039,"EUR":1.0},"base":"EUR"}}
  </script>
</div>
pes10k commented 3 years ago

I fixed the manual tests for this here: https://dev-pages.brave.software/cosmetic-filtering/text-ads.html

pes10k commented 3 years ago

@DiAnTTA should be fixed once tis gets merged https://github.com/brave/brave-core/pull/7751

If not, can you share the full URLs of pages where you're still seeing the problem?

SergeyZhukovsky commented 3 years ago

@DiAnTTA it's not in 1.21.37. It has been merged after that Nightly build was created. The fix will appear in the next Nightly.

bui-lang commented 3 years ago

@SergeyZhukovsky

Screenshot Nightly 1.21.38 ![Screenshot_2021-02-02-21-18-03-647_com brave browser_nightly](https://user-images.githubusercontent.com/74956696/106613345-aa378180-659c-11eb-9f0a-0da25df140dd.jpg) https://zingnews.vn/thong-tin-khach-hang-nguyen-kim-bi-hacker-rao-ban-post1179532.html ![IMG_20210202_212315](https://user-images.githubusercontent.com/74956696/106613744-1f0abb80-659d-11eb-94b8-bb6b09470baa.jpg)
SergeyZhukovsky commented 3 years ago

thanks @DiAnTTA, could you post a full link to the article please?

SergeyZhukovsky commented 3 years ago

nvm @DiAnTTA I see it https://zingnews.vn/thong-tin-khach-hang-nguyen-kim-bi-hacker-rao-ban-post1179532.html

rodrigoswz commented 3 years ago

Nightly v1.21.38

Ad banners still exist on AMP pages of websites like Android Police and 9to5Google, but APKMirror is fixed.

https://www.androidpolice.com/2021/02/02/latest-galaxy-buds-pro-update-improves-anc-and-voice-detect-mode/?amp

https://9to5google.com/2021/02/02/android-11-begins-rolling-out-for-the-nokia-8-3-5g/amp/

Screenshot_20210202-120749502.jpg

Screenshot_20210202-120626.png

antonok-edm commented 3 years ago

@ryanbr can you investigate https://zingnews.vn/thong-tin-khach-hang-nguyen-kim-bi-hacker-rao-ban-post1179532.html? Does it need a regional list? Looks like those are not hidden on desktop in Release/aggressive mode either.

Edit: same for https://www.androidpolice.com/2021/02/02/latest-galaxy-buds-pro-update-improves-anc-and-voice-detect-mode/?amp

antonok-edm commented 3 years ago

@SergeyZhukovsky https://9to5google.com/2021/02/02/android-11-begins-rolling-out-for-the-nokia-8-3-5g/amp/ is a regression, ad spots are correctly hidden in Release/standard blocking mode but not in latest Nightly, even in Aggressive.

Full HTML of one of the ad spots ```html
```
SergeyZhukovsky commented 3 years ago

@orspetol we need to keep it open still as the regression mentioned https://github.com/brave/brave-browser/issues/9955#issuecomment-771859523 isn't merged yet

orspetol commented 3 years ago

@SergeyZhukovsky I did not close this ticket (at least that I know of). Currently diagnosing how this could have happened. Thank you for reopening in my behalf. If it was somehow caused on my end I will vocalize how not to repeat the issue. Thanks.

mihaiplesa commented 3 years ago

https://github.com/brave/brave-core/pull/7751/commits has commits with fixes in commit message which close this issue automatically.

chris122380 commented 3 years ago

In what stable version could we expect to start seeing this? Not until Brave 21?

SergeyZhukovsky commented 3 years ago

@chris122380 yes it's correct, 1.21.x

LaurenWags commented 3 years ago

Verified passed with

Brave | 1.21.54 Chromium: 88.0.4324.152 (Official Build) beta (x86_64)
-- | --
Revision | 6579930fc53b4dc589c042bec9d0a3778326974d-refs/branch-heads/4324@{#2106}
OS | macOS Version 10.15.7 (Build 19H512)

Verified test plan from https://github.com/brave/brave-core/pull/7799. Confirmed no empty spaces from ads on https://9to5google.com/2021/02/02/android-11-begins-rolling-out-for-the-nokia-8-3-5g/amp/.


Verification passed on

Brave 1.21.56 Chromium: 88.0.4324.152 (Official Build) dev (64-bit)
Revision 6579930fc53b4dc589c042bec9d0a3778326974d-refs/branch-heads/4324@{#2106}
OS Ubuntu 18.04 LTS

Verified test plan from https://github.com/brave/brave-core/pull/7799. Confirmed no empty spaces from ads on https://9to5google.com/2021/02/02/android-11-begins-rolling-out-for-the-nokia-8-3-5g/amp/.


Verified passed with

Brave   1.21.59 Chromium: 88.0.4324.182 (Official Build) beta (64-bit)
Revision    73ee5087001dcef33047c4ed650471b225dd8caf-refs/branch-heads/4324@{#2202}
OS  Windows 10 OS Version 1909 (Build 18363.1256)

Verified test plan from https://github.com/brave/brave-core/pull/7799. Confirmed no empty spaces from ads on https://9to5google.com/2021/02/02/android-11-begins-rolling-out-for-the-nokia-8-3-5g/amp/.

chris122380 commented 3 years ago

@SergeyZhukovsky How should we report cosmetic issues for specific webpages? ksl.com for example with the ads removed leaves a gap above the first news article. I think I fixed this with other browsers on Android by adding this to the adblock rules: www.ksl.com##.mobile_wrap_top

From Brave Beta 1.21.51

Screenshot_20210215-224145

pes10k commented 3 years ago

@chris122380 the best thing to do is to report these issue upstream. EasyList is the best option here, since Brave (along with many other projects) all use it, and changes there will have the largest impact on the Web. If the maintainers wont accept the change there, they might refer you to other projects, many of which Brave also pulls from (EasyPrivacy, uBlock Origin, etc).

If none of those projects will accept the change (unlikely, but not impossible), please let us know then and we'll consider the change in a Brave specific list.

But, in general, the higher up the stream, the better (both for impact and maintainability)

rodrigoswz commented 3 years ago

AMP pages are really problematic for me, otherwise I think everything is working.

Another example that the problem persists in the latest Nightly v1.22.34: https://www.androidpolice.com/2021/02/18/pixel-dark-theme-is-no-longer-pitch-black-on-android-12/?amp

Screenshot_20210219-033017.png

antonok-edm commented 3 years ago

Another example that the problem persists in the latest Nightly v1.22.34: https://www.androidpolice.com/2021/02/18/pixel-dark-theme-is-no-longer-pitch-black-on-android-12/?amp

@rodrigoswz I can't reproduce the loading "Ad" box at the top, but I do still see the broken frame under the title.

cc @ryanbr can you take a look? Should just be a cosmetic filter, at least for the white banner

ryanbr commented 3 years ago

Okay, give it 24-48hrs. @rodrigoswz @antonok-edm

https://github.com/easylist/easylist/commit/d7ca65bab51f192617ea947e829a6d5dcde07688

Should clean up the site.

urbenlegend commented 3 years ago

I am using the latest nightly 1.23.4 with cosmetic filtering on (although I believe it is also enabled by default in Nightly) and I still see empty ad spaces on washingtonpost.com and nytimes.com

Screenshot_20210225-092038 Screenshot_20210225-092017

Other sites like apk mirror and that 9to5 link seem to work fine. Is this a regression or do we just need to wait for easylist to catch up?

ghost commented 3 years ago

I can confirm I experience the same issue as urbenlegend on sites like Washington Post or the NYT, among many others - on nightly with cosmetic filtering enabled.

I also have inline ads on sites like google with brave nightly with cosmetic filtering on on android. Not sure i that is the same issue as this or not, but I know on brave desktop they are removed if you set shields to aggressive. Screenshot_20210225-133910

antonok-edm commented 3 years ago

@daphnei-mee - inline first-party ads would be removed by enabling aggressive mode, which isn't supported yet on Android. Progress is tracked here: https://github.com/brave/brave-browser/issues/9831

@urbenlegend - @ryanbr could you investigate washingtonpost.com and nytimes.com?

srirambv commented 3 years ago

Verification passed on OnePlus 6T with Android 10 running 1.21.68 x64 build

apkmirror.com 9to5google.com
image image

Verification passed on Samsung Tab A with Android 10 running 1.21.68 x64 build

apkmirror.com 9to5google.com
image image