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

[Follow up to #33241] Sometimes blocked element reappeared again after refreshing the page #43040

Closed
1 of 6 tasks
hffvld opened this issue Dec 24, 2024 · 3 comments · Fixed by brave/brave-core#27269
Closed
1 of 6 tasks
Assignees
Labels
bug feature/CSS-element-block feature/shields The overall Shields feature in Brave. OS/Android Fixes related to Android browser functionality QA Pass - Android ARM QA/Yes release-notes/exclude

Comments

@hffvld
Copy link
Contributor

hffvld commented Dec 24, 2024

Description

Follow-up to #33241.

Steps to reproduce

  1. Launch Brave
  2. Enable the flag #block-element-feature and relaunch Brave
  3. Go to https://www.thebarreltap.com/ and wait for the age restriction pop-up
  4. Tap Brave Shields > Advances controls
  5. Block element > Select the element > Tap Block Element
  6. Refresh the page a few times > Observe

Actual result

Sometimes blocked elements reappeared again after refreshing the page.


2024-12-23_11-06-38.mp4

Expected result

If the distracting element is blocked, it must not reappear after refreshing the page.

Reproduces how often

Intermittent issue

Brave version

Brave build: 1.75.115
Chromium: 132.0.6834.57 (Official Build) canary (64-bit)

Device

  • Brand/model: Galaxy Fold 6, Pixel 7
  • Android version: 14, 15

Channel information

  • release (stable)
  • beta
  • nightly

Reproducibility

  • with Brave Shields disabled
  • with Brave Rewards disabled
  • in the latest version of Chrome

Miscellaneous information

No response

@hffvld hffvld added bug feature/CSS-element-block feature/shields The overall Shields feature in Brave. OS/Android Fixes related to Android browser functionality QA/Yes labels Dec 24, 2024
@ghost
Copy link

ghost commented Dec 24, 2024

This is wrong. It is actually doing what it is suppose to be doing.

xpath is a Procedural though, and the nature of Procedurals is that they are selectors made out of JS, which means, they are slower to apply than normal native cosmetics.

While xpath on its own is really powerful, it is not native to the adblocker, so it needs to be a slow procedural, unless Google adds xpath as a native selector, so it is an expected behavior and this is the exact reason why Procedurals shouldn't be used and they should only be the last resort if there is no other way to catch an element, they are not native and therefore slow, and cause elements to show and hide on refresh.

I have like a whole issue reporting every single issue with Procedurals compared to uBlock and xpath's only issue is that it makes some websites like quora really slow, but it is still slow by design.

So the main problem here, is that the Element Picker (I refuse to call it Block Elements still after 2 years #27313) on Android was decided to use the xpath instead of the usual native selectors like class or ID, plus the xpath used by Brave, is not even the 'nice' one that could have something like this: //*[@id="m-a-v-outer-contaner"]/div/div[2], which can help make better filters.

For example, Desktop Element Picker and uBlock would pick .m-a-v m-a-v_styled, which is not better but not worst, at least you can use that interchangeably between Adblockers, but even that one is still not the one you should use for this, because you are still not hiding the overlay. In the case of this page, it should be #m-a-v-outer-contaner since it is the toppest one that should hold every annoying element.
But the problem is you can only get that with Devtools, or the (left) slider uBlock Element Picker.

At the moment, not even even desktop's Brave Element picker is going to pick that ID, because since this update with the new Element Picker, they are adding weird rules when you use the slider, it will either pick only .m-a-v m-a-v_styled which is not good since it doesn't get rid of the overlay, and if you move the slider it will make invalid rules like #m-a-v-outer-contaner > >.

Of course, I am the ones that think people shouldn't use Element Picker anyway, Element Pickers are not good, while if you use Devtools for filters, you can use so many selectors (like Attributes) with pseudo-classes like :has(), and that's something Element Pickers don't seem to do, making them useless in my opinion.

So anyway, in resume, the issue is not the adblocker or the rule itself, but how Element Picker works, so the title should be something like: "Brave Element Picker shouldn't use xpath, a slow procedural, for any cosmetic, because it is not recommended, it should use only native selectors like other Element Pickers (Included Brave's desktop)". Because xpath is working as expected.
I even tested performance, and I got Brave doing the xpath in 3.75s, while uBlock did it around 4.11s. In my last test changing some settings Brave went down to 3s, and uBlock was a little slower with 3.22s so, Brave got it faster, but it is still slow and noticeable, and I doubt Brave Team can do so much when Procedurals are made of JS, and are not native, unless Brave makes them truly native.

I even found another issue against using xpath this way, I even got /html/body/div[17]/div/div[2] in some tests on Desktop and not [20] as mobile always seems to give, which would make the rule not only invalid, since the element wouldn't be found.

@kjozwiak
Copy link
Member

The above requires 1.75.166 or higher for 1.75.x verification 👍

@hffvld hffvld added the QA/In-Progress Indicates that QA is currently in progress for that particular issue label Jan 25, 2025
@hffvld
Copy link
Contributor Author

hffvld commented Jan 25, 2025

Verified on Pixel 6 using version(s):

Device/OS: Pixel 6 / oriole-user 15 AP4A.241205.013 release-keys
Brave build: 1.75.166
Chromium: 132.0.6834.111 (Official Build) beta (64-bit) 

STEPS:

  1. Follow the STR/TP from [Follow up to #33241] Sometimes blocked element reappeared again after refreshing the page #43040 (comment)
  2. Verify

ACTUAL RESULTS:

  • Verified that the already blocked element will not reappear after the page is refreshed/Brave is relaunched.
  • Verified that the XPath is not used to block page elements with the Block element feature.

2025-01-24_16-48-16.mp4

@hffvld hffvld added QA Pass - Android ARM and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Jan 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug feature/CSS-element-block feature/shields The overall Shields feature in Brave. OS/Android Fixes related to Android browser functionality QA Pass - Android ARM QA/Yes release-notes/exclude
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants