search

ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.88k stars 3.89k forks source link

Intent to implement: Viafoura embed #2442

Closed dougalg closed 7 years ago

dougalg commented 8 years ago

The proposed component would embed Viafoura's social widgets on a page inside an iframe. The widget would support all sizing options, and an embed-size postMessage event to handle resizing when necessary.

Tribune Publishing among other media clients of ours are currently developing an AMP implementation and would like to be able to include Viafoura widgets, such as our commenting widget, on their pages.

The primary data-attribute would be data-widget to specify the widget to implement.

Sample code to load the Viafoura commenting widget in amp:

<amp-viafoura
    layout="responsive"
    resizable
    data-widget="comments"
    height="600"
    width="500">
</amp-viafoura>

The widget would detect the domain, page URL, title, description, and image, and send that through the iframe along with any additional widget-specific parameters in order to load the correct commenting widget for the page.

Here is what our proposed implementation might look like: https://github.com/Tckool/amphtml/blob/amp-viafoura/extensions/amp-viafoura/amp-viafoura.md

cramforce commented 8 years ago

Lets definitely aim to find a way to implement this without an iframe.

1856 is currently in the works. Does viafoura provide value on top of this? If yes, could it be integrated into #1856?

dougalg commented 8 years ago

Thanks for the feedback. This proposed widget does not actually include our sharebar, and comprises a fair bit of extra logic. The idea is to expose the majority of our engagement tools in one amp component. I think your suggestion makes sense to use the amp-social-share component instead of our sharebar through a custom component. I will try to take a look to see exactly what functionality would be missing there that we might need and see where we can contribute.

Otherwise, in the context of this proposal, we are looking to provide primarily commenting support, but also top lists, and a variety of other components. I think commenting alone, not to mention top lists, are definitely outside the scope of the amp-social-share component. This component will provide our clients the ability to embed commenting on their AMP pages as they would on their desktop pages and does include a fair amount of functionality to support that (detecting a variety of title, image, description, etc. attributes) and is not meant to be only a sharebar (in fact we didn't plan to try to support the sharebar until we had better understanding of the amp-social-share project).

rudygalfi commented 8 years ago

cc @ericlindley-g who's been looking at comment widgets in AMP

cramforce commented 8 years ago

Makes sense. Do you provide any functionality where the comments could be embedded directly into the page and only on comment intent would we need an iframe?

dougalg commented 8 years ago

At the moment, we do not provide any functionality like that. The goal was to provide an interface for our social widgets that was similar to our regular implementation, but within the AMP framework. Ideally we would like to be able to provide our own component for client implementations.

dougalg commented 8 years ago

The reason that we have approached this as iframe-based is that it was the simplest implementation for our clients that preserves all existing functionality of our products.

We have quite a few clients (Tribune as I mentioned above, CBS Local, Hearst, LA Times, Gatehouse) who might be interested in AMP, but still want to be able to use our products with full functionality.

dougalg commented 8 years ago

I've submitted a pull request of our current working version so that you can take a closer look at what our vision is for this component, and provide more concrete feedback based on it.

dvoytenko commented 8 years ago

@dougalg Thanks again for the PR. I'm trying to catch up on this discussion and the widget itself. I found this URL and I believe it's your widget, right?

http://www.chicagotribune.com/news/local/breaking/ct-dennis-hastert-sentencing-delayed-met-20160323-story.html

I have few questions about it:

  1. It looks like it's not iframe'd but hosted inside of the parent page? Correct? I see @cramforce above asking about it. Could you please clarify this for me since PR is iframe-based?
  2. I noticed that the typical scenario is to show a button-looking section that says something like "10 comments" and when a user taps on it, it expands to show all the comments and the commenting form. Is this what the iframe intended to provide as well? I noticed that example gives the iframe a fairly big height.
  3. It looks like the comment form requires login (at least it's the case with Tribune). How do you see providing this from an iframe? The AMP itself provides the access protocol for publishers who wish to establish user identity and paywalls. So, this would have to be somehow reconciled, which I'm not sure how yet, so I'd like to hear a bit more about this.

Implementation wise, few quick notes:

/cc @ericlindley-g, @rudygalfi

dougalg commented 8 years ago

@dvoytenko thank you for taking the time to provide us with detailed feedback, it's very much appreciated!

Regarding your questions:

  1. The typical (non-amp) usecase would indeed be to embad the widgets on the parent page directly. However, for the AMP implementation, what we saw to be the simplest implementation for our clients would be to use an iframe.
  2. That implementation with a button that expands is a client-specific implementation. Although many of our clients use that functionality, it is equally common to just load the commenting widget on the page at page load time. Perhaps setting a maximum cap on the comment widget height for mobile would be considered an improvement?
  3. Yes, this is one of our concerns as well. If the commenting widget is embedded in an iframe we can of course provide logins within that iframe using our system, but anything more complex, like integrating with 3rd-party login systems or other amp-components would definitely pose problems. If it were possible to enable integrations with AMP logins between components, or if this is a goal that is currently under discussion, we would definitely be interested in joining that discussion.

Thanks for your implementation notes, we'll try to address those in the next week. For the sizing issue can we use something like amp-iframe as a guideline for how to properly implement resizing?

dvoytenko commented 8 years ago

@dougalg

Perhaps setting a maximum cap on the comment widget height for mobile would be considered an improvement?

Do you suggest that a page author can set the initial height and based on that your widget would determine whether to show a button or a list of comments right away? If so, I think it'd be ok, although I think it'd be better to simply define an attribute that would instruct the widget to do one or the other, e.g. show=button vs show=comments.

If the commenting widget is embedded in an iframe we can of course provide logins within that iframe using our system

In the meantime, what login system do you normally employ? Do you leave it up to publisher to implement and simply callback to it? Do you have your own identity system? This promises to definitely be tricky so we need to focus on it in a greater detail.

For the sizing issue can we use something like amp-iframe as a guideline for how to properly implement resizing?

We'll definitely do that. You can just copy-paste that code from amp-iframe and I think we'll get a chance to refactor it soon after.

orzage commented 8 years ago

@dougalg

The button version is an important use case for those looking to keep their pages lean by avoiding loading the commenting widget's resources at all if the user is not interested in the comments. With AMP, clients presumably do not have the option of providing their own implementation of a button that loads the amp-viafoura iframe when clicked, avoiding loading the iframe until the user clicks, so it would be nice to include that as @dvoytenko suggests.

dougalg commented 8 years ago

@dvoytenko @orzage Thanks for the continued detailed feedback. I think your idea to add an option to show a button which to hides/shows the comments certainly makes a lot of sense. My only concern is about internationalization. Is there any standard at the moment for internationalization in the AMP project?

For the login, we do provide our own login system which integrates social logins, as well as an option for clients to call back to our system and use their own custom login. What kind of issues do you foresee causing problems with the login?

dvoytenko commented 8 years ago

@dougalg How do you approach internationalization today? Do you simply leave it to the publisher or do you internationalize yourself? I think we should be able to extend lang propagation to your widget from the page at large. But if you prefer publisher to do internationalization - we can then make it part of the markup. E.g. the iframe (possibly preloaded) will not be shown until the button in the document is clicked.

I don't really anticipate issues with publisher login propagation since they would have to opt-in into that and thus, essentially, would give the approval to use this information. But there are couple of other things that concern me:

  1. Publisher would have to also propagate, at the very least, the user's name. Unless you ask user to enter the alias at all times.
  2. Any kind of identification of the user (ID) ideally would be transformed via a one-way hash, which would allow correlation on publisher's side, but would prevent unnecessary ID propagation otherwise.

With your own identity system, the issues are a bit more complicated. Since you will be deployed as a 3rd-party iframe, you won't be able to set cookies in many environments. I'm not sure if this is an issue for you today and if so, how you work it around. Please provide some detail. We can in theory extend our existing access protocol to you to mitigate these issues, but I need to see how this can be done.

dougalg commented 8 years ago

@dvoytenko Sorry for the long delay in answering.

For internationalization, we currently provide our own implementation, which will work fine for the commenting tool in the iframe, but I'm not sure how that might work in the context of an AMP component itself. ie: if we implement a button to show/hide commenting, what would the internationalization solution for that button's text look like? I guess it could just be specified by the client eg:

<amp-viafoura ...>Click to show comments</amp-viafoura>

As for the identification issue, I'd like to take some time to look into how you're implementing your access protocol. The problems with cookies will certainly cause problems as we do use a 24-hour cookie to maintain login status with the user. Of course this will have issues with some browsers or user settings profiles.

For client login propagation, we currently support a system whereby the publisher can provide a name, email address, and avatar to log a user in to our system by way of an api endpoint accessible with a client provided cookie. I'm not sure yet exactly what this integration would look like in AMP, but we would most likely need to look into other solutions, but as you say, they ought to be fairly straightforward.

dvoytenko commented 8 years ago

@dougalg Apologies, missed this response. Let me run through your points:

What would the internationalization solution for that button's text look like?

I think something like this could work:

<amp-viafoura ...>
  <button show-comments-button>Click to show comments</button>
</amp-viafoura>

We can explore options how to inject number of comments into the title if you'd like. This will completely gets us off the hook with internationalization, accessibility and exact design by leaving this to the publisher. WDYT?

As for the identification issue, I'd like to take some time to look into how you're implementing your access protocol.

Please do and let me know if you have any questions I should clarify. https://github.com/ampproject/amphtml/blob/master/extensions/amp-access/amp-access.md

The problems with cookies will certainly cause problems as we do use a 24-hour cookie to maintain login status with the user.

We can give you a special-use session identifier for the user - this is how AMP access works in general. Please take a look at that spec and let's discuss.

For client login propagation, we currently support a system whereby the publisher can provide a name, email address, and avatar to log a user in to our system by way of an api endpoint accessible with a client provided cookie.

We can do it, but the complication for us is to ensure that we limit where it's done. It's important that we only share this data when we can confirm the user's consent/intent to allow this sharing - i.e. based on the the user action (e.g. user clicks on "make a comment" button). This would be ideal for us. Obviously with an iframe, confirming the user action is a bit complicated, but we should be able to find a way. But to start with, could you please describe how you arrange it today? Do you automatically share identity without "consent" or have some other approach to this?

adelinamart commented 7 years ago

What's the latest status of this one?

dougalg commented 7 years ago

@adelinamart, thank you for your interest. Unfortunately, we've had to put our AMP integration project on hold for the time being. I will close this intent to implement for now, and re-open it later when we have time to work on it. Thank you and @dvoytenko for all the valuable feedback. We will definitely take it into consideration for our next pass at the component.