Allowing different aspect ratios on base-component

[nitzanvolman]

The current implementation assumes that in a responsive scenario the aspect ratio of a component remains the same across all the responsive breakpoints. This behaviour may not always be desired as in some cases a different aspect ratio may be needed for different screen sizes.
This pull request adds a "aspects" attribute to the base component, which accepts a series of media queries and a desired aspect ratio in % units (similar to the sizes css attribute)
See: examples/responsive.amp.html which was also added as a part of this request

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[cramforce]

Nice! I do, however, think we should stay as close to standards as possible (and generally implement them by the book).

Did you see that all AMP-tags support the media attribute?
https://github.com/ampproject/amphtml/blob/master/spec/amp-html-layout.md#media

This can be used to do what this PR implements. Its main problem is that mutual exclusion has to be handled by the author.

For this PR, I think the right way to more forward would be to implement amp-picture instead, since that seems to be the standards based mechanism to achieve the art direction desired here. See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/picture

[cramforce]

Thinking about this some more, I'd really like feedback on the media approach. It would definitely be nice to get rid of the redundancy where the only thing changing is aspect ratio.

[nitzanvolman]

Apart from creating bloated code,
the media approach will require a "refresh" when changing the screen width across a media "breakpoint" when switching one element to replace the other - which is undesired.
The refresh will hurt the experience, and may require some de-duping when multiple refreshes are made on a singe page view.

[dvoytenko]

I'd second that the question about the media. This is what we planned for different aspect ratios. It follows the picture element protocol in standards.

Ads is slightly a special case because iframe can actually have code that reacts to size changes. But other elements do not. For instance, in case of an image - one simply has to use a different image source.

One negative from media in ads is that AMP would switch to new ad element (and likely new ad entirely), but I'm not sure if this is indeed a valid use-case or a valid concern.

With that in mind, could you please clarify what you mean by "refresh" above?

[nitzanvolman]

I could have explained myself better, sorry about that.
Using the media attribute we expect AMP to hide one component and show another when changing the display width. (such as changing the orientation of a phone)
For the user this will appear as a refresh - which is what i meant... (which is actually seeing a different component added to the viewport and rendered)

Our use case is similar to that of amp-ad, and as @dvoytenko suggested this will cause a completely new ad to be presented to the user, which is undesirable for many reasons.
(for example: impressions are doubled.)

[cramforce]

I understand. I'm very open to adding support for <source > tags to all kinds of amp tags. That would work well for images (it would be a picture polyfill) and I think it could be also made to work for amp-iframe and similar components. If I'm not mistaken your goal could also be expressed in CSS.

[nitzanvolman]

Thanks for answering Malte,
Our requirement is to change the aspect ratio of an existing element, that may be already rendered, and already has an internal state we wish to maintain.
Could you explain how adding support for <source..> would help? or how we can achieve this in css?

[cramforce]

For ads it is actually often desirable to load a different ad on different screen resolution. E.g. desktop ads often sell for higher prices, and so one wants to switch to a different "slot id" or whatever terminology the ad network uses.

I believe that CSS standard media queries should work on AMP elements. In particular when they use layout=fill.

There are really 3 cases here, that people want to support

1. Load a different element (desktop Vs. mobile ad)
2. Load a different sources into same element (same image at different aspect ratio). This is what the picture element is for.
3. Change the aspect ration of existing element which can handle the reflow itself.

(3) is what is proposed here. I'd like us to see whether this can just be achieved in CSS. If it cannot or if that seems overly awkward, we need to do research what the standard answer in responsive design is and implement that in as standardized and idiomatic fashion as possible.

[nitzanvolman]

Exactly! option (3) is what we are after, and i agree completely that it's best to do this in css, if possible.
If not, it would be great to find another way to achieve this.

[dvoytenko]

Yeah, I definitely see the benefit. This directly addresses two rather ingrained problems in CSS:

1. It's not good with height management
2. It's not good with aspect ratios

The proposed solution also seems fairly clean to me, although expressing aspect ratios via media query seems a bit counterintuitive to me here. Feels like it's best to design aspect ratio based on the width breakpoints of the element itself.

So, I'm wondering if one of these options would work better:

1. Make aspects format more inline with srcset (but not the same format - it's pretty horrible). Basically, we express something like this: 500w -> 56%, 300w -> 70%, 80%.
2. Combine aspects with sizes. E.g. instead of sizes="(min-width: 500px) 320px" we could do sizes="(min-width: 500px) 320px 56%". I.e. the additional second parameter would indicate height % or value. This is a bit cryptic, but nice b/c it's backward compatible with sizes in HTML5.

[Effie]

We like option (1), especially because it allows the page author to use an element query instead of media query, which make expression of certain layouts easier (like having a responsive element on 50% of the page width)
it is unclear what you mean by the "w" unit though.

Option (2) can also work, if we can express the width in percentage and not only absolute values, like:

sizes="(min-width: 500px) 100% 56%"

since our widget occupies 100% of the available width.

We can take a crack at implementing option (1) assuming we can use a 'px' unit and and create a syntax like so:

aspects="1000px 30%, 500px 50%, 80%"

@dvoytenko is this what you meant?

[dvoytenko]

@Effie Yes. This is pretty much what I meant. I took w from srcset spec where it indicates width; one nuance with "px" above is that it can only be "px" for width - otherwise we'd have rather hard time calculating other units. I think this aspects format is good. Let me check few more things with it.

Option (2) can also work, if we can express the width in percentage and not only absolute values

We can't use % in sizes for width however - it's not allowed by the sizes spec. But since we are gravitating to option (1), we can skip this for now.

[dvoytenko]

@nitzanvolman @Effie I got a lot of pushback on the proposed format. The main problem is that we can't support static layout fast with it. Instead, I'd like to come back to the original idea of defining aspects via media query. I do think it should support your case still pretty well (in part because that's what you already proposed and I set you off track), but please let me know. In this case, we will do the following steps:

1. We will allow parseSizeList to accept % values just in this case.
2. We will add attribute aspects or heights (this we need to decide, see below). It would like exactly like sizes with the only exception that % are allowed. E.g. heights="(min-width: 320px) 50px, 50%".

On aspects vs heights. Since we will allow not only % values but also px and other units, I think a better name would be "heights". It will be pretty symmetric to "sizes" which actually really means "widths".

Let me know what you think. If this is good, I think this PR is very close.

[nitzanvolman]

@dvoytenko I think your suggestion is good.
I think the "height" name works well in this context, and supporting several unit types makes lots of sense.
Please let us know how you want to proceed, and if you'd like us to implement this.

[googlebot]

CLAs look good, thanks!

[dvoytenko]

@nitzanvolman Sure. Please go ahead and implement. So, decisions wise:

1. Let's call it "heights"
2. Let's modify parseSizeList to allow % explicitly and only do that for "heights" attribute, NOT for "sizes".
3. I think your intended implementation already looks good. But one request: in the applyLayout_ method where all asserts are made, let's assert that heights can only be used for layout=responsive (not inputLayout, but layout).

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[nitzanvolman]

@dvoytenko I believe this is it, please review and let us know if there is anything we missed.
btw, I've been having trouble with the google bot in this PR and in others, I followed the instructions on https://cla.developers.google.com/ (which now returns a 500 error) but can't get it be green.
not sure what else to do about it.

[dvoytenko]

This should change to heights, right?

[dvoytenko]

This looks great! Just a few smallish comments. Please also add tests for assertLengthOrPercent, SizesList with percent and custom-element changes. The first two should be pretty straightforward, and for the last one - please look for tests for sizes attribute, that should point the way.

Thanks a lot!

[dvoytenko]

boolean=

[dvoytenko]

All looks good! Just one minor comment. And please squash the commits. I will fix the Travis in the meantime.

[nitzanvolman]

rebased again, squashed, and added =
let me know if you see anything else we missed.

[dvoytenko]

@nitzanvolman could you please give CLA another chance? I think it was broken and now it's fixed. Let me know if it doesn't let you sign again.

[nitzanvolman]

You must be an owner of the contributors group in order to submit this CLA.

This will require help from my IT guys. Will try to get this done tomorrow when it's daylight...
(I previously completed this and it didn't give me such a hard time...)

[nitzanvolman]

@dvoytenko I singed the CLA again - should be ok now.

[dvoytenko]

@nitzanvolman I got to the bottom of the CLA problem, I believe. The commit belongs to @Effie and the pull request belongs to you. So, I think we can either have Effie sign CLA as well or resend the PR with commit under your name.

[nitzanvolman]

@dvoytenko I'm not sure technically how @Effie should sign, note that we are contributing this on behalf of a cooperation, in which case the instructions are to list the authorized signer and the primary contact person on the form, and include all of the committers of that cooperation in a dedicated google apps group (which we did: amp at taboola.com)
Will it help if @Effie replies to this thread with I signed it! ?

[dvoytenko]

Yes, could you please ask him to do this?

[willnorris]

No, having Effie comment isn't going to do anything. It's still going to have trouble finding Nitzan's CLA because of a bug (Googlers: b/23383092), but I've manually verified both and this is fine to merge. I'll try and get that bug fixed next week so that future collaborative PRs like this don't get stuck again.

[dvoytenko]

Awesome, thanks, @willnorris !

[nitzanvolman]

thanks @willnorris and @dvoytenko

[dvoytenko]

All merged! Thanks @nitzanvolman !