search

ampproject / amp-toolbox-php

AMP Optimizer PHP library
Apache License 2.0
74 stars 23 forks source link

Harden hero image optimization when `noscript > img` fallbacks are present #516

Closed westonruter closed 2 years ago

westonruter commented 2 years ago

This exposes an issue that arose with the AMP plugin v2.2.2 release. In this release, there was an update to the validator spec which included, for the first time, data-hero as being a recognized attribute on img elements. Because of this, when the AMP_Noscript_Fallback::initialize_noscript_allowed_attributes() method ran in AMP_Img_Sanitzer it included data-hero among the attributes that are copied to the to the noscript > img fallback. Resulting in this change from v2.2.1 (where the data-hero attribute is now present):

- <amp-img data-hero width="500" height="400" src="/img1.png"><noscript><img width="500" height="400" src="/img1.png"></noscript></amp-img>
+ <amp-img data-hero width="500" height="400" src="/img1.png"><noscript><img width="500" height="400" src="/img1.png" data-hero></noscript></amp-img>

Now that the noscript > img fallback has data-hero, the \AmpProject\Optimizer\Transformer\OptimizeHeroImages::findHeroImages() method in this repo incorrectly starts including both the amp-img and the noscript > img in the list of hero images to optimize. This later causes a fatal error when the removeLazyLoading method attempts to remove any loading attribute from the noscript > img because the element was removed from the DOM by the \AmpProject\Optimizer\Transformer\OptimizeHeroImages::generateImg() method. The result is a fatal error:

Couldn't fetch AmpProject\Dom\Element (0) [Error]
/app/public/wp-content/plugins/amp/vendor/ampproject/amp-toolbox/src/Optimizer/Transformer/OptimizeHeroImages.php:474

#0 /app/public/wp-content/plugins/amp/vendor/ampproject/amp-toolbox/src/Optimizer/Transformer/OptimizeHeroImages.php(474): DOMElement->getAttribute('loading')
#1 /app/public/wp-content/plugins/amp/vendor/ampproject/amp-toolbox/src/Optimizer/Transformer/OptimizeHeroImages.php(183): AmpProject\Optimizer\Transformer\OptimizeHeroImages->removeLazyLoading(Object(AmpProject\Optimizer\HeroImage))
#2 /app/public/wp-content/plugins/amp/vendor/ampproject/amp-toolbox/src/Optimizer/TransformationEngine.php(78): AmpProject\Optimizer\Transformer\OptimizeHeroImages->transform(Object(AmpProject\Dom\Document), Object(AmpProject\Optimizer\ErrorCollection))
#3 /app/public/wp-content/plugins/amp/src/Optimizer/OptimizerService.php(46): AmpProject\Optimizer\TransformationEngine->optimizeDom(Object(AmpProject\Dom\Document), Object(AmpProject\Optimizer\ErrorCollection))
#4 /app/public/wp-content/plugins/amp/includes/class-amp-theme-support.php(2131): AmpProject\AmpWP\Optimizer\OptimizerService->optimizeDom(Object(AmpProject\Dom\Document), Object(AmpProject\Optimizer\ErrorCollection))
#5 /app/public/wp-content/plugins/amp/includes/class-amp-theme-support.php(1750): AMP_Theme_Support::prepare_response('<!DOCTYPE html>...')
#6 [internal function]: AMP_Theme_Support::finish_output_buffering('<!DOCTYPE html>...', 9)
#7 /app/public/wp-includes/functions.php(5219): ob_end_flush()
#8 /app/public/wp-includes/class-wp-hook.php(307): wp_ob_end_flush_all('')
#9 /app/public/wp-includes/class-wp-hook.php(331): WP_Hook->apply_filters('', Array)
#10 /app/public/wp-includes/plugin.php(474): WP_Hook->do_action(Array)
#11 /app/public/wp-includes/load.php(1100): do_action('shutdown')
#12 [internal function]: shutdown_action_hook()
#13 {main}

So the fix is just to make sure that we skip over any element from being considered a hero if it is the child of a noscript element.

See support topic replies.

Update: It also turns out that the AMP plugin shouldn't have been including the data-hero attribute on the amp-img > noscript > img fallbacks in the first place, as that is a validation error. So I've opened https://github.com/ampproject/amp-wp/pull/7036 to fix this, but still the presence of this attribute should not cause the optimizer transformer to throw a fatal error.