Amphtml: Design Review 2020-01-29 16:30 UTC (amp-Iframely, CSS size limit)

Created on 12 Dec 2019 · 9Comments · Source: ampproject/amphtml

Time: 2020-01-29 16:30 UTC (add to Google Calendar)
Location: Video conference via Google Meet

The AMP community holds weekly engineering design reviews. We encourage everyone in the community to participate in these design reviews.

If you are interested in bringing your design to design review, read the design review documentation and add a link to your design doc or issue by the Monday before your design review.

When attending a design review please read through the designs before the design review starts. This allows us to spend more time on discussion of the design.

We rotate our design review between times that work better for different parts of the world as described in our design review documentation, but you are welcome to attend any design review. If you cannot make any of the design reviews but have a design to discuss please let mrjoro@ know on Slack and we will find a time that works for you.

Design Review

Source

mrjoro

Most helpful comment

Notes on relaxing CSS size limits

Why did we have a limit in the first place? To encourage CSS hygiene—hard to figure out when to remove CSS when they’re all globally scoped styles. AMP intentionally limited the number of bytes to 50,000, which was only one way to help with hygiene. This limit was the simplest to implement among options.
Why change this now? Today, more and more document authors are using either (1) generic CSS frameworks that load boilerplate CSS that may or may not apply to the document and (2) Bem syntax that hits our byte limit faster. Bem + object oriented CSS rationalize how CSS is being used in a document. If you use either of these approaches along with a CSS minifier, we still have a large number of documents that still cannot make it through the current size limit, many of which are complex and attract high levels of user engagement.
Impact of relaxing limits: The difference when CSS is compressed correctly and served from AMP Cache is 2KB of transfer after AMP's minification techniques — not a huge performance impact. By relaxing the limit somewhat, AMP still gets to enforce hygiene, just not as strictly.
How big is the change? The PR currently changes 55 LOC to validator across all format types. This is probably not needed for email (can be moved to a separate PR) since there are not many cases in AMP4Email that are concerned about the limit.
Alternatives considered:
- Keep existing limit: AMP can create a more general CSS minifier to help others do this — AMP should do this anyway, it can be done _along with_ rather than _instead of_ relaxing the limit.
- Adopting an adaptive limit: This would be a CSS limit based on document complexity (the more DOM nodes and more complicated structure => the more CSS allowed). The complexity of implementing this option is significant compared to current proposal. Every validator pass would need to inherit this new functionality. This was considered because of the assumption that DOM complexity is correlated to CSS size, which is untrue. Risk of opposite effect — publishers might be incentivized to add a billion DOM nodes to increase CSS limit, which hurts performance much more than the extra CSS to begin with.
How does CSS magnification handle AMP mustache/bind/etc?
Minification by Wordpress are aware of amp-bind, amp-list, other programmatic work within AMP, and not shake out features that would match combinatorial possibilities. This is not true for amp-script but Wordpress is working on this too. They don’t mingle class names for example, but a future minifier might be able to do this.
Is this size limitation increase needed proportionally across formats?
Not so much for email — people create custom CSS for AMP emails and don't often use other toolkits to create larger more complex CSS. Email authors began in a much more constrained place anyway and as a result there is not as much opportunity for abstraction. For stories, people are not hitting this limit significantly, but based off conversations it does happen.

/cc @kristoferbaxter

caroqliu on 29 Jan 2020

👍3

All 9 comments

The UI & Accessibility Working Group will be presenting their periodic updates at this Design Review (10-15 minutes).

/cc @ampproject/wg-ui-and-a11y @nainar

mrjoro on 15 Jan 2020

This would be a good time to review I2I: AMP-Iframely component #26226, which is accompanied by PR #26151

/cc @alanorozco
Also per Alan's comment:

Additionally, embedding iframes have concerns that require security review. Adding @ampproject/wg-security-privacy and @molnarg.

iparamonau on 24 Jan 2020

Link to UI and A11y WG update: https://github.com/ampproject/wg-ui-and-a11y/issues/38

nainar on 27 Jan 2020

@ampproject/wg-security-privacy for mention

alanorozco on 28 Jan 2020

I'd like to discuss the I2I for relaxing CSS limits.

PR: 26475

