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

Critical images js doesn't play well with `display` #1087

Closed
jeffkaufman opened this Issue May 28, 2015 · 4 comments

Comments

Projects
None yet
3 participants
@jeffkaufman
Copy link
Contributor

jeffkaufman commented May 28, 2015

On ngx-pagespeed-discuss someone said their page was intermittently getting js plastered across it on mobile. Specifically, PageSpeed was turning:

<a class="logo" href="http://lebonstream.net" title="Lebonstream | Un site utilisant WordPress"><img src="http://lebonstream.net/wp-content/uploads/2015/05/logo-header.png" alt="Lebonstream | Un site utilisant WordPress"/></a>

into:

<a class="logo" href="http://lebonstream.net" title="Lebonstream | Un site utilisant WordPress"><script pagespeed_no_defer="">(function(){var g=this,h=function(b,d)...pagespeed.CriticalImages.Run('/ngx_pagespeed_beacon','http://lebonstream.net/?a=b','bOk8u7_prl',true,false,'-tCPhA9VMdE');</script><img src="http://lebonstream.net/wp-content/uploads/2015/05/xlogo-header.png.pagespeed.ic.m4z1_NscpC.png" alt="Lebonstream | Un site utilisant WordPress" pagespeed_url_hash="1901629196" onload="pagespeed.CriticalImages.checkImageForCriticality(this);"/></a>

Which looked like:

screenshot_2015-05-28-07-57-29

Here's a static copt with the same problem: http://www.jefftk.com/repro-bad-js-issue2?PageSpeed=off

Here's a simplified page with the same problem: http://www.jefftk.com/repro-bad-js-issue3?PageSpeed=off

The basic issue is that we're inserting JS inside a tag that they've set all the children of to display: inline-block.

A fix is to put display:none on the js we insert: http://www.jefftk.com/repro-bad-js-issue4?PageSpeed=off

$ diff repro-bad-js-issue{3,4}.html
7c7
< <script pagespeed_no_defer="">

---
> <script style="display:none" pagespeed_no_defer="">

Another fix would be to insert the JS with base64 src= inlining instead (haven't tried this).

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented May 28, 2015

I think this was reported before, but I don't see the bug looking now.

@jmaessen

This comment has been minimized.

Copy link
Contributor

jmaessen commented May 28, 2015

I have a CL sitting around to fix this that needs tests written. My
proposed solution is to not inject the script into the middle of the page
at all, but to put it in the head. It means we'll occasionally inject it
when we don't need to, but that should basically never happen in practice.

Misplaced the bug number, which I used to have open in a tab. Grrr... No
previous solution has suggested adding a style= to the script, and I'm
pretty sure that's Just Wrong (it offends my delicate sensibilities,
anyway).

-Jan

On Thu, May 28, 2015 at 9:25 AM, Jeff Kaufman notifications@github.com
wrote:

I think this was reported before, but I don't see the bug looking now.


Reply to this email directly or view it on GitHub
#1087 (comment)
.

@jmaessen jmaessen self-assigned this May 28, 2015

@jmaessen

This comment has been minimized.

Copy link
Contributor

jmaessen commented Jun 1, 2015

This should be fixed; I've moved the script load to or to top of page if there's no head.

@crowell

This comment has been minimized.

Copy link
Contributor

crowell commented Jun 10, 2015

closed by 0184daf

@crowell crowell closed this Jun 10, 2015

@jeffkaufman jeffkaufman changed the title critical images js doesn't play well with `display` Critical images js doesn't play well with `display` Jul 27, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment