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

dedup_inlined_images breaks site combined with lazyload_images and defer_javascript. #850

Closed
GoogleCodeExporter opened this Issue Apr 6, 2015 · 2 comments

Comments

Projects
None yet
1 participant
@GoogleCodeExporter
Copy link

GoogleCodeExporter commented Apr 6, 2015

Turn on lazyload_images, dedup_inlined_images, and defer_javascript.

Assume the image gets shrunk enough to inline it, the data-url appears to be 
mapped into the script tag, rather than the img tag, blanking the image.  
Testcase coming.

Original issue reported on code.google.com by jmara...@google.com on 9 Dec 2013 at 2:27

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

I'm having difficulty reproducing this in a small testcase, and even on the 
site that reported the problem originally.  That site had (at my suggestion) 
turned off dedup_inlined_images in their configuration, and enabling it via 
query-parameters didn't make the problem come back, though I saw the problem 
myself before.

This is my attempt at a repro:
<!-- Image reference 1 -->
<img src="/mod_pagespeed_example/images/disclosure_arrow_dk_grey.png"></img>

<!-- Image reference 2 -->
<img src="/mod_pagespeed_example/images/disclosure_arrow_dk_grey.png"></img>


With configuration set to:
  ModPagespeedEnableFilters dedup_inlined_images
  ModPagespeedEnableFilters defer_javascript
  ModPagespeedEnableFilters lazyload_images
  ModPagespeedEnableFilters convert_jpeg_to_webp
  ModPagespeedEnableFilters rewrite_images,convert_jpeg_to_webp
  ModPagespeedJpegRecompressionQuality 50

I think the bug is in the dedup_inlined_images helper script:
pagespeed.DedupInlinedImages.prototype.inlineImg = function(
    img_id, script_id) {
  // Find the img to copy from, identified by the given img_id.
  var srcNode = document.getElementById(img_id);
  if (!srcNode) {
    // console.log('PSA ERROR: img_id="' + img_id + '": no img');
    return;
  }
  // Find the invoking script.
  var scriptNode = document.getElementById(script_id);
  if (!scriptNode) {
    // console.log('PSA ERROR: script_id="' + script_id + '": no script');
    return;
  }
  // Find the img to copy to, being the element before the invoking script.
  var dstNode = scriptNode.previousSibling;
  if (!dstNode) {
    // console.log('PSA ERROR: img_id="' + img_id + '": no sibling');
    return;
  }
  // Copy the src attribute then delete this script.
  dstNode.src = srcNode.getAttribute('src');
  scriptNode.parentNode.removeChild(scriptNode);
};



Based on my observations in the Chrome Dev Tools, I think that the assumption 
that we should be writing the image data to the previous sibling of the script 
is disturbed by some other filter.  It would be better to identify that image 
by ID.  But a repro testcase would be ideal.  I'll try to tweak that a bit more.

Original comment by jmara...@google.com on 9 Dec 2013 at 3:12

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Original comment by matterb...@google.com on 4 Jun 2014 at 9:10

  • Changed state: Fixed
  • Added labels: Milestone-v32, release-note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment