Support additional quota for inline CSS #17269
Comments
|
I think this is a reasonable compromise, the 1k per element seem generous and the extra 25k seems reasonable. After compression if it's maxed out it's about 9 x 1500 bytes packets or 0.05 seconds on a 2 mbit/sec connection. |
|
The 50k limit is pretty reasonable to me at this stage. I trim any CSS that the specific page is not using. So a normal 80kb CSS for an entire site including it's assets, usually ends up between 10-25kb of CSS, leaving my pages around the 35kb mark each. @jpettitt Not everyone has 2mbit/sec connection on 3G, we should cater for the lower-denomination to account for any areas that might have shoddy connection speeds or 2G only, to give them a nice experience too. |
|
@AcidRaZor 2G (50kbps) even with EDGE (500kbps) is going to be pretty bad no matter what we do. An extra 12.5kB of css assuming 50% compression is less than 1% of a typical AMP page weight (* see below). While I agree it would be nice to keep CSS down sites who are converting existing content have a struggle to get under 50K and converting inline styles is particularly expensive as you have to jump through some hoops to give them high precedence. [*] My admittedly unscientific survey based on a dozen or so sites shows between 800k and 1.5M as a typical compressed transfer for the initial viewport load of a news article. This goes up to ~2M once you scroll and lazy load ads and images come in. |
|
I'm not super familiar with the performance effects of these constraints, so this may be a stupid question. Why do we not want to "advantage" inlined CSS? Said another way, if there is a combined limit of 75k, why would we not just allow the developer to spend that limit either in the It seems to me strictly more convenient to developers to simply increase 50k->75k without the additional requirement that it must be split across the different places in the document. Is there a meaningful performance difference that warrants the complexity? |
|
ccing @kristoferbaxter on this to hear their thoughts on this. I would go with @Gregable's suggestion. Allow the total size of all CSS to not exceed 75k and then have developers utilize this as they see fit. We have discussed the fact that the performance involved in parsing inline style attributes to CSS expressed in the If I remember correctly that allows @jpettitt's usecase of low res background images as was discussed in the previous thread. |
|
I agree with @Gregable that we likely do not want to have separate quotas, and a combined count seems best. However, I wonder if we should preserve the limit at 50k and for those who need additional space we introduce something similar to an Origin Trial to allow for additional CSS bytes. Rationale: 50k is a tremendous amount of space to style a single document. Additional space without a clear and repeatable metric for impact analysis reduces the opportunity to understand the quota change's impact both synthetically and in real user metrics. By creating a mechanism to request additional space (up to a new max size), most documents will remain safely under the 50k constraint and documents who do request more bytes can be separately analyzed from a performance perspective to understand the impact this additional space has in real user metrics. |
|
Out of curiosity do we have metrics for how currently deployed AMP pages are doing against this 50k limit? Do we know what percent of AMP pages actually are close to hitting the limit? I think making a decision without these numbers would set us up for a slippery slope of repeatedly expanding this limit every so often. |
|
Edit: ... and that was wong. Thanks @nainar for walking me through it yesterday. Styles anywhere on the page have to be parsed to build the style tree for page layout because a style attribute at the bottom of the page may position an element in the first viewport (AMP restrictions limit this but the browser doesn't know that). There is a slight advantage to a smaller The tl;dr is for the most part it doesn't really matter where the styles are from a browser standpoint it's the total number of them, and the network time to transfer them, that changes performance. |
|
@jpettitt, there is negligible performance difference in having styles expressed inline or via the Expanding on @kristoferbaxter's proposal. We should push for preserving the limit at 50k for all pages by default. We can instead run an experiment for X months where we allow publishers to pass a flag into the page asking for the increased 75kb limit.
Potential concerns:
Talked this through with @jpettitt and @Gregable who were supportive of the idea. |
|
I like proposal from @nainar I'd like to make it an origin trial so we know who is using it and can depreciate it if it proves detrimental. |
|
I'm intrigued by the proposal—one question is whether an origin trial like this would be useful to developers, since the use cases that require lifting the CSS limits can in some cases be involved to implement, so folks might be less likely to do so if there's a risk that the change might not be permanent. @cpapazian, as a developer using AMP engaged in this discussion, would an origin trial as described here be useful for your use cases? Both in terms of the amount of CSS allowed (general limit of 75k v. 50k), and the notion that this could be an experiment for now? |
|
@ericlindley-g I've been following along, and this all seems mostly reasonable. I still prefer no limit for inline styles for reasons @jpettitt has detailed. The issue (for us) with the 50k limit isn't that it's not enough---if you don't count the top/left styling that we use to position pins in the grid, we're actually around 10-20k. The issue has been that we use styles to position dynamic content, so CSS size is tied to the amount of content we display on the page, and components/tools to lazy load that content have been pretty limited. So, with inline styles and some changes to amp-list that are pending, we could get our CSS size pretty low. I think if there is an origin trial, we would probably be interested, but we're also interested in finding ways to trim our CSS. |
|
@cpapazian Got it — that makes sense. So for your primary use case, we can consider the amp-list / infinite scroll work that @cathyxz is working on to have the biggest impact for your needs, though raising the limit (or eliminating the limit for inline) would help too. Would be curious to hear from other developers out there what the use cases are for raising the CSS limit. |
|
The use case @cpapazian describes is similar to a lot of the CSS busting use cases I've seen. Typically we'll have a site with a template CSS that, after minimization and tree shake, is ~40k. However within the article body that is an arbitrary amount of CSS This is < 0.1% of pages in production with the AMP converter (it has a multi stage compressor and if the CSS is > 50K it will rename all the classes and ID's to two character (b36) values which mostly brings it back under the limit. |
|
@cpapazian I just wanted to clarify a statement you made earlier. When you say you prefer no limit to inline styles do you mean you prefer 50kb for the a combined limit for inline styles + styles in the |
|
@ericlindley-g your concern here is valid. This would need to be a long running experiment with a long wind down time as well. We don't want to catch developers unaware when we turn off the experiment when we turn it off. |
this one. |
|
Here's an |
|
@jaygray0919 - that is a really nicely done amp page |
|
Thanks @Gregable Greg. Credit goes to the developers; I am the messenger. We have similar pages that are at-or-over the CSS limit. Our "summary" carousels work well and fit appropriately. But our "detail" pages - elaborating the summary data in carousels - are problematic. For example, we have some food 'species' pages - formatted similarly to the chemical elements - that are problematic (where most of the content come from an |
|
FWIW you could get that CSS down to 51K just by renaming all the classes to 1 or 2 letters and running it through CSSO There are also some mergeable blocks. For example there are 10 distinct blocks containing Matching elements server side to the complex .element .model .orbital .electron:nth-last-child(20):first-child~.electron:nth-child(11) {
transform: rotate(180deg)
}After class substitution .tr180 {
transform: rotate(180deg)
}After class rename .xy {
transform: rotate(180deg)
}That should bring it well under 50k. Edit: Looking further much of the space is the electrons. If you dropped all the |
|
thanks @jpettitt we'll experiment with your proposal. |
|
We've been testing the AMP WordPress RC 1 plugin on a number of the most popular themes, e.g. Divi, Genesis, BeTheme, Astra, etc. as part of our testing for our WooCommerce AMP plugin. Although the tree shaking works great overall the end result in terms of appearance and usability for the themes is terrible, as a result of the 50k CSS limit. Raising the total limit to 75k would make the WP AMP plugin truly valuable. Although staying within the 50k limit is possible when designing a site for AMP from the ground up e.g. https://charliefoundation.org/ it is a serious roadblock for the current WordPress marketplace. Without raising the CSS limits I don't see any way the WordPress community will adopt AMP outside of a few custom designed sites. |
I'd like to understand this statement a little better. The tree-shaking should only remove unused CSS, so I don't follow how it would break the appearance / usability of the page. |
|
@Gregable Here's a typical example. Functioning WordPress theme before the AMP plugin is installed: With AMP plugin installed after tree shaking removes CSS exceeding 50k limit: Every popular WordPress theme that we've tested other than Automatic's Twenty Seventeen suffers the same post tree-shaking non-usable outcome and it's all from the 50k CSS limit. |
|
I'm still trying to understand. The tree shaking shouldn't be affected by the size limit. It should just remove unused CSS (dead code). If the input is 100kB of all used CSS, the output should still be 100kB of all used CSS. If the input is 100kB if all unused CSS, the output should be 0 bytes. Am I misunderstanding what the tree shaking step is doing? Are you maybe saying that the process is tree shaking followed by implementing a hard cutoff of 50k? If so, that makes sense. I just want to understand better. |
|
/cc @amedina @westonruter |
@Gregable Yes, that is right. If tree shaking is unable to remove enough of the CSS for all the stylesheets, then each stylesheet that brings the total over 50KB will then get excluded. (The AMP plugin concatenates all Something else to note in relation to #4555 (Allow CSS larger than 50k if 90% used): the AMP plugin's CSS tree shaker is necessarily limited in how vigorously it shakes the tree. Since it has to happen at runtime, it has to be conservative in how it examines the selectors to see if they apply to the document. So it looks for whether a class name used in a selector is extant in the document, whether a given element name exists in the document, and so on. It doesn't evaluate attribute selectors or compound-element selectors. All of this to say: allowing CSS larger than 50k if 90% used could be difficult in practice to take advantage of in the AMP plugin for WordPress. |
|
@westonruter do you do class renaming? Verbose class names tend to be a major culprit along with badly formatted (repetitive) media queries. |
|
@westonruter and I were just discussing this, he's taking a look at adding identifier mangling to the wordpress plugin. |
|
Thanks guys. Great idea. I've opened an issue to explore implementation: ampproject/amp-wp#1518 |
|
Trying to catch up on all of this. It sounds like there is a desire to do some kind of experimentation that @nainar proposed. Currently the validator doesn't really handle experimentation like this. If we do introduce something to handle experiments we'll need to be very explicit that at some point the experiment will end and those pages may become invalid. Is that something we're comfortable with? |
|
Given that #26466 was implemented, is this issue still relevant? |
Building on #11881, this issue is to discuss increasing the limit on CSS while retaining reasonable safeguards.
Proposal:
<style amp-custom>tag in<head>styleattribute per elementQuestion for developers using AMP:
Does this address the desired use cases by folks in the other thread? Would fiddling with the numbers in each limit address the use cases, or does the premise not fit from the start?
Questions for developers working on AMP itself:
Does this mitigate abuse/performance risks?
Is it feasible to implement?
/cc @cpapazian @Gregable @choumx @aghassemi @nainar @jpettitt @jridgewell @dvoytenko @alabiaga
The text was updated successfully, but these errors were encountered: