Amphtml: Intent to implement: partially drop restriction on `style` attributes
Some restrictions have to be preserved, but most can be dropped at this time, I believe.
Some considerations:
!importantshould be prohibited as with stylesheets (should be done already for SVG).url()values (e.g. forbackground-image, etc) should be rewritable by cache (should be done already for SVG).- The only allowed values of
positionarestatic,relative(andabsolute?). - 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. - SSR should be able to work with publisher-authored
styleattributes.
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
dvoytenko
All 70 comments
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.
jpettitt
on 31 Oct 2017
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
on 31 Oct 2017
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.
Gregable
on 31 Oct 2017
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
on 31 Oct 2017
Actually, template sanitizer needs to be updated to remove !important and other rules.
dvoytenko
on 1 Nov 2017
+@sebastianbenz this will break https://github.com/ampproject/amp-toolbox/blob/master/ssr/lib/transformers/ApplyLayout.js
jpettitt
on 14 Dec 2017
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?)
cpapazian
on 14 Dec 2017
/cc @alabiaga
choumx
on 18 Jan 2018
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
cpapazian
on 14 Feb 2018
/cc @jridgewell
alabiaga
on 20 Feb 2018
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.
jridgewell
on 21 Feb 2018
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 notifications@github.com
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.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 verydifficult 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
https://github.com/ampproject/amphtml/issues/11881#issuecomment-367154160,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAeKUPta5AUPJdaB6S7IAhTAY_bGIVHbks5tW1FngaJpZM4QNEjN
.
alabiaga
on 21 Feb 2018
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?
cpapazian
on 21 Feb 2018
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.
jridgewell
on 21 Feb 2018
amp-style won't degrade gracefully. Maybe we can split this work into stages, e.g. for first pass, rewrite style to '' for fixed elements in FixedLayer#setup and output a user error.
choumx
on 21 Feb 2018
@choumx any thoughts on whether this could support media queries? IIRC, inline styles don't normally support media queries, but since it looks like this is actually going to be implemented as a style tag, that isn't necessarily a constraint.
alternatively, we would scoped style tags be a possible alternative?
cpapazian
on 1 Mar 2018
it looks like this is actually going to be implemented as a style tag
We're trying to implement this using the normal style attribute for now. The idea is we want graceful fallback and to avoid diverging from HTML if possible.
would scoped style tags be a possible alternative?
Hmm, why doesn't putting them in <style amp-custom> work?
choumx
on 1 Mar 2018
We're trying to implement this using the normal style attribute for now. The idea is we want graceful fallback and to avoid diverging from HTML if possible.
makes sense. 👍
for context, <style amp-custom> see: https://github.com/ampproject/amphtml/issues/11881#issuecomment-351826881. we're using the <style amp-custom> right now, but it is preventing us from positioning content (with position: absolute) that loads after the initial render of the page (with amp-list, for instance).
cpapazian
on 1 Mar 2018
Security: Need to integrate a CSS parser (e.g. Caja) for dynamic templates like amp-mustache.
RULE #4 - CSS Escape And Strictly Validate Before Inserting Untrusted Data into HTML Style Property Values
https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_Sheet#RULE_.234_-_CSS_Escape_And_Strictly_Validate_Before_Inserting_Untrusted_Data_into_HTML_Style_Property_Values
choumx
on 19 Mar 2018
Correction: No need for CSS parser since JS execution from CSS is not possible on modern browsers.
choumx
on 21 Mar 2018
Hi folks,
Just to clarify a few things here. CSS can be added to a page in three ways:
- External stylesheet - Not paint blocking in Chrome (As of M66 - Chrome parses external stylesheets only as they are applied on elements)
<style>tag - Render blocking - needs to be parsed to see what style the LayoutTree will take on.styleattribute - Render blocking (for same reason as<style> tag) - has higher specificity as compared to any style expressed in<style> tag.
nainar
on 29 Mar 2018
Also, since the reason for the 50kb CSS limit is to encourage developers to only deploy CSS they need and help speed up time to First Paint. I think the styles expressed using inline styles should count towards the limit. Maybe with a slight increase in the limit?
nainar
on 30 Mar 2018
@nainar why would we want to consider inline styles against the 50k limit? In the use cases I am imagining, these styles would be applied to elements below the fold initially, and so would not have to block initial render. Additionally, as mentioned above, we need to allow inline styles on elements in templates ... not sure how to incorporate those styles into the 50k limit.
cpapazian
on 31 Mar 2018
I don't believe it's possible to determine a priori whether a given style is above or below the fold.
We did discuss children of <template> elements and agree that those should not apply to the 50KB limit.
choumx
on 31 Mar 2018
I don't believe it's possible to determine a priori whether a given style is above or below the fold.
Sure. I guess what I am proposing is that adding inline styling to an element could block or delay render for that element. That would still be useful for us--we would just have to be careful about only using this for content below the fold or other secondary content that doesn't need to be available on initial render. An inline style feature with size limitations would probably not be useful to us.
cpapazian
on 31 Mar 2018
For static content, it should still help a bit since you'll save bytes on the CSS selectors e.g. .Pin__27, etc. Aren't you guys planning to use amp-list eventually too?
choumx
on 31 Mar 2018
Aren't you guys planning to use amp-list eventually too?
yes. we are hoping to use inline styles in amp-list templates. that would get around the size limit.
would love to see a way to get more styles on the initial html, too. my feeling is that secondary content and content below the initial fold should have different requirements on what is and isn't acceptable in terms of blocking render.
cpapazian
on 1 Apr 2018
Dropping some notes from an offline meeting I had with @choumx , @alabiaga , @honeybadgerdontcare:
The work to do here includes:
- [ ] Verify
url()values are rewritten by AMP cache (this already works in SVG's case, so likely no change). - [ ] Server Side Rendering will add
!importantto properties it sets. - [ ] Validator ensures that
!importantand<!--not present in style attributes. This can be a regex. - [ ] Validator will validate css correctly parsing (loosely, for example, won't validate what are considered valid CSS properties) the same as it does with stylesheets.
- [ ] Validator will disallow
position:sticky/position:fixedproperties in style attributes, but continue to allow them in<style>tag. - [ ] Validator will implement a 50k limit for the sum of
<style amp-custom>andstyleattributes. First iteration will count attributes in<svg>and<template>tags and children. We will verify that the new<svg>constraint doesn't break a significant number of pages. - [ ] Later iteration will exclude
<template>tags and children from the 50k limit.
Gregable
on 3 Apr 2018
@Gregable For server side rendering, what properties are set that require !important? Would merely increased specificity be sufficient, or do they really need to be !important?
src-code
on 3 Apr 2018
server-side rendering is doing what the Runtime would do, setting several properties such as display, height and width based on how the layout should be applied.
honeybadgerdontcare
on 3 Apr 2018
this:
Validator will implement a 50k limit for the sum of
<style amp-custom>and style attributes. First iteration will count attributes in<svg>and<template>tags and children. We will verify that the new<svg>constraint doesn't break a significant number of pages.
combined with this:
Later iteration will exclude
<template>tags and children from the 50k limit.
could lead to an abuse of tags, as that will be seen as a workaround for CSS restrictions. Supporting this directly (there will be tradeoffs) would be beneficial.
cpapazian
on 3 Apr 2018
@cpapazian I might be using ambiguous language here accidentally. The idea is to only relax constraints over time. So, initially the 50k limit will be the sum of the bytes found in the <style amp-custom> tag as well as the bytes found in all of the document's style attributes, including <svg> and <template>. This is the most restrictive form. Over time, we can (unless decided otherwise) stop counting bytes inside style attributes on tags inside a <template> tag. tag.
Gregable
on 4 Apr 2018
@Gregable making it the sum of makes this a useless feature. It's also unnecessary as style attributes do not have the same performance hit that a <style> tag in the header does because they are single element scoped.
jpettitt
on 20 Apr 2018
as @jpettitt mentions, this severely limits the capabilities of inline styles.
for us, this presents a maintenance nightmare, as the CSS has runtime dependencies (even if we use amp-list, we'd like to have some grid content available immediately and to provide a fallback div with more content). if there was a way to set absolute position of elements, we could separate content layout from "style", and use our standard CSS (which is much better than the current CSS we include in AMP pages).
if there is a real concern around unlimited inline styles (there does not seem to be a performance concern), then an alternative, which would solve our issue, is to extend amp-layout to support an "absolute" layout. similar to how fixed and fixed-width set an inline height/width style on the element, amp-layout could implement the inline styling for top/left/bottom/right (and maybe z-axis). obviously this would be redundant if inline styles was unlimited.
/cc @aghassemi
cpapazian
on 9 May 2018
I also vote for not including inline-style as part of the same CSS limit (not opposed to having another limit to discourage large data-urls for images, etc... however). @dvoytenko recently mentioned he has dome streaming DOM demos that shows inline styles are not blocking the same way CSS in head is.
aghassemi
on 10 May 2018
@dvoytenko recently mentioned he has dome streaming DOM demos that shows inline styles are not blocking the same way CSS in head is.
Doesn't that differ than what @nainar said?
jridgewell
on 10 May 2018
@jridgewell @nainar @dvoytenko I did a quick test and my conclusion is it does not block rendering for the whole document which is the big difference vs CSS in head. Chrome seems to progressively render and paint as document is streamed. In the following example, the first box is rendered immediately, the second box has a huge inline CSS that takes a bit to arrive, so that one and the next one are blocked.
https://storage.googleapis.com/ampconf-76504.appspot.com/test.html
Maybe we should put limit per element instead of an overall limit.
aghassemi
on 10 May 2018
I'd love to see a per element limit of at least 1K (this would allow for low res background images which would be a huge UX improvement over current loading placeholders)
jpettitt
on 10 May 2018
@aghassemi: Your example actually proves @nainar's point: it's blocking for all _following_ elements (the yellow box can't paint until the crazy box parses).
jridgewell
on 10 May 2018
I wonder if a alt attribute of equal length would also delay it and how much by - IE is it the CSS parse or just the network?
jpettitt
on 10 May 2018
@jpettitt to answer that question you would have to parse a significant amount of CSS/HTML, parsing (HTML or CSS) compared to a network request should be relatively minimal
nainar
on 10 May 2018
@jridgewell sure but bytes for the next elements has not arrived from network so there is nothing to render. what I found hard to test and still don't know is whether the browser would block rendering of following elements until inline css for the previous element is parsed and calculated. @nainar I assume based on your investigation, it does?
Per element limit seems more important to me than overall limit based on this behaviour.
aghassemi
on 10 May 2018
I wonder if a alt attribute of equal length would also delay it and how much by - IE is it the CSS parse or just the network?
This really two questions:
- Should we encourage attributes with absurdly long values, since they may hurt DOM parsing?
- Does style attribute block render of later elements?
I think the answer to the first one should be "no".
The second appears to be yes. The performance for the following two pages (when cached, eliminating network latency):
- Inline Alt
- Initial paint at 122ms
- Final paint at 426ms
- 304ms to paint page
- Inline Style
- Initial paint at 150ms
- Final paint at 585ms
- 435ms to paint page
jridgewell
on 10 May 2018
@aghassemi yup. So for example if your last element had a position:sticky/fixed and hence would be rendered before both the red and crazy box, the only way to determine that is to first parse the CSS on the page and then paint as you go along once you have a sense of the Layout and hence the Paint fragments.
If we parsing piecemeal in the position:sticky example you would see a jump which we don't.
@jridgewell the second hyperlink should read inline CSS?
nainar
on 10 May 2018
the second hyperlink should read inline CSS?
Hm? Aren't we testing inline style attribute?
jridgewell
on 10 May 2018
Sorry, I meant both hyperlinks currently say Inline Alt.
nainar
on 10 May 2018
I edited it after posting.
jridgewell
on 10 May 2018
🤦♀️ - the problem with GitHub you never refresh
nainar
on 10 May 2018
@jpettitt if I understand your suggestion correctly, you mean a 50K limit on the <style> tag. And a 1k limit on individual style attributes on elements? In that case just having 50 elements (which I assume most sites in the wild are above the limit on) would buy you double the limit?
nainar
on 10 May 2018
Some stats - 6x cpu slowdown
Original - huge style attribute 3.0 seconds
Same style applied as a class 3.0 seconds but no initial partial paint
Style converted to an alt attribute (same size but not parsed) 1.8 seconds.
I should also point out that's a 10 megabyte inline style, if we set a reasonable limit I really don't think this is going to be an issue.
jpettitt
on 10 May 2018
@nainar yes 1k per style attribute. I have a very specific use case in mind: low res background images.
jpettitt
on 10 May 2018
@jpettitt Inline styles are not the only way (or the ideal way IMO) to implement low-res image placeholders in AMP. We actually have an eng res starting on Monday working on a very similar feature.
choumx
on 10 May 2018
@choumx - it's not clear to me how that can be done for non-cache uses cases other than by the publisher. Doing it just for the cache doesn't help as that's generally not a case where slow load is an issue.
jpettitt
on 10 May 2018
@alanorozco's proposal from last year works both on and off AMP Cache: https://docs.google.com/document/d/1NWlpOe-sZONPAvxYjyEApI1PTdBqEVPBEd2ikYJOvvA/edit
choumx
on 10 May 2018
@jpettitt Can't you (today) use something like:
<amp-img src="reallybig.gif" ...>
<amp-img placeholder src="data:image/gif;base64,..." ...>
</amp-img>
</amp-img>
Some stats from my side, the idea is to show CSS parse time relative to DOM parse time, since we don't limit any other attribute in DOM, these numbers should be helpful in determining how much to limit style attribute:
Conditions:
- 800K attribute size (has complex CSS properties like shadow and gradient)
- 6x CPU slowdown
24 DOM elements:
- as
non-style: 1.8s - as
style: 2s
240 DOM elements:
- as
non-style: 15s - as
style: 17s
so although style CSS parsing is not free, it is not very significant either compared to DOM parsing, seems to be adding ~15% or so more. Now given:
1- we don't have limit on other random attributes.
2- CSS parsing seems fairly fast.
3- inline styles don't suffer from the 2015 Christmas CSS problem and everything is used.
1k per element limit seems totally reasonable to me.
aghassemi
on 10 May 2018
a number of layouts _already_ use inline styles without limits. fixed, fixed-height, flex, and fluid set height and/or width as inline styles and responsive sets display: block and padding-top.
if there are reasonable limits put on the inline style itself (@jpettitt's suggestion of 1k per element seems reasonable), then this would be no different than what already exists with amp-layout (or various AMP components that have a layout attribute).
cpapazian
on 10 May 2018
Today, a per-element limit is a meta problem since it allows a workaround for the 50K <style> tag. IMO, this allows a different kind of bad CSS hygiene by encouraging duplication CSS rules that ought to live in a <style> tag across the inline styles of many elements.
Maybe this won't be a big problem in practice, but being conservative now doesn't preclude tweaking the limits in the future. E.g. @honeybadgerdontcare is implementing this in a way so that we could have a separate combined limit for inline styles.
Per Malte, the long-term goal for AMP on web is to allow unlimited styles when we can confirm they are all being used. So this will change in the future anyways. We haven't discussed specifics but I think embedded AMP should be more strict and could use this constraint anyways.
choumx
on 10 May 2018
Per Malte, the long-term goal for AMP on web is to allow unlimited styles when we can confirm they are all being used. So this will change in the future anyways. We haven't discussed specifics but I think embedded AMP should be more strict and could use this constraint anyways.
Given that a style attribute is, by definition, being used this would argue for being more generous with style attributes.
jpettitt
on 11 May 2018
This is a high priority issue but it hasn't been updated in awhile. @alabiaga Do you have any updates?
ampprojectbot
on 5 Jun 2018
Update. @honeybadgerdontcare is working on server side changes relating to the count and validation of CSS size limit.
alabiaga
on 5 Jun 2018
@alabiaga to confirm, you guys are sticking with the decision to include inline styles in the 50k limit?
cpapazian
on 5 Jun 2018
Yes, for now.
choumx
on 5 Jun 2018
@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 https://github.com/ampproject/amphtml/issues/11881#issuecomment-388190364. 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?
cpapazian
on 5 Jun 2018
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.
Gregable
on 5 Jun 2018
thanks, @Gregable. that makes sense.
cpapazian
on 5 Jun 2018
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).
vamshi5889
on 20 Jun 2018
@vamshi5889 #16057 adds SVG to amp-mustache under a new version.
choumx
on 20 Jun 2018
Thanks for the info @choumx
vamshi5889
on 21 Jun 2018
This is now live in the validator and amp-mustache support will be enabled in canary/prod in #16544.
choumx
on 3 Jul 2018
Related issues
cvializ
·
3Comments
akshaylive
·
3Comments
sryze
·
3Comments
mkhatib
·
3Comments
westonruter
·
3Comments
Most helpful comment
@jridgewell @nainar @dvoytenko I did a quick test and my conclusion is it does not block rendering for the whole document which is the big difference vs CSS in head. Chrome seems to progressively render and paint as document is streamed. In the following example, the first box is rendered immediately, the second box has a huge inline CSS that takes a bit to arrive, so that one and the next one are blocked.
https://storage.googleapis.com/ampconf-76504.appspot.com/test.html
Maybe we should put limit per element instead of an overall limit.