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

amp-live-list: support amp-bind #7557

Closed
1 of 2 tasks
sebastianbenz opened this issue Feb 14, 2017 · 29 comments
Closed
1 of 2 tasks

amp-live-list: support amp-bind #7557

sebastianbenz opened this issue Feb 14, 2017 · 29 comments
Assignees

Comments

@sebastianbenz
Copy link
Contributor

sebastianbenz commented Feb 14, 2017

I'd be great if amp-live-list would support amp-bind. Two scenarios:

  • 1. amp-live-list could fire update events (<amp-live-list on="update:setState(liveBlogUpdated=true)" ...>). This be good to be able to update the elements in reaction to an update.
  • 2. evaluate bind expressions inside new amp-live-list content: e.g. if users could filter amp-live-list content, these filters need to be re-applied after live-list updates.

//cc @choumx

@jridgewell
Copy link
Contributor

/to @choumx, @erwinmombay

@dreamofabear
Copy link

/to @kmh287

@ericlindley-g
Copy link
Contributor

This seems useful, but I want to make sure I understand the value of doing this: @sebastianbenz — Since developers can specify updates to children of amp-live-list, what new use-cases would be supported by (1)?

Re: (2), is it possible to filter content now, or will this ability only be useful in the future when it is possible to filter?

@dreamofabear
Copy link

/to @sebastianbenz For Eric's questions above.

@sebastianbenz
Copy link
Contributor Author

One use case:

You can use amp-live-list to automatically update content on a page (by updating an item's timestamp). Together with a focus action (see #7626) it'd be possible to bring the updated element into the viewport, e.g. showing the last message in a message stream.

@ericlindley-g
Copy link
Contributor

Nice — sounds good to me :)

Also I could see this being used to create a custom interaction to update content, rather than strictly using the existing update button (if we also exposed the trigger to slot the new items into the page)

@cramforce
Copy link
Member

I'm a bit worried about people abusing this to run amp-bind on page load.

@dreamofabear
Copy link

Agree with Malte about (1) since AMP.setState() can affect elements outside of the <amp-live-list>.

We can start with (2) which is being implemented in #7990.

@ericlindley-g
Copy link
Contributor

Yikes — definitely agree with the risk here, and with the solution of starting with #7990

@sebastianbenz
Copy link
Contributor Author

I don't understand why this would enable running amp-bind on page load. I'd expect this event to only fire the first time an amp-live-list is updated, which is min 15s after page load.

@kmh287
Copy link
Contributor

kmh287 commented Mar 13, 2017

Could someone abuse 1 to periodically refresh bind attributes on an entire page?

@cramforce
Copy link
Member

Trying to figure out whether running this 15s after page load is better or worse :)

Could this be limited to only if live list found a delta? In that case it would be fine, I think.

@dreamofabear
Copy link

Reopening to track (1) in OP.

@dreamofabear dreamofabear reopened this Mar 20, 2017
@kmh287
Copy link
Contributor

kmh287 commented Mar 20, 2017

Sorry, my bad.

@ericlindley-g
Copy link
Contributor

Haven't checked in on this in a bit, but I'm assuming the event never fires without explicit user interaction (user taps the update button), as opposed to triggering when new data comes in. Is that the case?

@dreamofabear
Copy link

@ericlindley-g Current behavior post #7990 is that it does fire without user interaction, but only if there is an update. This admittedly does make me a bit nervous.

@ericlindley-g
Copy link
Contributor

Got it — I feel pretty strongly that we should only refresh bind attributes on explicit user action, unless we impose strict guidelines on what can be affected by these changes (no ads, no forms, no iframes, etc... — which I'm guessing isn't really feasible). WDYT?

Explicit user action would be tapping on a button
Scrolling down the page wouldn't count as an explicit user action
Swiping the carousel would be something in the middle (though we've essentially said it's okay for bind for now)

@dreamofabear
Copy link

SGTM. Would you mind filing an issue and cc'ing me and @kmh287?

@ericlindley-g
Copy link
Contributor

Will do. And sorry to come in late on this and cause churn, @choumx and @kmh287 —I really appreciate the change.

@kmh287
Copy link
Contributor

kmh287 commented Apr 3, 2017

Since we won't be going forward with 1, I'll close this issue.

@sebastianbenz
Copy link
Contributor Author

@ericlindley-g what are you concerns regarding automatically refreshing bind attributes with amp-live-list? What is this going to enable what's not already possible with amp-live-list?

@ericlindley-g
Copy link
Contributor

If I understand correctly, there are a few potential ways for amp-live-list updates to support amp-bind:

  1. Trigger bind events when new data comes in (rather than on user interaction with update button, which is already supported)

  2. Trigger updates to amp-state data when new amp-live-list updates come in

  3. Ensure that bindable elements inside of new amp-live-list items are properly bound, since they come in after page load

(2) & (3) don't pose any risk as far as I can see:
We definitely want to support (3), unless it's already supported by default ( @choumx , @kmh287 — is that the case, or do we need to build in support for this?).
(2) feels nice to have (though I'd be curious to hear what others think — if we're fetching data for amp-state at load time to better support e-comm, it might make sense to also enable this kind of polling for the freshest data after the initial load).

(1) is where risk comes in. I think there are a number of abuse cases, but the one that comes to mind immediately is loading an interstitial ad X seconds after page load, by just changing the CSS styling on a div that contains an iframe or amp-ad. It would take kind of a weird implementation on the backend to do this (publishing a page that every so often has a full-page ad), but there's still some amount of risk.

Though you also make a good point — maybe something similar is already possible with amp-live-list, even if it doesn't use amp-bind (like showing an interstitial ad * without * a close affordance). It's worth thinking more deeply about the core AMP principles, how we handle risks in relation to them, and how tradeoffs are evaluated.

@cramforce
Copy link
Member

cramforce commented Apr 4, 2017 via email

@ericlindley-g
Copy link
Contributor

@cramforce — SGTM :)

@choumx & @kmh287 — just to make sure, does amp-bind support #3 already, or do we need to build in additional support?

@kmh287
Copy link
Contributor

kmh287 commented Apr 12, 2017

@ericlindley-g Sorry I just saw this. 😅
#7990 introduced that functionality as part of work on this issue.

@ericlindley-g
Copy link
Contributor

Perfect—thanks @kmh287 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants