Open "block element" dialog as popup window

[theheroofvn]

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.)

Subscription links

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.

[sebastian]

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.

[sebastian]

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

[greiner]

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.

[greiner]

@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()
}

[kzar]

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.

[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?

[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.

[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.

[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.

[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.

[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.

[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.

[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.)

[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? ;)

[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.

[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.

[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.

[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.

[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.).

[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.

[kzar]

​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.)

[kzar]

​https://hg.adblockplus.org/adblockpluschrome/rev/d8ca1b897067

[kzar]

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

[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.

[Ross]

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)