uBlockOrigin / uBlock-issues

This is the community-maintained issue tracker for uBlock Origin
https://github.com/gorhill/uBlock
920 stars 77 forks source link

remove-attr does not always work #1445

Closed dimisa-RUAdList closed 3 years ago

dimisa-RUAdList commented 3 years ago

Example: https://news.rambler.ru/conflicts/45563172-smi-politsiya-ottesnila-protestuyuschih-so-stupeney-kapitoliya/

When scrolling down the strings, the rule rambler.ru##+js(remove-attr, class, .j-mini-player__video) should fix the player in a static position, but for some reason this does not happen. In version 1.31.2, this problem is absent and the rule works fine.

Config Google Chrome 87.0.4280.88 Firefox 84.0.2 uBlock Origin v1.32.5b7 default + [RU AdList](https://subscribe.adblockplus.org/?location=https://easylist-downloads.adblockplus.org/advblock%2Bcssfixes.txt&title=RU%20AdList%20for%20uBlock%20Origin), [Counters](https://subscribe.adblockplus.org/?location=https://easylist-downloads.adblockplus.org/cntblock.txt&title=RU%20AdList:%20Counters)
gwarser commented 3 years ago

Attribute is removed correctly and then new is created with j-mini-player__video--absolute. Some timing issue?

gorhill commented 3 years ago

I can't find an element matching .j-mini-player__video at that page. What do I need to do for such element to be present?

gorhill commented 3 years ago

Loos to me this work better if the goal is to prevent fixed position:

rambler.ru##.j-mini-player__container:style(position: initial !important)
gwarser commented 3 years ago

I can't find an element matching .j-mini-player__video at that page.

Removed by Ruadlist.

gorhill commented 3 years ago

Duh sorry, I couldn't find it because remove-attr worked on my side... So the issue is probably because remove-attr does not listen to DOM mutations.

uBlock-user commented 3 years ago

So the issue is probably because remove-attr does not listen to DOM mutations.

Yes, that's the issue, also it wouldn't work with 1.31.2 either for that matter.

dimisa-RUAdList commented 3 years ago

The rule rambler.ru##+js(remove-attr, class, .j-mini-player__video) has been working for a long time and stably: Gif

To see the j-mini-player__video element, use rambler.ru#@#+js(remove-attr, class, .j-mini-player__video).

Previously, :style type rules were used instead. But they did not always allow to keep the size of the player window. I can go back to them, but I would like to revive remove-attr.

Add For some reason, the previous video with version v1.32.5b7 was not fully displayed. There is probably some kind of duration limitation. Here it is separately: Gif

gorhill commented 3 years ago

I will add support for mutation observer in remove-attr in next dev build.

dimisa-RUAdList commented 3 years ago

I added a temporary fix, but when using it, the player window blinks when scrolling. Bad decision, but better than nothing.

https://github.com/easylist/ruadlist/commit/5687030498c08f291af974919a782e9a6a0864a6

gwarser commented 3 years ago

rambler.ru##+js(aeld, DOMContentLoaded, .j-mini-player__video) is working for me.

dimisa-RUAdList commented 3 years ago

@gwarser It's much better than: rambler.ru#?#.j-mini-player__container:style(position: initial !important; width: initial !important; height: initial !important)

Changed: https://github.com/easylist/ruadlist/commit/9691055bf06bc1b0b459e40f1e4f7b3b300c73cb

But still I would like to restore the remove-attr.

krystian3w commented 3 years ago

https://github.com/uBlockOrigin/uBlock-issues/issues/1445#issuecomment-756165152 - maybe too in rc?

when should be 100% compatible with AdGuard idea based on 20 ms delay after monitor blink someting:

https://github.com/AdguardTeam/Scriptlets/blob/master/wiki/about-scriptlets.md#-%EF%B8%8F-remove-class and after that periodically in order to DOM tree changes.


Real example for rc: https://github.com/AdguardTeam/AdguardFilters/blob/3e5090a4ace93cac26c9352831e07b0bebbd64ee/AnnoyancesFilter/sections/cookies_specific.txt#L4956

facebook.com#%#//scriptlet("remove-class", "_31e", "body > ._li") - now guest users have locked scroll after try read fanpages due delay on facebook with addition _31e in DOM tree.

Maybe more from cookies_specific also based "DOM tree changes" instead on OnLoad / DomContentLoaded (I suppose filmweb.pl).

gorhill commented 3 years ago

@dimisa-RUAdList use a third argument to indicate to the remove-attr scriptlet to stay and act on DOM changes, i.e.:

rambler.ru##+js(remove-attr, class, .j-mini-player__video, stay)
uBlock-user commented 3 years ago

After the update, rambler.ru##+js(remove-attr, class, .j-mini-player__video) itself works too

gorhill commented 3 years ago

It was working before the update on my side -- as pointed out above, timing issue -- and nothing functionally changed if the 3rd argument is not used.

dimisa-RUAdList commented 3 years ago

Indeed, in version 1.32.5b8, the old rule rambler.ru##+js(remove-attr, class, .j-mini-player__video) also works fine, without the third argument. However, I find the ability to linger for remove-attr scriplet quite helpful. But I will keep the current rule rambler.ru##+js(addEventListener-defuser, DOMContentLoaded, .j-mini-player__video) for now. In the current stable version 1.32.4 for Firefox, nothing else works except for it (except :style).

dimisa-RUAdList commented 3 years ago

I think I found another case.

Latest Google Chrome, Firefox, uBlock Origin 1.34.1rc0 default + RU AdList, Counters

  1. Open: https://www.lostfilm.run/series/DOTA_Dragons_Blood/season_1/episode_6/photos
  2. Click on any photo: https://i.imgur.com/a1a75oy.jpg
  3. Rule lostfilm.run##+js(remove-attr, oncontextmenu)didn't work: https://i.imgur.com/aDaXU9C.jpg

If you refresh the page, then everything works as it should.

ghost commented 3 years ago

If you refresh the page, then everything works as it should.

  1. Opened gallery by your steps use "Ajax" instead reload page from "0 progress".
  2. Possible change url in address bar without fully reload.
  3. I don't recommend block "Ajax" mechanism to use version scriptlet without , stay.

This may need use , stay:

lostfilm.run##+js(remove-attr, oncontextmenu, stay)

If you must support uBO older than 1.33.0 try:

       userResourcesLocation https://raw.githubusercontent.com/gorhill/uBlock/master/assets/resources/scriptlets.js

uBO for Firefox legacy "preQuantum" (older than 1.16.4.28) maybe needed is build legacy file with older syntax for userResourcesLocation:

# remove attr with dom/node changes
remove-attr.js application/javascript
(function() {
  const token = '{{1}}';
  if ( token === '' || token === '{{1}}' ) { return; }
  const tokens = token.split(/\s*\|\s*/);
  let selector = '{{2}}';
  if ( selector === '' || selector === '{{2}}' ) {
      selector = `[${tokens.join('],[')}]`;
  }
  let behavior = '{{3}}';
  let timer;
  const rmattr = ( ) => {
      timer = undefined;
      try {
          const nodes = document.querySelectorAll(selector);
          for ( let node of nodes ) {
              for ( let attr of tokens ) {
                  node.removeAttribute(attr);
              }
          }
      } catch(ex) {
      }
  };
  const mutationHandler = mutations => {
      if ( timer !== undefined ) { return; }
      let skip = true;
      for ( let i = 0; i < mutations.length && skip; i++ ) {
          const { type, addedNodes, removedNodes } = mutations[i];
          if ( type === 'attributes' ) { skip = false; }
          for ( let j = 0; j < addedNodes.length && skip; j++ ) {
              if ( addedNodes[j].nodeType === 1 ) { skip = false; break; }
          }
          for ( let j = 0; j < removedNodes.length && skip; j++ ) {
              if ( removedNodes[j].nodeType === 1 ) { skip = false; break; }
          }
      }
      if ( skip ) { return; }
      if ( 'requestIdleCallback' in self ) {
          timer = self.requestIdleCallback(rmattr, { timeout: 67 });
      } else {
          timer = self.setTimeout(rmattr, 1);
      }
  };
  const start = ( ev ) => {
      if ( ev ) { self.removeEventListener(ev.type, rmattr, true); }
      rmattr();
      if ( /\bstay\b/.test(behavior) === false ) { return; }
      const observer = new MutationObserver(mutationHandler);
      observer.observe(document.documentElement, {
          attributes: true,
          attributeFilter: tokens,
          childList: true,
          subtree: true,
      });
  };
  if ( document.readyState !== 'complete' && /\bcomplete\b/.test(behavior) ) {
      self.addEventListener('load', start, true);
  } else if ( document.readyState === 'loading' ) {
      self.addEventListener('DOMContentLoaded', start, true);
  } else {
      start();
  }
})();

If somebody still base on version older than 1.16.4.28 https://github.com/gorhill/uBlock-for-firefox-legacy/commit/bba23972faebc3df43b07a942b4870749f7845aa#diff-e6f727d342440f308271bdcac31b1830a345a5b30d13ae0349be72de85d97751

D4niloMR commented 7 months ago

2 cases possibly related:

https://github.com/uBlockOrigin/uAssets/pull/21422

and in the images on https://animefire.plus/

animefire.plus##[oncontextmenu], [ondragstart]:remove-attr(/oncontextmenu|ondragstart/) doesn't work, but animefire.plus##+js(ra, oncontextmenu|ondragstart) works.

The issue happens only in Chromium based browsers. I see that with :remove-attr the attribute is gone but the event listener is still there.

Screenshot Left side :remove-attr not used - Right side :remove-attr used

![image](https://github.com/uBlockOrigin/uBlock-issues/assets/70459964/9032e960-05ac-479d-98c7-c90be2b0b5fe)

uBlock-user commented 7 months ago

@D4niloMR animefire.plus##[oncontextmenu], [ondragstart]:remove-attr(/oncontextmenu|ondragstart/):watch-attr(oncontextment,ondragstart)

try

D4niloMR commented 7 months ago

@D4niloMR animefire.plus##[oncontextmenu], [ondragstart]:remove-attr(/oncontextmenu|ondragstart/):watch-attr(oncontextment,ondragstart)

Didn't work

gorhill commented 7 months ago

The issue happens only in Chromium based browsers

Yes, this is a known issue, discussed elsewhere I can't remember. The procedural cosmetic filters execute in the isolated world, and it seems with Chromium changing on... attribute will not be reflected in the main world's on... property. Hence why the scriptlet remove-attr is still available to take care of such cases for the time being.

garry-ut99 commented 7 months ago

Yes, this is a known issue, discussed elsewhere I can't remember.

Discussed here :