Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#2426 closed change (fixed)

Open "block element" dialog as popup window

Reported by: theheroofvn Assignee: kzar
Priority: P3 Milestone: Adblock-Plus-1.11-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: sebastian, greiner, kzar, philll, Ross Blocked By:
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29336084/
https://codereview.adblockplus.org/29336576/

Description (last modified by kzar)

Background

When a user clicks the "Block Element" button and selects an element a dialog is displayed which allows them to tweak the automatically generated filter(s) before saving. It also gives them the chance to abort.

At the moment this dialog is injected into the page as an iframe, while it works fairly well it has a number of problems.

The worst problem is that this requires the block.html page to be marked as "web accessible" for it to work in Chrome. Having web accessible pages allows websites to easily detect Adblock Plus.

What to change

Change the logic so that the dialog is instead shown as a popup window. In Safari the dialog will have to be shown in a new tab instead of a popup as Safari lacks the required APIs to open a popup.

(See the comments for the rational behind this. We tried and weighed up all the options before deciding to go with this approach.)

Hints for testers

Adblock Plus "popup"

When you click on the Adblock Plus icon at the top of the browser window a small "popup" opens that displays our logo, lists the number of blocked adverts and some other things. Things to test:

  • That when you navigate to a "non-page" like about:blank or the Chrome options page, that the "popup" does not show page related information, like number of ads blocked on this page or the "Enabled on this site" indicator.
  • When you're in the process of using the "block element" feature on the page it should give you the option of cancelling, when you're not it should give the option to start blocking elements.

Window opening / closing logic

Some of the code for both Chrome and Safari relating to opening new windows, noticing they've finished loading and noticing they've been closed has been changed as part of this issue. Things to test:

  • That the options page still opens correctly and works as normal.
  • When the "block element" dialog window is closed (not by clicking the cancel button) that the block element feature finishes.
  • The "block element" dialog opens correctly. (In Chrome as a popup window, but for Safari just as a new tab.)

We needed to move the code that makes subscription links work into a separate file. Ensure that functionality still works. Test what happens when you click on subscription links. (For an example try the "Subscribe" button on the testpages https://testpages.adblockplus.org/

Page / Pagemap changes

We've needed to make a tiny change to the Page / Pagemap abstraction inside the code. Unfortunately this means we should test some things that were changed:

  • Test that the "Block element" context menu item is displayed when it should be, and not when it shouldn't. (It is supposed to behave the same as before.)
  • Test that icon animations still work correctly, specifically that when a page is whitelisted the icon should be grey, with the animation slowly changing into a colourful notification icon, and back to the grey ABP icon. To test this whitelist the current webpage, then type this in the extension's console require("icon").startIconAnimation("critical");

Block element

Finally the "Block element" feature itself will need testing! The code has been substantially changed so it will need to be tested thoroughly on both Chrome and Safari. Some ideas:

  • Test blocking elements by right clicking on them and clicking "Block element".
  • Also test blocking elements by using the "Block element" button in the Adblock Plus icon "popup".
  • For Chrome make sure the "Block element" dialog is displayed a small popup window. For Safari ensure it's a new tab instead. Make sure things behave properly when you close the dialog window etc.
  • Make sure that the Adblock plus icon "popup" always gives the option to cancel element blocking when it's active. (Even when it was started from the context menu.)
  • Test that everything still works properly when blocking elements inside iframes using the context menu button. You might find my test page useful http://static.kzar.co.uk/iframe-test/ for this.
  • Try any edge cases you can think of, like what happens when the user navigates to a new page during the middle of blocking an element.

Change History (35)

comment:1 follow-up: Changed 5 years ago by sebastian

  • Cc sebastian added
  • Component changed from Unknown to Platform
  • Sensitive set

Set to confidential, as we don't want to encourage websites to detect Adblock Plus.

We are aware of this and already looked into it, coming to the conclusion that there is no easy way to get rid of web accessible resources. The only reasonable approach would be to use a popup for the "Block element" dialog. However, this wouldn't work on Safari, as popups including those created by extensions must be immediately triggered by user gestures.

As last resort, we could also randomize the filenames of our web accessible resources when building the extension, to mitigate the effect of detection scripts, as they would need to be updated every time we release a new version then.

comment:2 Changed 5 years ago by greiner

  • Cc greiner added

comment:3 Changed 5 years ago by sebastian

  • Priority changed from Unknown to P5

Set to P5 as the possible solutions (outlined above) aren't great.

comment:4 in reply to: ↑ 1 Changed 4 years ago by greiner

  • Tester set to Unknown

Replying to sebastian:

The only reasonable approach would be to use a popup for the "Block element" dialog. However, this wouldn't work on Safari, as popups including those created by extensions must be immediately triggered by user gestures.

I'd be in favor of this solution over the URL randomization. We could open the popup right away, show a loading indicator and populate it asynchronously to not be hindered by that Safari limitation. So if you agree with that approach, I'd be willing to work on that.

comment:5 Changed 4 years ago by greiner

  • Cc kzar added

@kzar What's your opionion on that? I discovered that PageFair started using this approach in their detection scripts (see code below) so it'd be great to tackle this issue before it becomes even more mainstream.

function l(a) {
  if (e.O("1.5.1")) {
    var b = {
        adblock: "chrome-extension://gighmmpiobklfepjocnamgkkbiglidom/img/icon24.png",
        adblock_plus: "chrome-extension://cfhdojbkjhnklbpkdaibdccddilifddb/block.html",
        adblock_pro: "chrome-extension://ocifcklkibdehekfnmflempfgjhbedch/components/block/block.html",
        adblock_premium: "chrome-extension://fndlhnanhedoklpdaacidomdnplcjcpj/img/icon24.png",
        adblock_super: "chrome-extension://knebimhcckndhiglamoabbnifdkijidd/widgets/block/block.html",
        adguard: "chrome-extension://bgnkhhnnamicmpeenaelnjfhikgbkllg/elemhidehit.png",
        adremover: "chrome-extension://mcefmojpghnaceadnghednjhbmphipkb/img/icon24.png",
        ublock: "chrome-extension://epcnnfbjfcgphgdmggkamkmgojdagdnn/document-blocked.html"
      },
      f = [],
      c = 0,
      d = function(d, n) {
        jQuery.ajax({
          url: n,
          success: function() {
            f.push(d)
          },
          complete: function() {
            c += 1;
            c === e.t(b) && a({
              chrome: f
            })
          }
        })
      },
      n;
    for (n in b) d(n, b[n])
  } else a()
}

comment:6 Changed 4 years ago by kzar

  • Cc philll added
  • Priority changed from P5 to P3

I don't know how feasible the popup approach is, or how much added complexity it would cause. I would have to try it to find out.

The randomisation option would be pretty simple, but has the downside that someone could keep an up to date list of possible URLs. This is plausible for a company offering anti-adblocking technology as a service, and they could likely even automate the process if they really wanted to.

So, my opinion is that we should get together a proof of concept for the popup approach for now. If it shows promise we can go that route, otherwise we can just go for URL randomisation or think about a possible third approach.

comment:7 Changed 4 years ago by kzar

  • Owner set to kzar

comment:8 follow-up: Changed 4 years ago by kzar

I've been thinking about this and playing with ideas for a couple of days now. Some thoughts:

  • I don't think using a popup would be too user friendly, I envision that when the user clicks an element to block the main page will rise to the top, effectively hiding the popup. The user would then have to switch back to the popup window to complete the creation of the filter.
  • The URL randomisation approach obviously isn't foolproof, it would be better to find another way.
  • I tried adding some logic to our build process to load the whole block.html and included scripts / stylesheets into a huge srcdoc string for the iframe. This turned out to be a pain for a number reasons and worse it won't work at all as it doesn't allow for access to the extension APIs that we require.
  • I've tried simply blocking AJAX requests to our web accessible resources, this actually seems to work pretty well in Chrome. I'm not sure how web accessible resources work in Safari however. Also perhaps it's possible that the detection scripts could perform the request in another way, for example by creating an iframe?

@Sebastian could you take a look at this proof of concept for Chrome and let me know what you think? Also could you tell me, does this issue apply at all to Safari and if so how do web accessible resources work on that platform?

Last edited 4 years ago by kzar (previous) (diff)

comment:9 in reply to: ↑ 8 ; follow-up: Changed 4 years ago by greiner

Replying to kzar:

  • I don't think using a popup would be too user friendly, I envision that when the user clicks an element to block the main page will rise to the top, effectively hiding the popup. The user would then have to switch back to the popup window to complete the creation of the filter.

There must've been a misunderstanding. The user initiates the element selection either by right-clicking on an element on the page or by selecting "Block element" in the icon popup (chrome.browserAction). The dialog popup (window.open) would only appear after the selection has been made. Therefore the main page won't rise to the top after selecting an element.

comment:10 in reply to: ↑ 9 Changed 4 years ago by kzar

Replying to greiner:

There must've been a misunderstanding. The user initiates the element selection either by right-clicking on an element on the page or by selecting "Block element" in the icon popup (chrome.browserAction). The dialog popup (window.open) would only appear after the selection has been made. Therefore the main page won't rise to the top after selecting an element.

Ah I see, sorry yes I misunderstood. I'll give the popup approach a try next, certainly sounds like it could work.

comment:11 Changed 4 years ago by sebastian

You might want to consider chrome.windows.create (instead window.open). Otherwise, you won't be able to hide the address bar on the popup. Moreover, window.open might get blocked by the browser if the popup isn't brought up by user-gesture. And since message passing is involved the browser might not know that it was originally triggered by a user-gesture.

Of course chrome.windows.create doesn't exist on Safari. However, window.open won't work there either because of the latter issue. But that should be fine, as all resources are web accessible anyway on Safari. While on the other hand the extension's base URL is randomized every time the extension is loaded, rendering it it impossible to detect the extension that way. But unfortunately that means that we need different logic for Safari than for Chrome here.

comment:12 follow-up: Changed 4 years ago by kzar

How about we generate a secret at runtime and then block any requests to our web accessible resources that don't include the secret. Should be fairly foolproof. I've made a quick proof of concept here that seems to work OK.

comment:13 in reply to: ↑ 12 Changed 4 years ago by greiner

Replying to kzar:

How about we generate a secret at runtime and then block any requests to our web accessible resources that don't include the secret. Should be fairly foolproof. I've made a quick proof of concept here that seems to work OK.

I guess that approach should be fine as long as we hide it behind the ext.getURL() implementation so that it transparently returns the URL with the appended secret whenever we're trying to get the URL of a web-accessible resource.

That way we could keep the entire logic in the background page and keep using ext.getURL() as usual.

comment:14 Changed 4 years ago by sebastian

I kinda like that idea. And in fact, it wouldn't be too complicated to implement. We could use chrome.runtime.getMainfest to get the list of web-accessible resources. Adding the secret for these in ext.getURL as suggest by greiner, and blocking requests without valid secret to those files in chrome/ext/background.js, completely transparently.

On the other hand, using a popup would make sense, generally. Injecting content in the DOM of arbitrary pages always bears the risk of undesired side-effects. We had these in the past, and there are still some corner cases we ultimately cannot get rid of as long as we inject UI into the page. For example when the viewport (or the frame) the dialog is injected into isn't large enough. But this isn't the only scenario.

Last edited 4 years ago by sebastian (previous) (diff)

comment:15 Changed 4 years ago by kzar

Cool idea, I've got it working it working with ext.getURL transparently now, what do you think? (It was a little trickier than it sounded because ext/common.js is used by both background and content scripts it means we have to grab the secret from the background if in a content script.)

comment:16 follow-ups: Changed 4 years ago by sebastian

Yeah, that is a problem. And I'm not sure whether, preemptively passing an additional message (and its response) around for every page (and frame) that is loaded, is an appropriate solution. Perhaps we could generate random enough data, based on synchronous DOM and/or extension APIs that are avialable and return consistent data in the background page as well as content scripts, like but possibly not only the UA.

Also, mind mind my second point in favor of popups? ;)

comment:17 in reply to: ↑ 16 Changed 4 years ago by greiner

Replying to sebastian:

Yeah, that is a problem. And I'm not sure whether, preemptively passing an additional message (and its response) around for every page (and frame) that is loaded, is an appropriate solution.

What about using chrome.storage.local instead? We could set a random value on startup and subsequently retrieve it from there. It's accessible both in the background page and in content scripts so we could use it in common.js without having to distinguish between those two environments.

comment:18 in reply to: ↑ 16 ; follow-up: Changed 4 years ago by kzar

Replying to sebastian:

Yeah, that is a problem. And I'm not sure whether, preemptively passing an additional message (and its response) around for every page (and frame) that is loaded, is an appropriate solution. Perhaps we could generate random enough data, based on synchronous DOM and/or extension APIs that are avialable and return consistent data in the background page as well as content scripts, like but possibly not only the UA.

Yea, good idea except we will need to use something in the Chrome API I suppose. If we just use navigator.userAgent or other things the website has access to they could reproduce the secret. I'm having a look through the Chrome extension APIs to see what we have access to in runtime scripts that we can use.

Also, mind mind my second point in favor of popups? ;)

Don't worry, have not forgotten about the popup approach :p I'll try it next.

comment:19 in reply to: ↑ 18 Changed 4 years ago by sebastian

Replying to greiner:

What about using chrome.storage.local instead?

I'm not sure whether it would be more appropriate to read from chrome.storage.local every time a page/frame loads. Note that chrome.storage is asynchronous as well. Also it's not guaranteed that the secret is already set when the content script runs. We would have to handle that scenario and register a chrome.storage.onChanged listener then. The idea was to use some synchronous APIs, if possible, so that we can call them from ext.getURL on demand, but see below.

Replying to kzar:

Yea, good idea except we will need to use something in the Chrome API I suppose. If we just use navigator.userAgent or other things the website has access to they could reproduce the secret.

Oh, of course, any data we could retrieve consistently through the DOM API could also be reproduced by a detection script. On the other hand, I couldn't find any extension API, available for content scripts, that would give us any data we could use here, synchronously.

It seems a solution that handles this stuff completely transparent is just not feasible at least not without a performance compromise. So we should probably rather message the background page explicitly to retrieve the secret when injecting the iframe.

But as I mentioned before, we are probably better off anyway, if we just get rid of web-accessible resources and use a popup here. As scripts detecting Adblock Plus isn't the only issue with using web-accessible resources and injecting these into web pages.

comment:20 Changed 4 years ago by kzar

OK it was fairly painful but I have a proof of concept for opening block.html as a popup under Chrome. It required more changes than I hoped but it actually works pretty well.

So I guess now is decision time, we have the following choices:

  1. Block AJAX requests to our web accessible resources. By far the simplest option but likely not 100% foolproof.
  2. Block requests to our web accessible resources that don't contain a runtime secret. Pretty foolproof but adds complexity.
  3. Remove block.html from web accessible resources and open it as a popup under Chrome. Completely foolproof as there is no longer anything web accessible but it adds more complexity again. It also means we have to maintain separate code for Safari to Chrome here.

Once we've decided the approach we want to go with I'll tidy things up and open a code review.

comment:21 Changed 4 years ago by sebastian

I'm aware that using a popup will be the most elaborate solution. However, as indicated before, we currently have a couple of other issues, besides being detectable, that can only be fixed by using a popup rather than injecting content into the page:

  • The dialog will be cropped if it doesn't fit into the viewport.
  • Page content might overlap the injected dialog (we already use the highest possible z-index but the page content might do the same).
  • The page's CSS/JS can interfere with the injected dialog (e.g. the dialog might get hidden or whatever, see #3500 for example).
  • Even if websites cannot detect our web-accessible resources anymore, they can still detect when the dialog is opened.

The problem with using a popup is that we would need to maintain a separate solution for Safari. But maybe not. I would be fine with removing that functionality on Safari for now. And let's see how many users complain. If it really hurts us we could still add it back using a separate solution. However, once we migrated to the content blocker API it wouldn't work as smooth anymore anyway.

Also note that if we only need to support the popup solution, we could also remove a fair amount of existing complexity (e.g. to inject the dialog, to make the dialog drag-able, to route messages from one frame to another, etc.).

Last edited 4 years ago by sebastian (previous) (diff)

comment:22 Changed 4 years ago by kzar

Sounds good to me, I'll get a review ready that uses the popup approach, removing the feature from the Safari extension. While at it I'll simplify the block page/scripts like you mentioned.

Last edited 4 years ago by kzar (previous) (diff)

comment:23 Changed 4 years ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:24 Changed 4 years ago by kzar

  • Description modified (diff)
  • Summary changed from Detect ABP by sending ajax request to Open "block element" dialog as popup window
  • Type changed from defect to change

comment:25 Changed 4 years ago by sebastian

  • Ready set

comment:26 Changed 4 years ago by kzar

  • Description modified (diff)
  • Review URL(s) modified (diff)

comment:27 Changed 4 years ago by kzar

  • Cc Ross added
  • Resolution set to fixed
  • Status changed from reviewing to closed

https://hg.adblockplus.org/adblockpluschrome/rev/4a56ffad1ea7

(Dev build announcement and up to date Hints for testers to follow tomorrow. This ended up as a pretty large change in the code, so will require quite a lot of testing for regressions.)

comment:28 Changed 4 years ago by kzar

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Safari-next

comment:29 Changed 4 years ago by kzar

  • Resolution fixed deleted
  • Review URL(s) modified (diff)
  • Status changed from closed to reopened

comment:30 Changed 4 years ago by kzar

  • Status changed from reopened to reviewing

comment:31 Changed 4 years ago by kzar

  • Description modified (diff)

comment:32 Changed 4 years ago by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:33 Changed 4 years ago by kzar

  • Sensitive unset

(This issue no longer needs to be confidential as the changes have landed and the development build blog post has been published.)

comment:34 Changed 4 years ago by Ross

I've been using this for the last day in Chrome (still need to check Safari) and it seems to be working well and I couldn't find any major issues with it. Block element works, context menu works, it keeps track of state across tab switches well and shows menu entries when expected.

  • From a user perspective, I found the popup dialog text/behaviour confusing:

"After closing this popup": The popup closes itself after mousing around a few elements on the page the first time you Open > Block Element. I didn't need to close it? If I then open the popup again manually, it doesn't close on its own while mousing around (but disappears on page click as expected).

"click (or right click)": Both are covered by "click" (imo) and the wording makes it sound as if right click will give me some sort of alternative behaviour, which it doesn't in this mode. (But does with context menu when not in element selection mode).

  • The Block dialog has lost context about what page it applies to

Previously, the dialog was embedded on the page, so the user knew which page it was going to apply to. Now, it's a floating, alt-tab-able popup containing just a URL/filter and buttons. It works fine if the user blocks the element right away after the popup opens. However if the user changes browser tabs or alt-tabs to other applications after opening the popup and then back again the user has no proper context as to what page the popup is going to apply to. There is only the filter to be added, which may or may not have the same domain as the site having block-element applied to it.

This can be seen by attempting to block the YouTube video on the ABP homepage then changing tabs and alt-tabbing away and back to the popup. The popup only includes a youtube URL/filter, there is no mention that this is going to apply to the ABP page.

Last edited 4 years ago by Ross (previous) (diff)

comment:35 Changed 4 years ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Implemented and working.

ABP 1.10.2.1558

Chrome 30 / 40 / 48 / Windows 7 x64
Chrome 48 / Ubuntu 14.04 x64
Chrome 45 / OS X 10.11
Opera 19 / 30 / Windows 7 x64
Opera 35 / Ubuntu 14.04 x64
Opera 30 / OS X 10.11
Safari 6 (OS X 10.8) / Safari 8 (OS X 10.10) / Safari 9 (OS X 10.11)

Note: See TracTickets for help on using tickets.