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.

Sign up for GitHub

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

Jump to bottom

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

Closed
alanorozco opened this issue Oct 2, 2019 · 17 comments
Closed

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

alanorozco opened this issue Oct 2, 2019 · 17 comments
Assignees
@dreamofabear
Labels
Component: amp-script P1: High Priority Type: Feature Request WG: runtime

Comments

@alanorozco
Copy link
Member

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.
@dreamofabear
Copy link

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.

@dreamofabear dreamofabear changed the title amp-state update from amp-script won't propagate updates to amp-list amp-script: Treat AMP.setState as "high trust" from user gesture Oct 2, 2019
@dreamofabear
Copy link

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).

@westonruter
Copy link
Member

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 westonruter mentioned this issue Oct 4, 2019
Closed
@dreamofabear
Copy link

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.

@dreamofabear dreamofabear mentioned this issue Oct 8, 2019
Closed
@mattwomple mattwomple mentioned this issue Oct 18, 2019
Closed
@meistudioli
Copy link
Contributor

meistudioli commented Oct 19, 2019 •
edited
Loading

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.

@dreamofabear
Copy link

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

@alastaircoote
Copy link

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!

@chasefinch
Copy link

chasefinch commented Nov 14, 2019 •
edited
Loading

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

@dreamofabear
Copy link

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

@chasefinch
Copy link

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?

@dreamofabear
Copy link

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.

@chasefinch
Copy link

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>

@dreamofabear
Copy link

Both.

The more subtle case is:

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

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

@chasefinch
Copy link

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.

@samouri samouri mentioned this issue Dec 12, 2019
Merged
@samouri
Copy link
Member

samouri commented Dec 16, 2019

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

@dreamofabear
Copy link

This supplemental semantic has yet to be implemented, though we could track that separately: #24862 (comment)

@samouri samouri mentioned this issue Dec 16, 2019
Merged
@dreamofabear dreamofabear reopened this Dec 16, 2019
@samouri samouri mentioned this issue Jan 16, 2020
Open
@jridgewell
Copy link
Contributor

Closed by #26046

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dreamofabear dreamofabear

Labels
Component: amp-script P1: High Priority Type: Feature Request WG: runtime
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@jridgewell @westonruter @alanorozco @alastaircoote @chasefinch @samouri @meistudioli @dreamofabear