Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I2I: Use aspect ratio CSS to implement responsive layout #30291

Open
3 of 4 tasks
dvoytenko opened this issue Sep 18, 2020 · 12 comments
Open
3 of 4 tasks

I2I: Use aspect ratio CSS to implement responsive layout #30291

dvoytenko opened this issue Sep 18, 2020 · 12 comments
Assignees
Labels
INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code Stale Inactive for one year or more

Comments

@dvoytenko
Copy link
Contributor

dvoytenko commented Sep 18, 2020

Summary

The new aspect-ratio CSS layout from CSS Sizing 4 is being actively worked on in Chrome and Firefox.

This CSS should eventually supersede layout=responsive. In the meantime, the proposal is to reimplement layout=responsive via aspect-ratio CSS where it's available.

See demos here.

Implementation brief

On the surface, the implementation is trivial:

if (layout == 'responsive') {
  if (CSS.supports('aspect-ratio: 1 / 1')) {
    element.style.aspectRatio = `${width} / ${height}`;
  } else {
    // The current code that injects a fake element.
  }
}

The situation is a lot more complex with AMP optimizer because there's no way to confirm the aspect-ratio CSS support on that level. This is still TBD, but one approach we can take:

<style>
@supports(aspect-ratio) {
  .i-amphtml-sizer-responsive {
    display: none !important;
  }
}
</style>

<amp-element style="aspect-ratio: 400/300;">
  <i-amphtml-sizer class="i-amphtml-sizer-responsive" ...></i-amphtml-sizer>
</amp-element>

In this approach, the optimizer will add both: new and legacy markup and we will control which one is used on the client side using the @supports() CSS.

Motivation

First, the aspect-ratio CSS layout is designed to be more universal. The layout=responsive is implemented via a fake element with the padding-top: {height/width * 100}% hack. This hack only works for bound-width case, such as normal block flow or flex-row layout. It doesn't work correctly for bound-height (e.g. flex-column) layouts and bound-width-and-height (position:absolute) layouts. aspect-ratio CSS will eventually support all three use cases.

Second, the aspect-ratio CSS layout will work better with out CSS constraints such as min-width and max-height.

Third, it's expected to be faster, since no fake elements are injected. In fact, eventually, aspect-ratio CSS will require no JS involvement at all. This would be a big benefit to CLS metric.

Fourth, in the Bento world, the fewer fake elements are injected the better. This is because any fake element will be visible to the user script and can confuse its behavior.

Launch

The following steps are necessary for launch:

  • Implement client-side support for a "raw" page.
  • Implement client-side support for an optimized page.
  • Implement client-side support for "intrinsic" layout.
  • Implement optimizer support.

/cc @ampproject/wg-approvers

@dvoytenko dvoytenko added the INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code label Sep 18, 2020
@dvoytenko dvoytenko self-assigned this Sep 18, 2020
@dvoytenko
Copy link
Contributor Author

The layout=intrinsic support "almost works" (see #30391). Unfortunately, there are a couple of inconsistencies between replaced and non-replaced intrinsic size treatment. See w3c/csswg-drafts#5549 and w3c/csswg-drafts#5537. These inconsistencies make backward-compatible support infeasible for the time being.

@dvoytenko
Copy link
Contributor Author

Aspect Ratio CSS has landed in Chrome 88.

@dvoytenko
Copy link
Contributor Author

Runtime launch is at 100%

@dvoytenko
Copy link
Contributor Author

Findings from the 1% launch.

In a wide majority of cases the aspect-ratio worked well in place of layout=responsive. However, we ran into issues with some pages specifying min-height and similar constraints on layout=responsive elements. The min-height, per Web spec, is applied to min-width automatically when an aspect-ratio is specified. You can see the difference in this demo (currently only works on a latest Chrome). In short: in the layout=responsive case the aspect ratio is applied first and the min-height is applied after. In the aspect-ratio case the min-height is applied first and the aspect ratio is applied after.

Possible ways forward:

  1. Force min-height: auto !important on layout=responsive elements. Counterpoint: this will break user CSS.
  2. Force min-height: auto !important, but allow opt-out via data-opt-out-from-new-aspect-ratio. Counterpoints: not backward compatible and page authors would have to discover discrepancies themselves.
  3. Create a new layout=aspect-ratio. It'd use the aspect-ratio when available and implement a partial polyfill using the same model layout=responsive. The browsers are implementing the new aspect-ratio aggressively and there's no full polyfill for this feature. So having a partial polyfill might be acceptable. Counterpoints: extra layout to support and page authors have to modify their pages.
  4. Do nothing and wait for page authors to start using the aspect-ratio themselves. Counterpoint: page authors would either wait for another ~2 years until they fill comfortable with coverage, or they'd use aspect-ratio without the fallback leaving pages completely broken on older browsers.

/cc @jridgewell @kristoferbaxter @caroqliu

@jridgewell
Copy link
Contributor

My vote is 2.

I actually think this is backwards compatible with layout=responsive, in that we can upgrade users from padding-top hack into apsect-ratio styles, and it will behave the same. If users want the additional min-height behavior of aspect-ratio, then they need to enable it with an attribute. This isn't a breaking change, because we've never supported this "min-height forces a min-width" behavior (you can't do that with padding-top).

@dvoytenko
Copy link
Contributor Author

The issue is that while we haven't explicitly communicated anywhere how min-height should would on layout=responsive, it had that behavior all along and it's hard to know now whether any particular element is using it intentionally (e.g. to break aspect ratio in some cases) or unintentionally (e.g. a bug).

@jridgewell
Copy link
Contributor

Ohhh, I mistakenly thought that forcing min-height: auto would make this behave the same as layout=responsive did (don't make width absurdly large), but of course it removes the min-height at small widths. Perhaps a better solution is max-width: 100% on these elements? I think only margin would cause an width overflow in this case.

@dvoytenko
Copy link
Contributor Author

I'll try max-width:100%, but I think your assessment is correct - it will overflow margins.

@samouri
Copy link
Member

samouri commented Oct 8, 2021

Were there any more updates here? Based on the difficulties above, my instinct is that we should do (4) now.

Do nothing and wait for page authors to start using the aspect-ratio themselves. Counterpoint: page authors would either wait for another ~2 years until they fill comfortable with coverage, or they'd use aspect-ratio without the fallback leaving pages completely broken on older browsers.

@samouri
Copy link
Member

samouri commented Oct 12, 2021

Based on a discussion with @dvoytenko, a potential path forward is to:

  1. Scrap the idea of implementing layout=responsive via aspect-ratio, since they have subtle incompatibilities.
  2. Create a new layout type layout=aspect-ratio which uses the css property when possible and essentially falls back to what responsive is today (i.e. sizer element etc).

Going to start by implementing (1) in #36318

@jridgewell
Copy link
Contributor

That sounds good to me.

@stale
Copy link

stale bot commented Oct 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code Stale Inactive for one year or more
Projects
None yet
Development

No branches or pull requests

3 participants