Skip to content

🖍 Fix rendering of noscript fallbacks in optimized AMP pages#29846

Merged
kristoferbaxter merged 5 commits intoampproject:masterfrom
westonruter:patch-5
Mar 18, 2021
Merged

🖍 Fix rendering of noscript fallbacks in optimized AMP pages#29846
kristoferbaxter merged 5 commits intoampproject:masterfrom
westonruter:patch-5

Conversation

@westonruter
Copy link
Copy Markdown
Member

@westonruter westonruter commented Aug 14, 2020 •
edited
Loading

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

@google-cla google-cla bot added the cla: yes label Aug 14, 2020
@westonruter westonruter mentioned this pull request Sep 24, 2020
Merged
@westonruter westonruter force-pushed the patch-5 branch 2 times, most recently from 8201359 to fa5f23a Compare September 24, 2020 23:41
@westonruter
Copy link
Copy Markdown
Member Author

westonruter commented Sep 24, 2020 •
edited
Loading

@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 westonruter mentioned this pull request Sep 29, 2020
Merged
@amp-bundle-size amp-bundle-size bot requested a review from Jiaming-X January 29, 2021 23:37
@westonruter
Copy link
Copy Markdown
Member Author

westonruter commented Mar 15, 2021

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

@westonruter
Copy link
Copy Markdown
Member Author

westonruter commented Mar 17, 2021

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

@kristoferbaxter
Copy link
Copy Markdown
Contributor

kristoferbaxter commented Mar 17, 2021

This change causes a test failure. Investigating.

@kristoferbaxter
Copy link
Copy Markdown
Contributor

kristoferbaxter commented Mar 17, 2021

@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?

westonruter
westonruter commented Mar 18, 2021
View reviewed changes
css/ampshared.css Outdated
z-index: 2;
}

noscript {
Copy link
Copy Markdown
Member Author

@westonruter westonruter Mar 18, 2021

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
Copy Markdown
Contributor

@jridgewell jridgewell Mar 18, 2021

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).

@jridgewell @westonruter
Update css/ampshared.css
899d707 
Co-authored-by: Weston Ruter <westonruter@google.com>
kristoferbaxter
kristoferbaxter approved these changes Mar 18, 2021
View reviewed changes
Copy link
Copy Markdown
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.

@kristoferbaxter kristoferbaxter merged commit feed790 into ampproject:master Mar 18, 2021
@westonruter westonruter mentioned this pull request Mar 30, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jridgewell jridgewell jridgewell approved these changes

@alanorozco alanorozco Awaiting requested review from alanorozco

@Jiaming-X Jiaming-X Awaiting requested review from Jiaming-X

+1 more reviewer

@kristoferbaxter kristoferbaxter kristoferbaxter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes PR use: In Beta / Experimental PR use: In LTS PR use: In Stable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@westonruter @kristoferbaxter @jridgewell @ampprojectbot