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

Change boilerplate bootstrap <style> to <script> #323

Closed
dvoytenko opened this Issue Sep 28, 2015 · 32 comments

Comments

Projects
None yet
@dvoytenko
Copy link
Collaborator

dvoytenko commented Sep 28, 2015

Milestone: Post Preview

The script is:

  <script>
    (function(w, d) {
      d.documentElement.style.opacity = 0;
      w.onerror = function() {
        d.documentElement.style.opacity = 1;
      };
    })(window, document);
  </script>

And the counterpart in AMP runtime:

    win.document.documentElement.style.opacity = 1;
    win.onerror = ampErrorHandler;
  1. The script itself will live in it's own file.
  2. It will be released on request (not automatically) as following:
    2.1. The file will be compiled, minified and obfuscated
    2.2. The script will be integrated into validator. Validator will validate against all known versions of this script.
    2.3. Once validator is updated, it will be posted in the "Setup" publishers' guide.
  3. We will remove requirement of "amp-custom" in the user stylesheet's <style> element. We will go back to require a single <style> in the document.
  4. We can separately discuss if the proxy should automatically update a matched version of this script to the latest known version.

@dvoytenko dvoytenko changed the title Change boilerplate bootstrap \<style> to \<script> Change boilerplate bootstrap <style> to <script> Sep 28, 2015

@adactio

This comment has been minimized.

Copy link

adactio commented Oct 9, 2015

Looks good. Here's what I proposed in #548...

How about setting the default opacity with JavaScript instead?

<script>document.body.style.opacity=0</script>

It's shorter and has the same effect; if JavaScript isn't available, the CSS doesn't get applied—no need for a noscript element. This way, the opacity is always set with the same technology: JavaScript.

Caveat: I'm guessing that (because of the asynchrous loading of the ampproject JavaScript file), this script element that I'm suggesting (which, like the current style element, should be blocking) would need to be placed before the script element that calls the external file:

<script>document.body.style.opacity=0</script>
<script src="https://cdn.ampproject.org/v0.js" async></script>

@dvoytenko's script looks a lot more robust than my simple suggestion (with the addition of the onerror handler).

In any case, I'm really, really glad to see the opacity reset being moved out of style and into script. And being able to drop the proprietary amp-custom attribute is definitely a bonus!

@kzap

This comment has been minimized.

Copy link

kzap commented Oct 9, 2015

+1 fallback for nonjs or failed js rendering should be part of it, what if the js errors and the body never gets displayed...

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Oct 9, 2015

/cc @Gregable @powdercloud this is coming down the pike.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Oct 13, 2015

The primary problem with this is that it does not work with our default CSP.

We cannot use nonces in AMP because they are not compatible with cache-control: public (besides not being supported in Safari).

We could use a hash, but that would very complicated (Maybe OK) but also turn on unsafe-inline in Safari which is an absolute no-go.

So, while I absolutely love the suggestions here I think we cannot implement them until Safari ships a robust CSP implementation. One of our goal in AMP is robust naive serving. Making every single publisher on the planet support a complex dynamic CSP seems very unlikely to be successful.

Does anyone have an idea that works without any inline JS?

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Oct 13, 2015

Yeah, I haven't thought about CSP. It's definitely a bit of a show stopper. But perhaps we can still make our current boilerplate simpler and more reliable? Among other things:

  • We can always set opacity on documentElement instead of body. Not a huge deal, but a little easier to work with.
  • Maybe we can figure out a way to remove amp-custom from user style? I think it's better to add an attribute to boilerplate style or maybe none at all - it's kind of clear which is which.
  • Register default error handler as the first thing in our binary to reset opacity in case of failure.
@powdercloud

This comment has been minimized.

Copy link
Collaborator

powdercloud commented Oct 13, 2015

I think with the css parser getting ready it should be ok to have the opacity thing inside the same style tag as what's now known as the amp-custom style. We need to play a little bit with this to get a feel for how to implement rules for the css though.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Oct 13, 2015

@dvoytenko those are all good suggestions, but they don't address the big win which would be to be robust in the case of JS load failure.

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Oct 13, 2015

@cramforce Yes, unfortunately. But it looks like we don't have much of an option at this time? If we can't inline style and can't do nonce do we have any other option?

One version would have window.onerror handler set up as a first thing not only in the main amp.js binary, but in all extension binaries - these all will reduce the probability of failure without handler. We can also enable viewer integration to do the same.

@powdercloud

This comment has been minimized.

Copy link
Collaborator

powdercloud commented Oct 13, 2015

Is there some way to listen to the script element that's loading amp.js, like there is with onreadystatechange with xml http requests?
Also, could something where you set a timer be acceptable? E.g., after n seconds of trying to render the doc smoothly, fall back to making whatever is there visible?
(OK, quite possibly these are dumb suggestions feel free to ignore haha)

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Oct 13, 2015

Well, if we can insert inline script - we can do it all with ease as described above. The problem is that our CSP doesn't allow inline styles and things are even more complicated when Safari is involved.

@powdercloud

This comment has been minimized.

Copy link
Collaborator

powdercloud commented Oct 13, 2015

Abusing a CSS animation is out as well for it, yes?

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Oct 13, 2015

@powdercloud I think that is an amazing idea!!!

An opacity animation that stays at 0 for N seconds that then goes to 1.

@adactio

This comment has been minimized.

Copy link

adactio commented Oct 13, 2015

@powdercloud @cramforce Ooh, that's smart!

My main concern with the current situation is that mixes JavaScript and CSS, assuming both will work in tandem. A solution that's either all JavaScript or all CSS would be much more robust. Until now, I had really only been thinking of the JavaScript options, but what you're proposing (all CSS) sounds much more straightforward and elegant.

@erwinmombay

This comment has been minimized.

Copy link
Member

erwinmombay commented Oct 14, 2015

@powdercloud hah, that would be a really neat sol'n.

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Oct 14, 2015

Yeah, I think it's a cool idea. We can try that. It could look something like this:

    body {
      opacity: 0;
      animation: amp-starter 0s 10s 1 normal forwards;
    }
    @keyframes amp-starter {
      0%   { opacity: 0; }
      100% { opacity: 1; }
    }    

We can give it a max of, say, 10 seconds to wait before the opacity is reset to 1 automatically. We'd still need a JS solution that would reset body at the earliest opportunity to

body {
  opacity: 1;
  animation: none;
}

The benefit here is, of course, that opacity can be reset to 1 if JS fails to load/initialize for any reason. On the other hand we need to consider:

  1. It won't be as critical, but we may still want to use noscript
  2. With vendor prefixes, this CSS will definitely be a copy-paste-only boilerplate
@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Oct 14, 2015

I think a good timeout value would be 3-4 seconds, very similar to Chrome's default timeout for fonts.

@adactio

This comment has been minimized.

Copy link

adactio commented Oct 14, 2015

@dvoytenko wrote:

body {
  opacity: 0;
  animation: amp-starter 0s 10s 1 normal forwards;
}
@keyframes amp-starter {
  0%   { opacity: 0; }
  100% { opacity: 1; }
}

Would it possible to drop the initial opacity: 0 declaration on the body? Right now there's an assumption that both opacity and animation which is a pretty safe assumption, but still a bit nervous-making.

I don't the think the lack of the initial opacity: 0 would cause a flash of content or anything, right? (because the CSS is blocking)

body {
  animation: amp-starter 0s 3s 1 normal forwards;
}
@keyframes amp-starter {
  0%   { opacity: 0; }
  100% { opacity: 1; }
}
@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Oct 14, 2015

Ok. I think boilerplate will look like this then:

