Intent to implement: partially drop restriction on style attributes

[dvoytenko]

Some restrictions have to be preserved, but most can be dropped at this time, I believe.

Some considerations:

1. !important should be prohibited as with stylesheets (should be done already for SVG).
2. url() values (e.g. for background-image, etc) should be rewritable by cache (should be done already for SVG).
3. The only allowed values of position are static, relative (and absolute?).
4. Based on some future optimizations we want to do, we should probably also disallow top:X, left:X, right:X, bottom:X. Though we may be able to remove this restriction.
5. SSR should be able to work with publisher-authored style attributes.

The only real restrictions comparing to the current state of things are position and top/bottom/etc restrictions. The question is whether, given these restrictions, this is still a good idea to implement.

/cc @Gregable @honeybadgerdontcare @jpettitt

[jpettitt]

Even with the position and top/bottom/etc restriction I think this is well worth doing. It significantly reduces the complexity of converting existing content, particularly article bodies with journalist authored html.

[Gregable]

What will we do with mustache templates and amp-bind attributes here? Would we want to run a CSS parser in javascript runtime?

I recommend that we do not allow style attributes inside a mustache templates or as amp-bind attributes.

[Gregable]

I also suggest that we count the bytes in these style attributes toward the amp-custom byte limit, otherwise we'll see folks making things worse by stuffing style attributes full of rules to avoid the byte size limit.

[dvoytenko]

Templates are fine. amp-bind probably not at first, but could consider separately.

Byte size, however, is not relevant here. The reason why we want limit on CSS style up top is to ensure that we start rendering body faster. This is because, all CSS styles parsed, even those that are never used or used at the very bottom of the document, blocks rendering of the whole document. Style attributes actually allow us to achieve this goal better since they are by definition scoped and thus we start rendering faster.

[dvoytenko]

Actually, template sanitizer needs to be updated to remove !important and other rules.

[jpettitt]

+@sebastianbenz this will break https://github.com/ampproject/amp-toolbox/blob/master/ssr/lib/transformers/ApplyLayout.js

[cpapazian]

Based on some future optimizations we want to do, we should probably also disallow top:X, left:X, right:X, bottom:X. Though we may be able to remove this restriction.

Allowing top/left/right/bottom styles would unblock something that we at Pinterest have been stuck on for a while. For the current implementation of our pin grid in AMP, we calculate the position of each pin (for multiple viewports) server side and use that to generate a CSS class for each pin.

for example:

.Pin__27 {
  left: 728px;
  top: 958px;
}
.Pin__28 {
  left: 182px;
  top: 988px;
}
.Pin__29 {
  left: 364px;
  top: 988px;
}
.Pin__30 {
  left: 910px;
  top: 1023px;
}
.Pin__31 {
  left: 0px;
  top: 1123px;
}
...

There are two major drawbacks to this approach that inline styling of top/left attributes could help to address.

First, this adds a ton of CSS to the style tag, which means we are always operating dangerously close the 50k limit. And, since most of these pins are below the fold initially, that styling does not need to block the initial page render.

Second, because we need to calculate the pin positions up front, we can't do anything with dynamic content. We'd like to start using <amp-list> and fetch additional and/or personalized content after the initial page load, but without some sort of custom styling per-item, this is not possible.

Ideally, this would come with some sort of way to update styles (amp-bind?) when viewport size changes, since this wouldn't be compatible with media queries.

If you decide to continue disallowing top/left/right/bottom attributes, I would love to discuss other possible ways to solve our grid problem (maybe additional style tags in amp-list or mustache templates?)

see:
https://www.pinterest.com/amp/explore/highland-cattle/

[dreamofabear]

/cc @alabiaga

[cpapazian]

Is it possible to release this early under an experiment flag? We have some work that is blocked on this change that we would love to get started on ... /cc @ericlindley-g

[alabiaga]

/cc @jridgewell

[jridgewell]

Ok, so there's an issue with this on the runtime side. Up until this, we've considered element.style to be a pristine record of runtime's changes to the element's style. Specifically, we can set a value, then unset it to get back to the publisher supplied style state.

<style>
  .Pin__27 {
    left: 728px;
    top: 958px;
  }
</style>
<script>
  pin27.style.top = '10px';
  pin27.offsetTop; // => 10
  pin27.style.top = '';
  pin27.offsetTop; // => 958
</script>

This style (:rimshot:) of mutate then unset is used extensively in FixedLayer, dom#toggle, and I'm sure a few other places.

So it's possible to allow this, but under the current AMP usage, we would would either have to due a runtime transformation or refactor a lot of very difficult code (FixedLayer).

---

The runtime transformation we can do is:

• Detect inline styles on an element
  • Cut style attribute
  • Add a uniq attribute to the element (something like -i-amphtml-inline-style-${id++})
  • Create a new style element, add copied styles to it with selector [-i-amphtml-inline-style-0]
  • Inject style element into head.

This "simulates" inline styles, while preserving the runtime expectations on element.style object.

[alabiaga]

Thanks for your input Justin. I will work on this runtime transformation and will keep you up to date with its progress.

…

On Tue, Feb 20, 2018, 6:12 PM Justin Ridgewell ***@***.***> wrote: Ok, so there's an issue with this on the runtime side. Up until this, we've considered element.style to be a pristine record of *runtime's* changes to the element's style. Specifically, we can set a value, then unset it to get back to the publisher supplied style state. <style> .Pin__27 { left: 728px; top: 958px; }</style> <script> pin27.style.top = '10px'; pin27.offsetTop; // => 10 pin27.style.top = ''; pin27.offsetTop; // => 958</script> This style of mutate then unset is used extensively in FixedLayer, dom#toggle, and I'm sure a few other places. So it's possible to allow this, but under the current AMP usage, we would would either have to due a runtime transformation or refactor a lot of very difficult code (FixedLayer). ------------------------------ The runtime transformation we can do is: - Detect inline styles on an element - Cut style attribute - Add a uniq attribute to the element (something like -i-amphtml-inline-style-${id++} - Create a new style element, add copied styles to it with selector [-i-amphtml-inline-style-0] - Inject style element into head. This "simulates" inline styles, while preserving the runtime expectations on element.style object. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#11881 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAeKUPta5AUPJdaB6S7IAhTAY_bGIVHbks5tW1FngaJpZM4QNEjN> .

[cpapazian]

Inject style element into head.

@jridgewell my understanding is that the 50k CSS limit was meant to prevent a hard block in the render path. The styles that we'd like to inline are for content below the fold. Would adding an arbitrary amount of inline styling lead to performance issues? or do you mean to suggest that the inline styles would be injected into the head after initial render?

[jridgewell]

or do you mean to suggest that the inline styles would be injected into the head after initial render?

These would be after initial render. We could discuss SSR (doing the above algorithm on server) with the AMP Cache devs, but it would have to fall under the 50k limit.

In the end, it'd probably mean still forbidding the style attribute (which the browser would control) and instead giving you an amp-style (or whatever name) that we could control when it applies.

[dreamofabear]

Yes, for now.

[cpapazian]

@choumx would love to see a little more of the reasoning here in the thread. I thought @aghassemi made a compelling argument against the notion that inline styles posed a performance concern, summarized in #11881 (comment). Concerns by you and @nainar seemed to boil down to the fact that this could be abused, which seem overblown and unrealistic. If, as you say, this is just a "for now" decision, Is there a planned timeline for expanding the limit?

[Gregable]

The "for now" decision is simply just to move forward with implementation. It should probably not be construed as a decision on the limit we wish to have.

[cpapazian]

thanks, @Gregable. that makes sense.

[vamshi5889]

Hey guys, I had an ideal scenario with some issues which I asked couple of months ago 14018

The problem I faced at that time was I cannot use SVG (for binding color dynamically in inline css) inside amp-mustache.

Then I looked for the basic HTML 5 options and resolved my issue by using table with bgcolor attribute.

Hope this might help someone who are looking to bind colors dynamically. And there could be other ways of achieving CSS behavior without style attribute (inline style).

[dreamofabear]

@vamshi5889 #16057 adds SVG to amp-mustache under a new version.

[vamshi5889]

Thanks for the info @choumx

[dreamofabear]

This is now live in the validator and amp-mustache support will be enabled in canary/prod in #16544.