Promote noscript fallback images with fetchpriority=high to hero and automatically SSR
#544
Conversation
| @@ -555,6 +555,7 @@ private function generateImg(HeroImage $heroImage, Document $document) | |||
| $imgElement = $document->createElement(Tag::IMG); | |||
| $imgElement->setAttribute(Attribute::CLASS_, self::SSR_IMAGE_CLASS); | |||
| $imgElement->setAttribute(Attribute::DECODING, 'async'); | |||
| $imgElement->setAttribute(Attribute::FETCHPRIORITY, 'high'); | |||
There was a problem hiding this comment.
Thinking about this some more now... perhaps we actually shouldn't automatically add fetchpriority=high to all hero images, since there can be multiple. The fetchpriority=high attribute should be reserved for the LCP image. So I think instead of automatically adding it to all heros, we should instead only it when the amp-img > noscript > img contains fetchpriority=high.
Additionally, we should automatically SSR any amp-img that has a noscript > img with fetchpriority=high.
But if an amp-img has data-hero alone, that wouldn't necessarily warrant adding fetchpriority=high.
There was a problem hiding this comment.
Thinking about this some more now... perhaps we actually shouldn't automatically add fetchpriority=high to all hero images, since there can be multiple.
Completely agree here and I have updated it per suggestion.
thelovekesh
force-pushed
the
feature/fetchpriority-on-hero-images
branch
from
e757cc1
to
3852f92
Compare
…eroImages::generateImg()`
| if ($node->tagName === Extension::IMG && ! $node->hasAttribute(Attribute::DATA_HERO)) { | ||
| $highFetchpriorityImage = $document->xpath->query( | ||
| self::NOSCRIPT_IMG_FETCHPRIORITY_HIGH_XPATH_QUERY, | ||
| $node | ||
| )->item(0); | ||
|
|
||
| if ($highFetchpriorityImage instanceof Element) { | ||
| $node->setAttribute(Attribute::DATA_HERO, ''); | ||
| } | ||
| } |
There was a problem hiding this comment.
With this logic in place, we won't need to add data-hero to images with high fetch priority in AMP Image sanitizer.
| 'preserves fetchpriority attribute from noscript fallback img' => [ | ||
| $input( | ||
| '<amp-img width="500" height="400" src="/img1.png"><noscript><img src="/img1.png" width="500" height="400" fetchpriority="high"></noscript></amp-img>' | ||
| ), | ||
| $output( | ||
| '<amp-img data-hero width="500" height="400" src="/img1.png" i-amphtml-ssr><img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" fetchpriority="high" src="/img1.png"></amp-img>' | ||
| ), | ||
| ], | ||
|
|
There was a problem hiding this comment.
Am I right that this test is somewhat redundant with the second?
There was a problem hiding this comment.
Updated the test cases in 16b9f66
| '<amp-img width="500" height="400" src="/img1.png"><noscript><img src="/img1.png" width="500" height="400" fetchpriority="high"></noscript></amp-img>' | ||
| . '<amp-img width="500" height="400" src="/img2.png"><noscript><img src="/img2.png" width="500" height="400"></noscript></amp-img>' | ||
| . '<amp-img width="500" height="400" src="/img3.png"><noscript><img src="/img3.png" width="500" height="400" fetchpriority="high"></noscript></amp-img>' |
There was a problem hiding this comment.
For the second image, how about adding data-hero to it to see what happens when preceded by a fetchpriority=high image, and then the third image can have neither data-hero nor fetchpriority (as really the source markup shouldn't have multiple fetchpriority=high.
Co-authored-by: Weston Ruter <westonruter@google.com>
westonruter
changed the title
fetchpriority=high on hero imagesfetchpriority=high to hero and automatically SSR
See ampproject/amp-wp#7601