Closed Bug 384729 Opened 17 years ago Closed 14 years ago

Hide FlashBlock placeholder for blocked ads

Categories

(Camino Graveyard :: Annoyance Blocking, enhancement)

Product:
Camino Graveyard
Component:
Annoyance Blocking
Platform:
PowerPC
macOS
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: batwood.bugs, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

Patch for comment #9
19.29 KB, patch
Details | Diff | Splinter Review 
This is a feature request to hide the FlashBlock placeholder for ads.  As discussed on the forums, it can be awkward to see a FlashBlock placeholder disappear when clicked.  The ad blocking mechanism is hiding the Flash ad, but not the placeholder.

http://forums.mozillazine.org/viewtopic.php?p=2801735

There are a couple of solutions, one being to change the FlashBlock code to recognize when an <embed> or <object> tag is hidden by the ad block code and then disable the placeholder.  But I'd rather not have the FlashBlock team change their code just to accommodate Camino, mostly because any time we make a change, we'll have to make sure they change their code as well.

So I think we should change our ad blocking CSS style to hide the Flashblock placeholder.  Since we aren't sure the order that a placeholder gets created and an ad gets hidden (at least I think we aren't sure), we'll need to modify ad_blocking.css to hide the placeholder whether the ad is visible or not (another reason not to modify the flashblock code since it may run before the ad is actually hidden).

For the most common Flash ads that I've seen, there is a tag structure like:
 <object>
  <embed src="some ad URL">
  </embed>
 </object>

Camino hides the <embed> tag, but usually the FlashBlock placeholder replaces the <object> tag with a <div> tag.  Argh, that means we can't use sibling selectors in the CSS.  Luckily, in this case the FlashBlock <div> tag will have an attribute embedsrc="some ad URL".  We can then add tags such as this to the ad_blocking.css code:

div[embedsrc*="/ad."]

In other words, copy the embed entries in ad_blocking.css and search-and-replace:
 :s/^embed.*\[src/div[embedsrc/

This hides the FlashBlock placeholder for the ads I've tested so far.  If there are still some that aren't hidden, we can figure out why not later.  

I'm adding a patch for all of the current entries in the CSS.  In the future, anytime you add an "embed" entry to ad_blocking.css, make sure to add a corresponding "div[embedsrc]" entry.  Or maybe there is a better solution?
This patch didn't work for tomshardware.com.  The reason is that the flash ad there is just an <embed> tag with no <object> tag so the above method doesn't work.

A more robust solution is to use div[title*="..."] instead of div[embedsrc*="..."].  Flashblock tries to save the Flash URL in the title attribute no matter what the structure.  The embedsrc attribute is only used to hold the URL of the <embed> tag if it's inside an <object> tag.

I'll upload the new patch, but please let me know if this doesn't work for some ads.
Attachment #268638 - Attachment is obsolete: true
Version 1 of the patch had a problem on http://www.spiegel.de/international/ (placeholder still visible, although the ads are blocked). That was the only site where this technique failed - so far. Probably for the same reason as in comment 1: only an embed is used.
Patch v2 doesn't have that problem. Testing further...
Simon, Stuart, any comments on this approach?
Comment on attachment 268745 [details] [diff] [review]
Updates patch to use 'title' attribute of placeholder instead of 'embedsrc'

r=ardissone on this.  I've been running with this locally for a while and haven't seen any side effects.

I tried to get some of those IMDB Flash ads that we block with IDs instead of src, but they aren't running any this month (or not on the listings I clicked through).

Since neither Simon nor Stuart have voiced any objections so far, going for sr here.
Attachment #268745 - Flags: superreview?(sfraser_bugs)
Attachment #268745 - Flags: review+
Hmm, I worry when our ad blocking CSS gets a whole boatload of new rules that it's going to start hurting pageload performance. This also forces us to maintain two sets of parallel style selectors.

Since hiding <object> and <embed> tags via CSS isn't really specific to ad blocking, should FlashBlock work around this problem itself?
I agree that parallel styles isn't the cleanest (but at least it's all contained in one place).  Ideally, Flashblock could detect if the ad is visible, then only create the placeholder if it is.  Is there any way to make sure that the adblock CSS runs before the Flashblock CSS+jscript?

For reference, the flashblock css looks like:
...
object[src*=".swf"]
{ -moz-binding: url("chrome://flashblock/content/flashblock.xml#flash") !important; }

Another way is to edit our own version of flashblock.css to add the adblock styles to the flashblock stylesheet.  This means 1) we can't just copy the css file from the flashblock team and 2) we need to update it anytime the adblock css changes.
Philip, can you comment on the second part of comment 5 from a Flashblock perspective (iirc, you were the one who suggested we take the approach outlined here)?

I had the same concerns as Simon initially, but I haven't really noticed noticeable pageload degradation.  But I haven't run Tp, either.

(In reply to comment #6)
> Is there any way to make sure that the adblock CSS runs before the Flashblock 
> CSS+jscript?

This didn't use to be the case (e.g., Gecko 1.8.0); I don't know if anything changed here for Gecko 1.8.1.
You can't hide /all/ flash objects/embeds as not all are advertisements and some are content e.g. youtube and cnn and there are people who want to be able to unblock content flash while still blocking the surrounding flash advertisements. Thus you would still need something clickable and visible in these cases.

As for the flashblock css. One approach is to create a stylesheet e.g. caminoBlock.css that only imports from both the flashblock stylesheet and your specific adblock styles. This means you load caminoBlock.css instead of flashblock.css directly and thus use the unchanged flashblock stylesheet.
Thank you for the comments Philip.  I also have another solution that avoids changes to flashblock, avoids duplicate tags in Camino's adblock system and gets around two stylesheets modifying the same element.

If flashblock is not enabled, let Camino's adblock hide flash ads.  When flashblock is enabled, disable Camino's flash ad removal and let Flashblock replace flash ads.  So when flashblock and adblock are both enabled, the flash ads will have the placemarks but when clicked will show the ad.  But this should be fine as the ad is initially hidden at least by the placemark.

To do this, we need to separate the flash ad styles from ad_blocking.css into a new file flash_ad_blocking.css and a few lines of code to toggle the flash ad css depending on the state of ad/flash block.  Going forward, new flash tags go in one css, all other ad tags go in the other.

I have a patch for this ready if you agree that this is worth looking at.  Hopefully we can close this one out for good soon.
That sounds like a reasonable solution.
Not sure what the etiquette is for a parallel solution, but I went ahead and obsoleted the other patch since this one seems more likely to make the cut.  btw, when testing don't forget to add flash_ad_blocking.css to the project :)
Attachment #268745 - Attachment is obsolete: true
Attachment #277929 - Flags: review?(alqahira)
Attachment #268745 - Flags: superreview?(sfraser_bugs)
From the point-of-view of someone who doesn't want to see Flash ads, this second solution seems less satisfying.

As things work now, if I have both blockers on, all I ever see is the placeholder div; if I click on it (to find out what it is), poof.  I don't see an ad.

With the proposed patch, if I'm understanding correctly, if I have both blockers on, I'll see the placeholder div, and if I click on it (to find out what it is), I'm now seeing an evil ad.

To me, the proposed solution is worse than the current bug, and I'd rather live with the bug than see ads we're capable of blocking.

I'm also not fond of maintaining two files to do ad-blocking, but that's a secondary issue.
I was about to comment the same as Smokey.
I thought the purpose of this bug was to hide the flashblock placeholder when both blockers are running, and the flash thing is a known (blocked) ad.
That is what the first patch was doing quite well. The proposed second patch doesn't do that, as far as I understand it.

-----
I've been using the first patch for months now, without noticing issues with page loading / page rendering speed. Granted, nothing scientific, just perception.
Given that one of the most popular requests that we get from our contacts page is to make Flashblock work with AdBlock[Plus] so that the user doesn't even see our placeholders for AdBlocked advertisements, I have to agree with Smokey and philippe. Just chipping in with some real world observations from our user base.
Response to the second patch seems universal, so let's go with another option.

Philip, good to know about AdBlock/Plus.  Do you know how it works?  Camino hides the <embed> or <object> tag of flash ads by setting the style to display:none. I also have a patch that modifies Flashblock instead so that it only shows a placeholder if the flash elements are visible.  If this worked for AdBlock too, it might be worth including in Flashblock (and we wouldn't have to touch Camino).  If you don't mind, I'll follow up by email to go over the details.
Comment on attachment 277929 [details] [diff] [review]
Patch for comment #9

Clearing the review request for now, per discussion in comment 15.  If that option doesn't work out, we can revisit this (but since this patch is code rather than CSS, I'm not the right reviewer, anyway).
Attachment #277929 - Flags: review?(alqahira)
Philip, is comment 15 something that Bryan ever discussed further with you? Is his idea of patching Flashblock to show placeholders only when the element is visible a viable one? If so, let's just do that and call it good (unless there's a compelling reason to do something else).
I don't remember Bryan ever showing me the patch he mentioned but it should be relatively simple to test for the element hight/width and if either is 0 then we don't show a placeholder.

However in recent times I've seen an increase in web 2.0 sites that use a hidden "Flash Controller" object that interacts with the rest of the webpage via javascript (I've seen one being used as a browser independent XMLHTTPRequest proxy for example). If you can't click on that element to activate it, then that webpage may never load completely since much of that page is dynamically generated via data fetched by the controller.
I looked for a patch, but it must have gotten lost when my hard drive failed early this year.

I think the method I used was to test whether it was visible by doing something like:

    var style = window.getComputedStyle(obj, "")
    if (style.display == 'none') return false
    if (style.visibility == 'hidden') return false

(or maybe it should be style.getPropertyValue("display")?)

Then only add a FlashBlock placeholder if it's showing.  This gets around the problem with "zero size" windows that Philip mentions.
> Philip, good to know about AdBlock/Plus.  Do you know how it works?  Camino
> hides the <embed> or <object> tag of flash ads by setting the style to
> display:none.

Is this done via CSS? If so, you could also try adding { -moz-binding: none !important; } to prevent flashblock from processing these particular elements.

Adblock/Plus can't use this technique because it kicks in after flashblock to walk the DOM tree (ABP prevents the flash from actually loading much earlier using content policies but that leaves the object still taking up blank space in the viewport).
(In reply to comment #20)
> > Philip, good to know about AdBlock/Plus.  Do you know how it works?  Camino
> > hides the <embed> or <object> tag of flash ads by setting the style to
> > display:none.
> 
> Is this done via CSS? If so, you could also try adding { -moz-binding: none
> !important; } to prevent flashblock from processing these particular elements.

Camino's adblocker is currently all CSS
of the type
 object[src*="evil-ad"] {display:none !important;}
So you're saying that adding ' -moz-binding: none !important;' would prevent Flashblock to process those elements ?

Hmm, me goes testing that...
I'd have to check the precedence rules for CSS but as you know, Bob, !important trunps more specific rules which trump less specific.

I've just checked and we (flashblock.css) uses !important. Try removing !important from flashblock.css first.
So, after testing this extensively for a while: adding '-moz-binding: none !important;' in ad_blocking.css is certainly an improvement. For many swf ads, the flashblock placeholder is not called/shown.

Some sites still show the flashblock overlay: seen (randomly) on
yahoo.co.jp/
finance.yahoo.com

randomly as in: for the same source of swf ads, the placeholder is sometimes triggered, sometimes not.

the flashblock placeholder always shows up:
elpais.com/global/
nytimes.com/
lemonde.fr/

The given uri are a sample, I didn't take note of all sites. I'm not sure what those swf ads have in common, probably some sort of js-powered injection

So, it is a bit of a mixed bag, but an improvement.

fwiw, removing the !important in flashblock.css didn't make a difference.
I'd like to see about doing some more testing with '-moz-binding: none' with a view to perhaps taking that as an improvement after b3.
Fwiw, I've had the '-moz-binding: none !important' in my ad_blocking.css ever since comment 23 without any additional issue to note. Sometimes the trick fails as noted. The overlay is still there, clicking on it collapses and hides the ads.

(elpais and lemonde are sites that I actually visit regularly and I've put some additional protection on my side - at permissions.sqlite level)
This should be fixed by bug 549250, which adds '-moz-binding: none !important' to ad_blocking.css.
Assignee: batwood.bugs → nobody
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: