Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from Apr 8, 2022
Merged

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

merged 2 commits into from Apr 8, 2022

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Apr 7, 2022 •
edited

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 ampproject/amp-wp#7036 to fix this, but still the presence of this attribute should not cause the optimizer transformer to throw a fatal error.

@westonruter westonruter added Bug Something isn't working SSR Related to the serverside rendering of the Optimizer Optimizer labels Apr 7, 2022
@westonruter westonruter marked this pull request as ready for review April 7, 2022 22:35
@westonruter westonruter mentioned this pull request Apr 7, 2022
Merged
@westonruter westonruter changed the title Fix hero image optimization when noscript > img fallbacks are present Harden hero image optimization when noscript > img fallbacks are present Apr 7, 2022
@schlessera schlessera added this to the 0.11.1 milestone Apr 8, 2022
@schlessera schlessera merged commit f6c3daf into main Apr 8, 2022
@schlessera schlessera deleted the fix/find-hero-images branch April 8, 2022 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schlessera schlessera schlessera approved these changes

Assignees
No one assigned
Labels
Bug Something isn't working Optimizer SSR Related to the serverside rendering of the Optimizer
Projects
None yet
Milestone
0.11.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@westonruter @schlessera