kristoferbaxter on 29 Jan 2020

Is there anything special to be done to join a meeting? It says "you'll join when someone lets you in"

iparamonau on 29 Jan 2020

@iparamonau We're logging in now.

kristoferbaxter on 29 Jan 2020

Notes

Agenda

UI and a11y WG Update
amp-Iframely component
CSS Size Limit Change

UI and a11y WG Update

Skipped since no members of the TSC or Approvers Working Group were present. Will distribute via comment on this issue.

amp-Iframely component

Wraps a iframe, aggregates 3rd party embeds
URL Preview for things not known to its systems
2000 Embedding Partners

On Layout Stability

When height needs to be change it requests it.
Uses attemptSizeChange
If rejected, this is not handled for when the iframe needs to be smaller.

Why not just an iframe?

Decoration
Differences in output for amp and non-amp implementations.
Needs additional permissions that amp-iframe doesn't permit. (allow-*, have to pass permissions to 3rd party origins).
Example: A Video Service needs Camera and Audio Permission.
Can we avoid allow-*? This grants all permissions indiscriminately.
amp-iframe has an attribute to set which permissions to allow to the iframe.
Would allow-* be permitted by validator?
Does AMP Iframe allow allow-*, allow-microphone, etc?
Feature policy when amp document is hosted inside an amp cache, the feature policy might restrict microphone and other sensitive permissions. This could lead to broken expectations.

Four items needing resolution before continuing:

Could this just use amp-iframe instead of a custom component? What issues does this solve that amp-iframe cannot easily accommodate.
Could AMP permit the allow-* value for the allow attribute of iframe? Would this work across all AMP environments, email, web, canonical document, embedded AMP document.
Would allow-* or similar pass validation?
Tag the security working group, facilitator is out on parental leave and we need to get someone else involved.

kristoferbaxter on 29 Jan 2020

Notes on relaxing CSS size limits

Why did we have a limit in the first place? To encourage CSS hygiene—hard to figure out when to remove CSS when they’re all globally scoped styles. AMP intentionally limited the number of bytes to 50,000, which was only one way to help with hygiene. This limit was the simplest to implement among options.
Why change this now? Today, more and more document authors are using either (1) generic CSS frameworks that load boilerplate CSS that may or may not apply to the document and (2) Bem syntax that hits our byte limit faster. Bem + object oriented CSS rationalize how CSS is being used in a document. If you use either of these approaches along with a CSS minifier, we still have a large number of documents that still cannot make it through the current size limit, many of which are complex and attract high levels of user engagement.
Impact of relaxing limits: The difference when CSS is compressed correctly and served from AMP Cache is 2KB of transfer after AMP's minification techniques — not a huge performance impact. By relaxing the limit somewhat, AMP still gets to enforce hygiene, just not as strictly.
How big is the change? The PR currently changes 55 LOC to validator across all format types. This is probably not needed for email (can be moved to a separate PR) since there are not many cases in AMP4Email that are concerned about the limit.
Alternatives considered:
- Keep existing limit: AMP can create a more general CSS minifier to help others do this — AMP should do this anyway, it can be done _along with_ rather than _instead of_ relaxing the limit.
- Adopting an adaptive limit: This would be a CSS limit based on document complexity (the more DOM nodes and more complicated structure => the more CSS allowed). The complexity of implementing this option is significant compared to current proposal. Every validator pass would need to inherit this new functionality. This was considered because of the assumption that DOM complexity is correlated to CSS size, which is untrue. Risk of opposite effect — publishers might be incentivized to add a billion DOM nodes to increase CSS limit, which hurts performance much more than the extra CSS to begin with.
How does CSS magnification handle AMP mustache/bind/etc?
Minification by Wordpress are aware of amp-bind, amp-list, other programmatic work within AMP, and not shake out features that would match combinatorial possibilities. This is not true for amp-script but Wordpress is working on this too. They don’t mingle class names for example, but a future minifier might be able to do this.
Is this size limitation increase needed proportionally across formats?
Not so much for email — people create custom CSS for AMP emails and don't often use other toolkits to create larger more complex CSS. Email authors began in a much more constrained place anyway and as a result there is not as much opportunity for abstraction. For stories, people are not hitting this limit significantly, but based off conversations it does happen.