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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃枍 Fix rendering of noscript fallbacks in optimized AMP pages #29846

Merged
merged 5 commits into from
Mar 18, 2021

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Aug 14, 2020

I noticed when looking at an optimized AMP page with images that the noscript fallbacks do not render when JavaScript is turned off in the browser. This doesn't occur in unoptimized AMP because the .i-amphtml-* class names are not not present in the document.

For example, in this optimized amp-img with a noscript fallback:

<amp-img src="https://blog.amp.dev/wp-content/uploads/2020/08/Asset-2-80-1024x510.jpg" alt="" class="wp-image-3600 amp-wp-enforced-sizes i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" srcset="https://blog.amp.dev/wp-content/uploads/2020/08/Asset-2-80-1024x510.jpg 1024w, https://blog.amp.dev/wp-content/uploads/2020/08/Asset-2-80-300x150.jpg 300w, https://blog.amp.dev/wp-content/uploads/2020/08/Asset-2-80-768x383.jpg 768w, https://blog.amp.dev/wp-content/uploads/2020/08/Asset-2-80-1536x766.jpg 1536w, https://blog.amp.dev/wp-content/uploads/2020/08/Asset-2-80.jpg 2000w" width="1024" height="510" layout="intrinsic" i-amphtml-layout="intrinsic">
    <i-amphtml-sizer class="i-amphtml-sizer">
        <img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;base64,PHN2ZyBoZWlnaHQ9JzUxMCcgd2lkdGg9JzEwMjQnIHhtbG5zPSdodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZycgdmVyc2lvbj0nMS4xJy8+">
    </i-amphtml-sizer>
    <noscript>
        <img src="https://blog.amp.dev/wp-content/uploads/2020/08/Asset-2-80-1024x510.jpg" alt="" class="wp-image-3600" srcset="https://blog.amp.dev/wp-content/uploads/2020/08/Asset-2-80-1024x510.jpg 1024w, https://blog.amp.dev/wp-content/uploads/2020/08/Asset-2-80-300x150.jpg 300w, https://blog.amp.dev/wp-content/uploads/2020/08/Asset-2-80-768x383.jpg 768w, https://blog.amp.dev/wp-content/uploads/2020/08/Asset-2-80-1536x766.jpg 1536w, https://blog.amp.dev/wp-content/uploads/2020/08/Asset-2-80.jpg 2000w" sizes="100vw" width="1024" height="510">
    </noscript>
</amp-img>

The noscript is hidden by this CSS rule:

[layout]:not([layout=container]):not(.i-amphtml-element)>* {
    display: none;
}

This PR excludes noscript elements from being hidden on optimized AMP pages.

Granted, the styling is broken here for the fallbacknoscript > img, but at least it is rendered.

Update: It also ensures that noscript > * fallback elements get displayed with fill layout to behave similarly to shadow DOM contents constructed with JS.

JS enabled JS disabled before PR JS disabled after PR
js-enabled js-disabled-before js-disabled-after

@westonruter
Copy link
Member Author

westonruter commented Sep 24, 2020

@alanorozco I've amended this PR to also address the problem of the noscript > * elements not behaving like the fill content as they should in optimized/transformed AMP documents.

I've put together a more detailed example comparing the same amp-img in unoptimized AMP and optimized AMP, with both JS enabled and JS disabled, and then showing how the patch is expected to fix the problem: https://amp-noscript-fallback-styling.glitch.me/

table

For another example of how the lack of this being in ampshared.css necessitated me including CSS in amp-custom, see Automattic/jetpack#17251.

@westonruter westonruter changed the title 馃枍 Prevent optimized AMP from hiding noscript fallbacks 馃枍 Fix rendering of noscript fallbacks in optimized AMP pages Sep 24, 2020
@westonruter
Copy link
Member Author

Issue also reported on the support forums: https://wordpress.org/support/topic/images-cannot-be-displayed-without-javascript-despite-noscript-tags/

@westonruter
Copy link
Member Author

This issue came up yet again on Slack. @alanorozco Could you review please?

@kristoferbaxter
Copy link
Contributor

This change causes a test failure. Investigating.

@kristoferbaxter
Copy link
Contributor

@alanorozco

This PR appears to break a set of tests related to expected errors for amp-iframe and amp-youtube. Can you take a look please?

z-index: 2;
}

noscript {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this should only be targeting noscript elements that are children of AMP elements, but at the very least it should be restricted to the body:

Suggested change
noscript {
body noscript {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue was specificity. I'm happy with whatever targets noscript directly, instead of not(noscript).

Co-authored-by: Weston Ruter <westonruter@google.com>
Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks all for finding a path forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants