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 | 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:
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)
Change History (9)
This ticket was mentioned in Slack in #meta-wordcamp by coreymckrill. View the logs.
6 years ago
#3
@
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
@
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.
#5
@
6 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
@
6 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 likelogo
would be more descriptive/obvious<p class="message">
: something more specific might be helpful to avoid conflicts, likesponsored-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
@
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.
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: