Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple Themes: Use of $(window).load() clashes with ads #108

Closed
laurelfulford opened this issue Apr 25, 2018 · 16 comments
Closed

Multiple Themes: Use of $(window).load() clashes with ads #108

laurelfulford opened this issue Apr 25, 2018 · 16 comments
Assignees
Labels
Support [Type] Bug Something isn't working
Milestone

Comments

@laurelfulford
Copy link
Contributor

laurelfulford commented Apr 25, 2018

We've had a few reports of sites using Rowling and Rebalance not loading when ads were enabled, and Goran not loading completely (header isn't visible).

The issue looks to be related to these themes using $(window).load() to show the header, or full content, in these themes. It's not working well with something in the ads code.

Swapping to $(document).ready() seems to fix the above issue, but may have unintended consequences. So it will need to be tested with each theme carefully to make sure it's a good replacement, and if no, look for other alternatives.

More details and related tickets here: pNEWy-bRB-p2

(Edited to add) Affected themes (there may be more):

  • Rowling ✅
  • Rebalance ✅
  • Goran ✅
  • Baskerville 2 ✅
  • Textbook ✅
  • Illustratr ✅
  • Altofocus ✅
@laurelfulford laurelfulford added this to the All themes milestone Apr 25, 2018
@KokkieH
Copy link

KokkieH commented Apr 30, 2018

I commented on the P2, but in case you missed it, I've also observed the issue with Baskerville-2, Pinboard, Textbook, Illustratr, and Altofocus thus far.

@laurelfulford
Copy link
Contributor Author

laurelfulford commented Apr 30, 2018

Thanks @KokkieH! I was just coming here to add those after I realized I'd missed them from the thread 😅

I'm updating the issue description to include a list, so we don't lose track!

@akmyta
Copy link

akmyta commented Apr 30, 2018

Another reported issue: #1125244-zen
Theme: Rowling

laurelfulford added a commit that referenced this issue May 1, 2018
…ns that show hidden elements on the page, to help work around ads issue. See #108.
@laurelfulford
Copy link
Contributor Author

Patch for Rowling here: D12613-code. I've removed the part where the content is hidden in the first place, because it doesn't seem needed in this theme. I'd appreciate your thoughts on this one, @allancole, if you think this change is over-reaching!

@laurelfulford laurelfulford self-assigned this May 1, 2018
laurelfulford added a commit that referenced this issue May 1, 2018
…nt.ready, as a work-around with an issue with the ads JavaScript. See #108.
laurelfulford added a commit that referenced this issue May 1, 2018
…t.ready, as a work-around for an issue with the ads JavaScript. Also removed styles/JavaScript that hid the body tag until page load was completed as it didn't entirely seem necessary in this case. See #108.
laurelfulford added a commit that referenced this issue May 1, 2018
…round for an issue with the ads JavaScript. See #108.
@laurelfulford
Copy link
Contributor Author

Patch for Baskerville 2 here: D12659-code.

@laurelfulford
Copy link
Contributor Author

Patch for Goran here: D12660-code

@laurelfulford
Copy link
Contributor Author

Edited the main post to remove Pinboard since it's a premium theme; moved here: 2358-wpcom-premium-themes

@laurelfulford
Copy link
Contributor Author

For Illustratr's fix (#114), it addresses the missing content issue, but there is other JavaScript (like the bit to make images wider than the content) that doesn't seem to work the same with $(document).ready(). I've left them as is for now since the pressing problem was the projects not displaying; we can circle back as need be.

@laurelfulford
Copy link
Contributor Author

I'm going to close this because all of the listed themes have been patched. Will follow up in the related P2 threads; I've also added a comment to #1125244-zen.

@gpmw
Copy link

gpmw commented May 10, 2018

Reporting another instance of this here with the Oxygen theme.
#1135929-zen

@gpmw gpmw reopened this May 10, 2018
@laurelfulford
Copy link
Contributor Author

Thanks for the report, @gpmw!

This sounds like it is related to the ads somehow, but doesn't seem to be theme-specific. I can recreate it using another theme (Independent Publisher 2), and the issue seems to be related to the Gallery slideshow carousel, not anything specific in the themes.

I think this should be reported in the Jetpack repo, with all the steps to recreate: https://github.com/automattic/jetpack

The endless page load that's mentioned in the ticket also can't be fixed on the theme-level -- that's something happening on the ads side, and can still happen with the themes I updated here.

I also should have noted when I closed out this ticket -- any new reports of issues with themes and ads should go in their own individual tickets. I'd grouped the original set into one, but new reports will be harder to spot if they're tacked on here :)

@laurelfulford
Copy link
Contributor Author

There was already an existing issue -- moved here: Automattic/jetpack#6244

@gpmw
Copy link

gpmw commented May 10, 2018

Thanks!

crunnells added a commit that referenced this issue May 22, 2018
Found a couple of instances of (window).load() that weren't working with ads, so replaced with (document).ready()
@rclations
Copy link

Is it possible to patch Cubic as well?

User report: C03TY6J1A/p1539719524000100-slack-livechat

@rclations rclations reopened this Oct 16, 2018
@kathleenjross
Copy link

The user followed up 7335989-hc.

@laurelfulford
Copy link
Contributor Author

Thanks for the report @rclations and @kathleenjross!

I've moved this to #289 for visibility.

Just to reiterate, because these kinds of comments get lost very easily in longer tickets like this:

Any new reports of issues with themes and ads should go in their own individual tickets. I'd grouped the original set into one, but new reports will be harder to spot if they're tacked on here, and it's hard know who's following up with what! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Support [Type] Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants