Amphtml: amp-script: Treat AMP.setState as "high trust" from user gesture

Created on 2 Oct 2019 · 17Comments · Source: ampproject/amphtml

With the following script handler:

document.getElementById("add-script").addEventListener("click", () => {
  AMP.getState("items").then(serializedItems => {
    const items = JSON.parse(serializedItems);
    AMP.setState({ items: items.concat("item " + items.length) });
  });
});

Error case here. amp-bind updates propagate into the list (with state added per script), but amp-script changing the state won't cause the list to re-render.

amp-script High Priority Feature Request runtime

Source

alanorozco

Most helpful comment

Addendum: If the AMP.setState is not from a user gesture, allow mutation of amp-script's subtree if it allows arbitrary mutation (fixed-layout and < 300px height).

choumx on 2 Oct 2019

👍2

All 17 comments

This is because AMP.setState in worker JS is currently treated as a "low trust" action. However, we probably can hook into amp-script's gesture detection mechanism to support this.

choumx on 2 Oct 2019

Addendum: If the AMP.setState is not from a user gesture, allow mutation of amp-script's subtree if it allows arbitrary mutation (fixed-layout and < 300px height).

choumx on 2 Oct 2019

👍2

Addendum: If the AMP.setState is not from a user gesture, allow mutation of amp-script's subtree if it allows arbitrary mutation (fixed-layout and < 300px height).

But setting state inherently can affect elements _outside_ the amp-script subtree, no? Consider the case of adding a dark mode toggle in #20951. At page load should amp-script be able to access localStorage to obtain the user preference and then call AMP.setState({"mode":"dark"}) which then can then mutate some element outside the amp-script subtree, like mutating a class on the body:

<!-- ... -->
<body class="mode-light" [class]="'mode-' + mode">
    <amp-state id="mode"><script type="application/json">"light"</script></amp-state>
    <!-- ... -->

That seems very useful but it also seems somewhat perilous as setting page state could allow endless content shifting opportunities.

westonruter on 4 Oct 2019

Yea, the idea is to only mutate the DOM of the amp-script element. amp-list does something similar on render -- only evaluates bindings in children.

choumx on 4 Oct 2019

Addendum: If the AMP.setState is not from a user gesture, allow mutation of amp-script's subtree if it allows arbitrary mutation (fixed-layout and < 300px height).

@choumx :

Does that mean amp-bind will mutate once fulfill requests fixed-layout and < 300px height ?

<amp-script width="300" height="100" script="local-script">
  <p [text]="status.cat">cat name</p>
</amp-script>
<script type="text/plain" target="amp-script" id="local-script">
  ...
  AMP.setState({
    status:{
      cat: 'shadow'
    }
  });
</script>

But above case doesn't mutate.

meistudioli on 19 Oct 2019

Right, this issue is tracking support for that. It still needs to be implemented.

choumx on 21 Oct 2019

👍1

Yea, the idea is to only mutate the DOM of the amp-script element.

It might be worth clarifying this in the docs. Right now they say:

This enables advanced interactions between amp-script and other AMP elements on the page via amp-bind bindings. These elements can be inside (descendants) or outside (non-descendants) of the amp-script element.

Was wondering why my code manipulating state for elements outside of amp-script wasn't working!

alastaircoote on 28 Oct 2019

👍1

@choumx, is @alastaircoote's suggestion accurate, that the docs are incorrect?

chasefinch on 14 Nov 2019

Yes, the documentation is misleading. I can quickly fix the problematic sentence.

choumx on 14 Nov 2019

I see. Out of curiosity, what's to stop someone from wrapping most of the document in a single amp-script so that all of the relevant components will be updated on setState?

chasefinch on 14 Nov 2019

As far as AMP.setState and DOM mutation is concerned, it won't matter whether a component is inside or outside of an amp-script. What matters is whether the AMP.setState is triggered as a result of a user gesture or not.

Ignoring AMP.setState though, one could create an AMP page as a single large amp-script element.

choumx on 14 Nov 2019

Thanks for the info, I almost understand. Could you clarify which of #p1 and #p2 in the user-gesture-based example below will display "Shadow" once this fix is implemented?

<p id="p1" [text]="status.cat">cat name</p>

<amp-script script="local-script">
  <p id="p2" [text]="status.cat">cat name</p>
  <button id="button">Update cat name</button>
</amp-script>

<script type="text/plain" target="amp-script" id="local-script">
  ...
  document.querySelector(".button").addEventListener("click", () => {
    AMP.setState({
      status: {
        cat: 'Shadow'
      }
    });
  });
</script>

chasefinch on 14 Nov 2019

Both.

The more subtle case is:

If AMP.setState is _not_ backed by user gesture, AND
The amp-script element is fixed layout with height < 300px

Then: allow DOM mutation of only amp-script children anyways.

choumx on 14 Nov 2019

Got it. The documentation is probably clear enough as-is then, I got tripped up by some of the earlier discussion in the thread.

Currently using setState in amp-script anyway (AMP.setState({aKey: aValue})), and working around this bug by specifying on="tap:AMP.setState({aKey: aKey})" to the next logical button for the user to tap (in my case it's a close button), which refreshes all bindings that depend on aKey document-wide.

chasefinch on 14 Nov 2019

👍1

@choumx: I think you can close this now that #26001 is submitted.

samouri on 16 Dec 2019

This supplemental semantic has yet to be implemented, though we could track that separately: https://github.com/ampproject/amphtml/issues/24862#issuecomment-554040403

choumx on 16 Dec 2019

Closed by #26046