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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow style in noscript #20609

Closed
aghassemi opened this issue Jan 30, 2019 · 18 comments
Closed

Allow style in noscript #20609

aghassemi opened this issue Jan 30, 2019 · 18 comments

Comments

@aghassemi
Copy link
Contributor

Summarizing conversations with @westonruter

It is common to have CSS rules to target js vs no-js. Certain components need to become hidden or presented differently in no-js case.

Currently html:not([amp-version]) is one workaround to set specific no-js CSS rules. This works because v0.js adds that attribute but it is hacky and if SSR ever decides to add that attribute, it would break the workaround.

Maybe the proper approach is allowing style in noscript tag. I don't think it needs to fit the 50k limit either.

@westonruter @Gregable @cramforce

@cramforce
Copy link
Member

I'd be OK with that, but with a low KB limit. It would only ever be a little additional styling, and a large noscript-stylesheet would weigh heavily on the default case.

@aghassemi
Copy link
Contributor Author

I thought content of noscript don't get parsed with JS enabled. My understanding is that it becomes essentially a hidden text node. Limit is good regardless.

@westonruter
Copy link
Member

I think allowing noscript > style is one solution, but it would be preferred from a WordPress context if the no-JS rules could be written in the style[amp-custom]. In other words, the html:not([amp-version]) is easier to work with. As @sebastianbenz suggests in #17832, what if there is a runtime-ready classname added to the html element?

@cramforce
Copy link
Member

I think key would be to analyze the lifecycle including optimized AMP.

The document can be

  • visible, waiting for JS
  • invisible, waiting for JS
  • visible, JS timed out
  • visible, noscript
  • visible JS loaded

Ideally, these can be distinguished and devs can target the earliest stage that works for them to avoid e.g. re-layout once JS eventually loads.

@Gregable
Copy link
Member

cc @alin04 since this may eventually intersect with the golang packager bug regarding noscript content parsing.

@jpettitt
Copy link
Contributor

FWIW this is valid html

<head>
  <noscript>
    <style> ... </style>
  <noscript>
</head>

<noscript> can also contain <link>. We could allow a link to a stylesheet and cache the sheet if we need to do so to preserve privacy (may not be needed if it's impossible to pre-render in a non JS environment anyway). Using a <link> would minimize the performance impact on the JS case at the expense of the much less common <noscript> case.

@westonruter
Copy link
Member

westonruter commented Jan 31, 2019

FWIW this is valid html

Is it?

image

I get validation errors when adding:

<noscript>
    <style>body {color:red}</style>
</noscript>

@westonruter
Copy link
Member

westonruter commented Jan 31, 2019

Sorry. You said HTML, not AMPHTML. My mistake.

@Gregable
Copy link
Member

Is there still interest in this request? I don't see a resolution to determine a final implementation plan and interest has died off, so for now I'll just close it and we can re-implement.

@westonruter
Copy link
Member

westonruter commented Aug 18, 2021

(Accidentally hit submit before I finished.)

@westonruter
Copy link
Member

westonruter commented Aug 18, 2021

Reopening since it has re-arisen as a need.

I've been watching a Gutenberg issue for an Accordion block with the thought that it could be a good fit for the amp-accordion Bento component. Three points were raised as requirements (WordPress/gutenberg#21584 (comment)):

  1. Graceful degradation when JS is not active.
  2. Keyboard accessibility.
  3. Semantic markup.

The second two points are accounted for in amp-accordion. However, it seems the first is not.

If I disable JavaScript and go to https://preview.amp.dev/documentation/components/amp-accordion-v1.0.example.1.html the result is that the first two sections cannot be accessed.

Seems like there's a need for Bento components (and classic components) to ship with noscript styles which can be used to account for this.

For example:

<noscript>
	<style>
		amp-accordion > section:not([expanded]) > :last-child {
			display: block !important;
		}
	</style>
</noscript>

See https://bento-amp-noscript-styles.glitch.me/

When JS is enabled When JS is Disabled
image image

Instead of adding noscript > style elements, an alternative would be to make use of the scripting CSS media feature which would allow for no-JS styles to be included in the same stylesheet. Unfortunately, no browser supports this currently: https://developer.mozilla.org/en-US/docs/Web/CSS/@media/scripting#browser_compatibility

So going with noscript > style, I think each component can be distributed with a *.noscript.css file alongside any *.css file.

So the boilerplate HTML the user should include (for Bento) would look like:

<!-- AMP runtime script -->
<script async src="https://cdn.ampproject.org/v0.js"></script>
<!-- Bento component stylesheet -->
<link rel="stylesheet" type="text/css" href="https://cdn.ampproject.org/v0/amp-bento-component-name-1.0.css">
<!-- :sparkles: noscript Bento component stylesheet -->
<noscript>
    <link rel="stylesheet" type="text/css" href="https://cdn.ampproject.org/v0/amp-bento-component-name-1.0-noscript.css">
</noscript>
<!-- Bento component script -->
<script async custom-element="amp-bento-component-name" src="https://cdn.ampproject.org/v0/amp-bento-component-name-1.0.js"></script>

For the AMP case, AMP Optimizer should construct this noscript > style and populate it with the no-JS stylesheets for all components on the page.

So to start with at very least we need noscript > style to be valid AMP. And once we have this, we can add *.noscript.css files for each component (where appropriate) and then the Optimizer can inline them onto the SSR'ed page.

@Gregable
Copy link
Member

So to start with at very least we need noscript > style to be valid AMP.

Allowing <noscript><style amp-custom>...</style></noscript> is different than allowing an external CSS file. Do we need the custom version or just the external .css file?

@westonruter
Copy link
Member

I think the only need is for noscript > style[amp-custom]. The Bento example I gave for noscript > link would be for hand-authored non-AMP pages so it's not relevant for the validator.

For AMP pages, there is the similar existing Optimizer logic for the runtime CSS. The AmpBoilerplateTransformer adds a style[amp-runtime] but if fetching v0.css fails then it will instead add a link[amp-runtime]. Nevertheless, the failure case is not valid AMP as only style[amp-runtime] is in the spec:

tags: { # '<style amp-runtime>`, transformed AMP
html_format: AMP
enabled_by: "transformed"
tag_name: "STYLE"
spec_name: "style[amp-runtime] (transformed)"
descriptive_name: "style[amp-runtime]"
mandatory: true
unique: true
mandatory_parent: "HEAD"
attr_lists: "nonce-attr"
attrs: {
name: "amp-runtime"
mandatory: true
value: ""
dispatch_key: NAME_VALUE_PARENT_DISPATCH
}
attrs: {
name: "i-amphtml-version"
mandatory: true
value_regex: "^\\d{15}|latest$"
}
spec_url: "https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml/#stylesheets"
}

So I think the Optimizer transformer for component-specific noscript CSS can behave similarly: it can try to gather up the proposed *.noscript.css files for the components used on the page and add them to a noscript > style[amp-custom] element, but if fetching fails it may add (invalid) noscript > link elements instead.

Something else to consider is whether there should be a differentiation between user-authored noscript styles and component-specific noscript styles. Should there be separate noscript > style[amp-noscript] (or noscript > style[amp-runtime] ) in addition to allowing noscript > style[amp-custom], where the former comes before the latter?

@honeybadgerdontcare
Copy link
Contributor

In progress. cl/391926674

@Gregable
Copy link
Member

Gregable commented Sep 2, 2021

This is fixed and live everywhere.

@Gregable Gregable closed this as completed Sep 2, 2021
@westonruter
Copy link
Member

Confirmed!

Given example document, the noscript > style[amp-noscript] is valid:

image

JS Enabled JS Disabled
js-enabled js-disabled

Thank you!

@Gregable
Copy link
Member

Gregable commented Sep 2, 2021

No problem. If you later discover that we need support for additional css rules for each extension as mentioned above, please open a new issue for that.

@westonruter
Copy link
Member

westonruter commented Sep 4, 2021

As a few next steps I see, to open in other issues:

  1. Add noscript.css stylesheets to each component, e.g. for amp-accordion to force each collapsed section to be displayed.
  2. Create a new optimizer transformer that automatically adds these noscript stylesheets into a noscript > style[amp-noscript]. (In lieu of the previous point, this could be done initially with a single noscript stylesheet shipped with the optimizers.) See Add SSR transformer which adds noscript > style[amp-noscript] with base styles amp-toolbox-php#349.
  3. Add awareness of the style[amp-noscript] to the AMP-WP plugin so that plugin/theme authors can make use of it. See Add support for no-JS styling and noscript > style[amp-noscript] amp-wp#6603.

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

No branches or pull requests

8 participants