<style>body{opacity:0;-webkit-animation:-amp-start 0s 10s 1 normal forwards;-moz-animation:-amp-start 0s 10s 1 normal forwards;-ms-animation:-amp-start 0s 10s 1 normal forwards;animation:-amp-start 0s 10s 1 normal forwards}@-webkit-keyframes -amp-start{0%{opacity:0}to{opacity:1}}@-moz-keyframes -amp-start{0%{opacity:0}to{opacity:1}}@-ms-keyframes -amp-start{0%{opacity:0}to{opacity:1}}@keyframes -amp-start{0%{opacity:0}to{opacity:1}}</style>
<noscript><style>body{opacity:1;-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>

It looks like we actually can drop some of prefixes but we'll probably have to keep -webkit :(

A version without opacity:

<style>body{-webkit-animation:-amp-start 0s 10s 1 normal forwards;-moz-animation:-amp-start 0s 10s 1 normal forwards;-ms-animation:-amp-start 0s 10s 1 normal forwards;animation:-amp-start 0s 10s 1 normal forwards}@-webkit-keyframes -amp-start{0%{opacity:0}to{opacity:1}}@-moz-keyframes -amp-start{0%{opacity:0}to{opacity:1}}@-ms-keyframes -amp-start{0%{opacity:0}to{opacity:1}}@keyframes -amp-start{0%{opacity:0}to{opacity:1}}</style>
<noscript><style>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>

This version assumes that zero-offset state in -amp-start animation is executed before the first paint.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Oct 14, 2015

@dvoytenko See @adactio's idea that would probably allow being a bit more aggressive on the non-prefix side.

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Oct 14, 2015

@adactio, dropping initial opacity works fine on devices I checked. The animation would have to change to start immediately (currently delayed 10s above) and would run instead for that duration and would need to use a step-end timing function or similar.

I haven't yet found a clear confirmation that the keyframes animation MUST start before the first paint - so that needs to still be confirmed.

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Oct 14, 2015

But I think it's reasonable to expect that zero-offset step is applied immediately. So, I think this should work well.

@kevinmarks

This comment has been minimized.

Copy link

kevinmarks commented Oct 14, 2015

@dvoytenko you have opacity 0 in noscript in #323 (comment)

which is worse than opacity 1 currently; I thought the point was to avoid the noscript distinction entirely

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Oct 14, 2015

@kevinmarks Typo. Fixed. I also added a no-opacity version.

@adactio

This comment has been minimized.

Copy link

adactio commented Oct 14, 2015

@dvoytenko Thanks for testing the version with initial opacity: much appreciated.

@rudygalfi

This comment has been minimized.

Copy link
Contributor

rudygalfi commented Jan 30, 2016

Closing this assuming #998 covers it and necessary validator changes have happened. Reopen if still needed.

@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented Feb 2, 2016

@sriramkrish85, @cramforce: I believe this can be closed now?

@cramforce cramforce closed this Feb 2, 2016

@adactio

This comment has been minimized.

Copy link

adactio commented Feb 4, 2016

The docs are still out of date:

https://www.ampproject.org/docs/get_started/create/basic_markup.html

Can we re-open this until the docs are updated?

@powdercloud

This comment has been minimized.

Copy link
Collaborator

powdercloud commented Feb 5, 2016

@adactio Thank you I filed ampproject/docs#32.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Feb 5, 2016

Next docs push will fix. Should happen any day.

On Fri, Feb 5, 2016 at 1:23 PM, Johannes Henkel notifications@github.com
wrote:

@adactio https://github.com/adactio I filed ampproject/docs#32
ampproject/docs#32.


Reply to this email directly or view it on GitHub
#323 (comment).

@erwinmombay

This comment has been minimized.

Copy link
Member

erwinmombay commented Feb 5, 2016

confirming with @Meggin if we can do a release

@Meggin

This comment has been minimized.

Copy link
Collaborator

Meggin commented Feb 5, 2016

Need to make a few changes to validation doc. Can you give me an hour?
On Feb 5, 2016 2:23 PM, "erwin mombay" notifications@github.com wrote:

confirming with @Meggin https://github.com/Meggin if we can do a release


Reply to this email directly or view it on GitHub
#323 (comment).

@erwinmombay

This comment has been minimized.

Copy link
Member

erwinmombay commented Feb 5, 2016

docs pages should be updated w/in the hour

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