Making WordPress.org

Opened 6 years ago

Closed 4 years ago

#3752 closed defect (bug) (worksforme)

Sponsor logos in the Sponsors widget get removed by ad blockers

Reported by: coreymckrill's profile coreymckrill Owned by:
Milestone: Priority: low
Component: WordCamp Site & Plugins Keywords: good-first-bug has-patch
Cc:

Description

Some browser addon-based ad blockers, such as uBlock Origin, have a rule that hides elements with the .sponsor-logo class. The Sponsors widget in the wc-post-types plugin uses this class when displaying a list or grid of sponsors, which can result in an empty-looking widget. Here are a couple of examples:

https://cl.ly/0h0Y0m1V1Q07

https://cl.ly/103S0a2E1b1d

The tricky part is, a lot of existing camps already have styles in their stylesheet for .sponsor-logo and other sponsor related elements, so changing the class name would potentially break the layout for a lot of WordCamp sites. Also, other elements that are rendered by the Sponsors widget and also in the [sponsors] shortcode use classes that begin with the sponsor- prefix, so even if sponsor-logo is addressed, there could be problems with future ad blocking rules.

This will probably require conditionally changing all instances of the prefix, and setting a feature flag and using wcorg_skip_feature() to determine whether to use the old prefix or the new one.

Attachments (1)

3752.diff (2.3 KB) - added by imath 5 years ago.

Download all attachments as: .zip

Change History (9)

This ticket was mentioned in Slack in #meta-wordcamp by coreymckrill. View the logs.


6 years ago

#2 @iandunn
6 years ago

Here's some previous conversations, for background:

The TL;DR is that we tried to get added as an exception for AdBlock, but don't meet their criteria. IMO, the sponsor logos are ads, so it makes sense that they'd be blocked.

Here's a longer quote from one of the above conversations with more details:

hmmm, i'm not sure we should necessarily change it. it _is_ a sponsor logo, and it _is_ an advertisement, so changing the class name to avoid it being blocked would really be trying to subvert adblock (and by extension, the user's wishes), rather than correcting a mistake that was causing a false positive.

there's also the issue of backwards compatibility; we can't really change the class, since there will be lots of camps with custom css targeting it, and a change would break those customizations. (technically, we could do it if we also updated all those sites, but i think that'd be way more trouble than it's worth).

and even if we did change it, there's nothing to stop them from blocking the new name in the future. it's not likely that they'd target wordcamps specifically, but whatever we choose would have to be semantically appropriate, so it'd probably have "sponsor" in there somewhere.

what it really comes down to is the user is choosing to run adblock, and there are dozens of other extensions they could run to change how they see the site, that's just the nature of the web.

since it's more of a philosophical decision than a technical one, though, ultimately it's not really up to me. the community team meeting tomorrow in #community-team would be the best place to get an official decision

#3 @iandunn
6 years ago

Ah, I should have finished reading before commenting :)

Using a feature flag to change the class is a good idea for the technical side of things. I'm still kind of leery of the change from a philosophical point of view, but we can always discuss it w/ the team and see what everyone thinks.

#4 @dd32
6 years ago

I think this paragraph from the above message is super relevant:

and even if we did change it, there's nothing to stop them from blocking the new name in the future. it's not likely that they'd target wordcamps specifically, but whatever we choose would have to be semantically appropriate, so it'd probably have "sponsor" in there somewhere.

I would expect to see sponsor logos blocked again in the future, WordCamps are not special in the scheme of the web, it'll just be a game of whack-a-mole where if CSS classes are changed it'll get blocked again at some point in the future (maybe tomorrow, maybe next year, etc) and the conversation will repeat.

I've seen ad blockers block ticket pages for conferences before (not WordCamps) because one of the field options contained '-ad-' as part of the field ID.

@imath
5 years ago

#5 @imath
5 years ago

  • Keywords has-patch added; needs-patch removed

Hi @coreymckrill & @iandunn

As we discussed on Slack, 3752.diff is using JavaScript to check if all Sponsor logos have been blocked by an AdBlocker. If so, it displays a message to the user to try to have him allow the blocked logos.

#6 @iandunn
5 years ago

Thanks! The patch looks pretty good at first glance. A few possible improvements:

  • We probably also want to target the [sponsors] shortcode, which would probably be .wcorg-sponsor-description img.
  • widget.innerHTML = '<p class="message">' + sponsorWidget.message + '</p>' ; looks like it could be an XSS vector if the string weren't hardcoded; it might be better to [create a new DOM node programatically](https://vip.wordpress.com/documentation/vip-go/vip-code-review/javascript-security-best-practices/#escaping-dynamic-javascript-values), just to make it obvious that it's safe, and avoid the change of a vulnerability being introduced in the future if the string is updated to include user input.
  • We might want to make the querySelectorAll lookup only target children of . wcb_widget_sponsors to avoid false positives elsewhere on the page.
  • I think it'd be nicer to create a normal CSS rule in the .css file, and just add the corresponding class to the new DOM element, rather than setting the CSS from JS.
  • a isn't a meaningful variable name, something like logo would be more descriptive/obvious
  • <p class="message">: something more specific might be helpful to avoid conflicts, like sponsored-blocked. There may even be a conventional name that AdBlock, etc use for this purpose, to ensure that it won't also be blocked.

There's a few minor things that running phpcs will catch as well.

xref: https://wordpress.slack.com/archives/C08M59V3P/p1546728991052200

This ticket was mentioned in Slack in #meta-wordcamp by iandunn. View the logs.


4 years ago

#8 @iandunn
4 years ago

  • Resolution set to worksforme
  • Status changed from new to closed

It seems like this may not be an issue with the new block, but feel free to reopen if you're still seeing it.

It also doesn't seem like we ever resolved the discussion above about whether or not to do it, unless that happened somewhere else and wasn't linked here.

Note: See TracTickets for help on using tickets.