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

Provide a way to optimize all resource attributes in a tag #466

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

Comments

Projects
None yet
1 participant
@GoogleCodeExporter
Copy link

GoogleCodeExporter commented Apr 6, 2015

The following use-case outlines the use of multiple resource attributes in a 
single '<img>' tag.
The attributes are 'src' and 'data-loadsrc'.  Today, mod_pagespeed seems to 
grab the first one and doesn't have a way to understand that it should pay 
attention to the second resource.  

THe ModPagespeedUrlValuedAttribute does give you the kind of configurations you 
would need to understand that so that you wouldn't have to pay the cost of 
looking ahead once you've found the first resource attributre. E.g. 
'ModPagespeedUrlValuedAttribute img data-loadsrc Image' 

<img src="foo.jpg" title="Foo" width="950" height="300" 
class="slide-on-viewport" 
data-loadsrc="/cms_assets/images/photos/promo/foo1.jpg"/> <div 
class="foo"></div>

<img 
src="data:image/gif;base64,R0lGODlhAQABAJH/AP///wAAAMDAwAAAACH5BAEAAAIALAAAAAABA
AEAAAICVAEAOw==" title="Foo" width="950" height="300" class="slide-on-viewport" 
data-loadsrc="/cms_assets/images/photos/promo/foo1.jpg"/> <div 
class="foo"></div>

Original issue reported on code.google.com by hayes...@gmail.com on 14 Jul 2012 at 8:55

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Original comment by jmara...@google.com on 16 Jul 2012 at 4:37

  • Changed state: Accepted
@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Any timing on this issue, we are also waiting for a fix. 

Original comment by thomas.feldhaus on 10 Apr 2013 at 9:29

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

@thomas.feldhaus: could you give an example of what you're doing that needs 
this?

Original comment by jefftk@google.com on 10 Apr 2013 at 11:24

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

We use this to deliver different image-sizes (mobile, desktop/pad, retina). We 
thought this is already possible with „ModPagespeedUrlValuedAttribute“. 

http://www.jvm.com/de/cases/index.html

<img class="case_teaser" 
src="/media/_uploads/contents/cases/rwe_riese_film,7E1_TEASER.jpg.pagespeed.ce._
tXFa5NvmU.jpg" 
data-medium-src="../../media/_uploads/contents/cases/rwe_riese_film~1_MEDIUM.jpg
" 
data-retina-src="../../media/_uploads/contents/cases/rwe_riese_film~1_MEDIUM_RET
INA.jpg" alt="RWE Energieriese"/>

Thanks! 

Best from Hamburg!

Original comment by thomas.feldhaus on 15 Apr 2013 at 8:22

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

You're running into a limitation of ModPagespeedUrlValuedAttribute: we can only 
rewrite one attribute per element.  This is presently a limitation of how 
resource identification works, but it's definitely worth leaving this bug open 
as lifting that restriction would be nice (there are already places in the 
resource identification code where we choose from among several possible 
resources to optimize).

Original comment by jmaes...@google.com on 15 Apr 2013 at 2:43

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

hope this help page speed

Original comment by irenlow...@gmail.com on 2 Aug 2013 at 8:21

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Issue 685 has been merged into this issue.

Original comment by jefftk@google.com on 25 Sep 2013 at 4:34

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

There are many use cases where this limitation gets in your way with no viable 
workaround. With data attributes being more and more widely used, we just can't 
fully rely on PageSpeed and this is unfortunately. In one of our recent 
projects, we have landscape and portrait aspect ratio images (responsive 
design), a shortlink, used for social sharing. Of all those, only the shortlink 
gets rewritten as it happens to be the first match.

Limitations you can work around are acceptable, but limitations with no 
workarounds is something that should be prioritized.

Original comment by nikolaynkolev on 25 Sep 2013 at 4:56

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Specifically, ScanElement() in resource_tag_scanner.cc returns (and is expected 
to return) only one url-valued-attribute for an element.  To fix this 
ScanElement should probably return a vector<HtmlElement::Attribute*> and the 18 
places that call ScanElement need to start looping over all returned attributes.

Original comment by jefftk@google.com on 25 Sep 2013 at 5:22

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

It seems to be a pretty significant work effort, but one that's well worth it!

Original comment by nikolaynkolev on 25 Sep 2013 at 8:17

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Please give this issue a chance! We definitely need to optimize not 1 
attribute, but more. If there can be done a quick fix - to find a custom link, 
than standard one, this also helps (i.e. it's much more important to optimize 
attached resources than placeholder image).

Original comment by sp...@webo.name on 10 Oct 2013 at 7:57

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Original comment by jmara...@google.com on 10 Oct 2013 at 7:58

  • Changed state: Started
@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

This issue was closed by revision r3554.

Original comment by jefftk@google.com on 24 Oct 2013 at 9:37

  • Changed state: Fixed
@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Great job, Jeff! Can't wait to get this into my Nginx build! :)

Original comment by nikolaynkolev on 24 Oct 2013 at 10:09

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Awesome! Everybody doing responsive and/or hidpi-aware designs will thank you. 
Though I don't get the benefit as I work at another company now. I will have to 
tell the guys at the old company to roll this out.

Thanks!
Pavel

Original comment by bakhi...@google.com on 25 Oct 2013 at 12:43

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Ha! "Another company". This is what I get for not keeping track of who I'm 
logged-in as.

Original comment by bakhi...@google.com on 25 Oct 2013 at 12:45

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

> Can't wait to get this into my Nginx build!

I still need to update ngx_pagespeed's trunk-tracking to build against this 
revision of PSOL, so it's not available yet.  That will be some time in the 
next week or so if you're up for running a bleeding-edge version compiled from 
source.  Otherwise it should be in the next release.

Original comment by jefftk@google.com on 25 Oct 2013 at 1:35

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Original comment by hui...@google.com on 25 Oct 2013 at 8:23

  • Added labels: Milestone-30, release-note
@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Original comment by hui...@google.com on 25 Oct 2013 at 8:29

  • Added labels: Milestone-v30
  • Removed labels: Milestone-30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment