glimmerjs / glimmer-vm

MIT License
1.13k stars 190 forks source link

<noscript> tags when JS enabled are not ignored #839

Open snewcomer opened 6 years ago

snewcomer commented 6 years ago

@jsit and I found a weird bug.

When placing an img or style tag in <noscript> tags, the content inside still gets applied when JS is enabled. I found this in a fastboot (ember 3.1.2) app as well as in a non-fastboot app (ember 3.3).

Note - the image src gets downloaded, but doesn't display.

What does work is something like this, where figure has a css background-image.

<noscript>
  <figure></figure>
</noscript>

What doesn't work -

A simple test to try in application.hbs is (with a variety of scenarios depending on your app setup):

<noscript>
  <style>
    body {
      background-color: red !important;
      font-size: 0.2em !important;

      > .ember-view {
         background-color: red !important;
      }
    }
  </style>
</noscript>

Let me know if anybody sees something different!

chancancode commented 6 years ago

I agree this is a bug that should probably be fixed, though, in practice I think it's probably best to put this in your index.html anyway. Are you using server-side-rendering?

snewcomer commented 6 years ago

Yep, a server side rendered app.

Also, this use case is for lazy loading images (in a component). When JS is disabled, we still want to render the original high res image served from fastboot. But when enabled, we would like to save some bandwidth; however, seeing the unexpected results.

chancancode commented 6 years ago

Makes sense. I think <noscript>...

rwjblue commented 6 years ago

I guess I don't grok why the browser doesn't just take care of this for us?

rwjblue commented 6 years ago

Anyone have a link handy to the specs around noscript?

chancancode commented 6 years ago

@rwjblue I suppose we can file a bug, but document.createElement('noscript') is quite nonsensical. I suspect the noscript semantics might just all be a parse time/tree-construction time thing.

chancancode commented 6 years ago

https://html.spec.whatwg.org/multipage/scripting.html#the-noscript-element

So, a couple of problems:

The noscript element represents nothing if scripting is enabled, and represents its children if scripting is disabled. It is used to present different markup to user agents that support scripting and those that don't support scripting, by affecting how the document is parsed.

But on the other hand:

The noscript element has no other requirements. In particular, children of the noscript element are not exempt from form submission, scripting, and so forth, even when scripting is enabled for the element.

So, omitting them on CSR is not exactly right either, though probably no one would care.

rwjblue commented 6 years ago

I'm just not sure that we have a bug here. It seems to me that folks should guard any <noscript> content in an {{#if isFastBoot}} or somesuch. As far as I can tell rendering <noscript> content only makes sense during SSR...

snewcomer commented 6 years ago

{{isFastBoot}} seems like a good solution for fastboot enabled apps! Thanks. My only remaining question would be for non fastboot apps. I haven't found a solution yet.

rwjblue commented 6 years ago

Hmm, I don’t understand what you mean. If an app doesn’t have fastboot, you never render without JavaScript and therefore should never have <noscript>. What am I missing?

snewcomer commented 6 years ago

I'm sorry. Yes you are correct. I wouldn't put noscripts in there but for the index.html.

I think this potentially could be closed since the fastboot check solved the issue for me.

rwjblue commented 6 years ago

kk, thanks for confirming!

@chancancode - What do you think?

tomdale commented 6 years ago

Having markup wrapped in <noscript> still execute in the browser is at best very surprising and at worst a potential vector for security vulnerabilities. In the React world, this is a common pattern for dealing with lazy loaded images—user agents with scripting get the empty placeholder, user agents without scripting get the normal <img> tag.

In terms of semantics, I believe <noscript> and children should be rendered during SSR serialization, but should not be constructed during client-side rendering or rehydration. In terms of implementation strategy, I'm open to suggestions, but I suspect we can add a little bit of state to the DOMBuilder classes (like we do with SVGs already) to turn construction operations inside a <noscript> into no-ops.

chancancode commented 6 years ago

In the React world, this is a common pattern for dealing with lazy loaded images—user agents with scripting get the empty placeholder, user agents without scripting get the normal tag.

@tomdale I'm not sure if I understand how this works. Can you elaborate with some example markup/JSX?

chancancode commented 6 years ago

Reading the spec again, I think I misunderstood it. When scripting is enabled, what we are supposed to do is to insert the content as text.