<amp-script> forbids mutations in a way that's confusing

[morsssss]

Preamble

amp.dev says:

The rules for mutations are as follows:

1. For amp-script elements with non-container layout, mutations are always allowed.
2. For amp-script elements with container layout, mutations are allowed for five seconds following a user gesture. This five second window is extended once if a fetch() is triggered.

I didn't find this behavior to be consistent, so I finally did some testing! The results lead me to believe that we may not be applying these rules consistently. Either we should change the documentation or the way we apply the rules.

(I haven't tested fetch() yet. But I did use setTimeout() and setInterval(func, 200) to see how long mutations were permitted.)

Tests

• with layout="fixed", dimensions set: setInterval() works forever on load or on user event
• with layout="fixed", dimensions not set: setInterval() works for 5 seconds on user event, but is disallowed on load. However, immediate mutations are allowed on load.
• with layout="responsive", dimensions not set: immediate mutation works on load. setInterval() fails on load. setInterval() works for 5 seconds on user event.
• with layout="responsive", dimensions set: setInterval() works forever on user event or on load.

I thought these would be the two possible cases: layout == "fixed" or layout != "fixed". But layout=="container" was a bit different:

• with layout="container", dimensions set or not: setInterval() disallowed on load, but works for 5 seconds on user event. immediate mutation allowed.

See also #26511
/cc @samouri

[morsssss]

Trying fetch, I get similar results to the above. Except:

with layout="responsive", dimensions not set: fetch() fails on load. fetch() works >5 seconds on user event.

[samouri]

Ah, so the actual two cases aren't fixed vs. !fixed, but container vs. !container. According to the layout docs, a few of the cases you listed should flatten into the same case. Specifically the "dimensions not set" cases are actually violations (and you should be seeing errors in the console for missing height/width), and those violations will fallback to layout=container.

Your discovery regarding initial mutations always being allowed is being addressed by #26401.

The only thing I'm left puzzled by is why setInterval would have any different handling in the layout=container case than the rest of the hydration-phase mutations.

[samouri]

Just used the AMP Playground to test this out. setInterval does work, but will cause worker-dom to terminate pretty quickly on its first mutations outside of hydration phase if layout=container.

If you either have the interval not perform mutation (e.g. console.log) OR give it a fixed layout, then setInterval won't cause the worker to be terminated.

[morsssss]

Yep, you're right - the cases do collapse:

layout=="container": immediate mutation allowed. setInterval() disallowed on load. setInterval() works for 5 seconds after user event.

layout!="container", dimensions set: immediate mutation allowed. setInterval() works forever on load or on user event.

layout!="container", dimensions not set: immediate mutation allowed. setInterval() fails on load. setInterval() works for 5 seconds on user event.

This is close to what's documented, but not quite the same. And I'm not sure whether it's the behavior we desire....

[morsssss]

Oh, and one more:

layout!="container", dimensions not set: fetch() fails on load. fetch() works >5 seconds (probably forever) on user event.

So, the question is, why do we allow some mutations forever, but others for 5 seconds?

[samouri]

I think explaining some of the rationale behind how/why the cases collapse can help (and why container is important here).

• layout==container elements can change their size after being laid out (dynamic sizing). Therefore we are restrictive about allowing mutations because we don't want it to cause objects to jump around.
• layout!=container elements will have fixed size after their first time being laid out (statical layout). Because of this, there are no restrictions and we always allow mutation.

With that rationale in mind, here are cases that collapse:

• layout==container, dimensions set: even though someone has written layout==container, this actually becomes a static layout. therefore mutations are allowed forever
• layout!=container, dimensions not set: dims need to be set for every layout!=container, or else it will fallback to container which has restrictions.

There are two things you called out that are inconsistent and we should get to the bottom of:

1. dynamically laid out elements should not be allowed to immediately mutate since that is inconsistent with what we intended. This is a bug we are addressing now.
2. setInterval and fetch() should both always succeed, regardless of user interaction. this is because they are not considered mutations. what might fail is if you are performing mutations in their callbacks.

[morsssss]

About #2 above: sorry, I began to use shorthand and was unclear! When I talk about setInterval() and fetch()... both of those APIs work. It's just that any mutations I tried to make after one of them failed.

Notice however that the behavior's always the same when layout=="container", whether dimensions are set there or not.

So, collapsing the cases:

layout=="container", or layout!="container" && dimensions not set: immediate mutation allowed. setInterval() disallowed on load. setInterval() works for 5 seconds after user event.

layout!="container" && dimensions set: immediate mutation allowed. setInterval() works forever on load or on user event.

The first case makes sense to me, if we document it clearly: if the element doesn't have fixed dimensions, we allow limited changes after a user event. Or on load, which, as you say, we don't actually want to do.

The second case also makes sense: if dimensions are fixed, do whatever you want. Although of course the developer could make all sorts of crazy things happen in a quite large space if they wanted to.

fetch() behavior is different, but I thiiiink we've documented that.

[samouri]

@morsssss: I think you've correctly collapsed the cases.

One more detail about what we intend to allow on load is that I think we want to allow for all non-visible mutations. Things like adding event listeners, and AMP.setState (since bind eval doesn't happen on load). But that conversation is going to happen in #26401.

Is there anything else we should get to the bottom of before closing this issue?

[AndrewKGuan]

Hi all,
it seems to me that mutation (setState) is not allowed on load inside any promise for container layout with/without dimensions? Here's a test page I put together:
https://funky-thorium.glitch.me/#development=1

[morsssss]

Ok, I did another thorough test here. This time, I put my results in a table so that they would make maximum sense. I don't have the patience to convert this to MD, so here's a lovely screenshot:

Arranging the results in the proper order makes the current state of things clear. We have a set of restrictions in place for these cases:

• layout == 'container'
• layout != 'container', dimensions not set

And no restrictions whatsoever in this case:

• layout != 'container', dimensions set

I think this is simple enough to keep the current behavior and document it, even though it's a littttle confusing. And I know we do want to change the onload behavior.

On the other hand, what @AndrewKGuan points out is concerning. Andrew, can you share your code? If so this should become a different issue.

[morsssss]

Amendment: as we've been discussing, we don't actually allow changes forever after a fetch() in variable-sized components. It's actually the time required for the fetch plus the usual 5 seconds.

[morsssss]

Ok, @samouri , two more things...

1. Thinking about this further... is it wise to require developers to specify dimensions in HTML instead of CSS? What about responsive design?

2. At @sebastianbenz 's suggestion, I tried layout="fill". It's actually more permissive than layout="container". We allow mutation forever on load even when dimensions aren't set. Maybe there's some logic to that, but if I have this right, it's now too complex for me to document or advise folks...

[morsssss]

also, @AndrewKGuan , you're right - state can't be set if layout=='container', whether dimensions are set or not. But if layout != 'container', it's allowed.

Which brings up again the question I saw in #26401 : should <amp-script> be allowing state changes that don't result in a DOM mutation?

[samouri]

For (1), it is that we simply can't figure out the desired height otherwise. Theres no way of directly seeing what the user specified in CSS for the HTML element.

For (2), thats because layout="fill" is also considered a layout with a defined size (static once laid out). The full list is here:

amphtml/src/layout.js

Lines 140 to 155 in 3199056

	/**
	* Whether an element with this layout inherently defines the size.
	* @param {!Layout} layout
	* @return {boolean}
	*/
	export function isLayoutSizeDefined(layout) {
	return (
	layout == Layout.FIXED ||
	layout == Layout.FIXED_HEIGHT ||
	layout == Layout.RESPONSIVE ||
	layout == Layout.FILL ||
	layout == Layout.FLEX_ITEM ||
	layout == Layout.FLUID ||
	layout == Layout.INTRINSIC
	);
	}

[morsssss]

1. I was hoping there was some way I didn't know of 😬 All I knew is that getMatchedCSSRules never made it to all browsers, and it's been deprecated.

Well, this could be a problem, but I've never heard anyone complain about it. We'll see if it becomes a problem.

2. About layout=="fill": looking above, I don't think I was clear. My question is, why do we allow our most permissive behavior for that layout type even when dimensions are not set? I don't think we allow that for other layout types.

[morsssss]

I'm going to go ahead and change the documentation to reflect what we've determined here. So, it's not layout=="container" vs everything else. It's layout != 'container' and dimensions set in HTML vs everything else.

I can close this issue and make a new one for the layout=="fill" case. I can see why this might have happened, but I don't think we should allow special behavior here, since it's not hard to abuse.