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-iframe: Add support for message protocols of popular libraries that do resizable iframes #22714

Closed
westonruter opened this issue Jun 5, 2019 · 19 comments · Fixed by #24917
Closed

amp-iframe: Add support for message protocols of popular libraries that do resizable iframes #22714

westonruter opened this issue Jun 5, 2019 · 19 comments · Fixed by #24917
Assignees
@westonruter
Labels
P3: When Possible Type: Feature Request WG: runtime

Comments

@westonruter
Copy link
Member

Many sites use libraries like Pym.js to implement resizable iframes. In AMP, amp-iframe has its own message protocol for implementing resizable iframes (#728).

Currently, in order to take existing legacy content and put it into AMP pages, they have to be updated to use the the AMP-specific message protocol. It would be great if amp-iframe could be made aware of the protocols of popular resizable iframe libraries (like Pym.js), as this would facilitate migration to AMP.

This was proposed by @thomaswilburn in nprapps/pym.js#182 (comment):

Alternatively, if the AMP project actually cares about working with the community, perhaps they should implement compatibility with legacy Pym messages. There are a lot of static graphics generated with tools like our Dailygraphics Rig that are not going to be upgraded to a new Pym or Sidechain, because it's not reasonable to dig all of them up and republish them. Having AMP support the news industry's de-facto embedding standard would be a real sign of goodwill.

For more historical context: #495
@westonruter westonruter mentioned this issue Jun 5, 2019
Closed
@westonruter
Copy link
Member Author

/cc @aghassemi

@amedina
Copy link
Member

amedina commented Jun 5, 2019

/cc @cramforce @pbakaus @dvoytenko @rudygalfi

@cramforce
Copy link
Member

We've made changes like this in the past.

How does their protocol look like? I couldn't quickly find docs.

@westonruter
Copy link
Member Author

Messages are serialized into a string, prefixed by pym with arguments joined by the delimiter xPYMx:

https://github.com/nprapps/pym.js/blob/57feb680ac3ff7aeef080e5efe0ddbe665530eac/src/pym.js#L85-L102

So a message to update the height of the FOO iframe to 300 pixels, I believe would look like:

pymFOOxPYMxheightxPYMx300

@dvoytenko
Copy link
Contributor

IMHO, there's no problem to support 1-2 very common resize message formats. But I'd stay away from using the libraries themselves - they often have their own legacy overhead.

@westonruter
Copy link
Member Author

Agreed. Support the protocols but don't incorporate the libraries.

@cramforce
Copy link
Member

Weird flex but OK :)

I had never heard of Pym. How common is it?

@thomaswilburn
Copy link

It's probably the most common solution used by newsroom interactive teams today. It's used at NPR, almost all of our members stations, as well as (I think) the Texas Tribune, St. Louis Post-Dispatch, INN-supported newsrooms, and many others.

The message format isn't great, but we don't have the ability to change it now, it was implemented long before I joined the team. We have introduced a new Pym- and AMP-compatible solution called Sidechain, but it isn't in production use yet. And due to the way that we publish graphics to static file hosting, it's not feasible to go back and upgrade them all.

@thomaswilburn
Copy link

For context, our CDN distribution for pym.nprapps.org, which is used by us and by other people who embed with this script, sees about 2.5-4M requests per day. That's not the complete story, as many users probably have their own customized version or they bundle it in from the NPM package, but it's not nothing either.

@jeffersonrabb
Copy link

Pym.js/AMP support has also been requested by a few publications in the Newspack world, including Reveal, The Rivard Report, The Chicago Reporter, and The Hechinger Report.

@benlk
Copy link

benlk commented Jun 5, 2019 •
edited
Loading

INN maintains a plugin that wraps a Pym.js embed in a shortcode or block: https://github.com/INN/pym-shortcode https://wordpress.org/plugins/pym-shortcode/

If there's something we can do to make that work better with the WordPress AMP plugin, please chime in on the Pym.js Embeds plugin's AMP support issue: Automattic/pym-shortcode#59

(and if there's stuff that needs to be done to make the plugin compatible with Newspack, we'd like to know so we can get that implemented, too.)

@cramforce
Copy link
Member

Ideally, sender would be updated to send both this and other formats.

if that is infeasible, I think it is fine to add support for commonly used message formats on the receiver end.

@dvoytenko
Copy link
Contributor

PYM support is something we considered some time ago. Just didn't get to it. One point of disconnect: I don't believe PYM supports the "rejection" signal when we deny resize. That was a bit of a concern. Not an issue for user-gesture-based resizing, but definitely could be an issue for onload.

@amedina
Copy link
Member

amedina commented Jun 13, 2019

@dvoytenko would this be a blocker? Or can we move to add protocol support to make libraries like Pym.js AMP compatible?

@philipjohn philipjohn mentioned this issue Jun 18, 2019
Closed
4 tasks
@aghassemi aghassemi mentioned this issue Jun 21, 2019
Closed
6 tasks
@dvoytenko dvoytenko assigned westonruter and unassigned dvoytenko Jul 3, 2019
@dvoytenko
Copy link
Contributor

@westonruter No, I don't think it'd be a blocker. We could always follow the same protocol and send "rejection" message as well, in case anyone can react to it.

@westonruter westonruter mentioned this issue Sep 18, 2019
Merged
@westonruter westonruter mentioned this issue Oct 5, 2019
Merged
@westonruter
Copy link
Member Author

@dvoytenko Finally opened a PR: #24917.

@westonruter
Copy link
Member Author

IMHO, there's no problem to support 1-2 very common resize message formats. But I'd stay away from using the libraries themselves - they often have their own legacy overhead.

I just listened to @calebcordry's talk just now about an “Sustaining an ever-growing AMP component library” at the AMP Contributors Summit; it got me thinking about whether there should be a separate WordPress embed component (#18378, amp-wordpress-embed) as opposed to extending amp-iframe. In the PR for augmenting amp-iframe to support Pym.js messages (#24917), the logic is essentially identical to the core of the amp-wordpress-embed being proposed in #24952.

Instead of listening for message events with data containing Pym.js serialized string, the amp-wordpress-embed is listening for message events containing data that has an object with keys for message and value, where message can either be height or link. The latter one is the unique one for WordPress, as instead of requiring theallow-top-navigation-by-user-activation sandbox keyword, WordPress sends a link message with the desired URL and then the parent window only follows the URL if it with the same origin as the window being embedded.

All that to say, the logic needed for WordPress embeds is similar to Pym.js, though it does have the additional facility for navigation. Does this make WordPress embeds warrant having a separate extension?

@dvoytenko
Copy link
Contributor

@westonruter I think we should view pym support and the amp-wordpress-embed separately for now, b/c pym is used in non-wordpress embeds as well, right? Otherwise, imho, you should take for amp-wordpress-embed to design review.

@newsroomdev
Copy link

Very excited for #24917 @dvoytenko @westonruter. This could fix hundreds of pages on Axios, thousands if you include the previously mentioned news sites 👏🎉📈

alanorozco pushed a commit that referenced this issue Feb 6, 2020
@westonruter
✨Add amp-iframe support for Pym.js width and height resize messages (#…
39b1f61 
…24917)

* Add amp-iframe support for Pym.js width and height resize messages

Fixes #22714.

Pym.js supports the client window sending several types of messages, including:

* `height`
* `width`
* `parentPositionInfo`
* `navigateTo`
* `scrollToChildPos`

The only two messages that seem relevant in an AMP context are width and height, as they map to the embed-size message that AMP is currently looking for.

* Use listen event-helper instead of addEventListner directly

* Rename unlisten_ to unlistenPym_

* Use this.win instead of window

* Indicate event param is non-nullable MessageEvent

* Use getData event helper

* Warn user about unsupported Pym.js messages

* Replace function binding with arrow function to fix JSC_TYPE_MISMATCH

* Add test for Pym.js messages for height and width

* Add missing tag argument to user().warn()

* Use startsWith string helper

* Replace array splice with direct references; fix format comment doc

* Add missing ID from Pym message

* Populate undefined variable sentinel
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@westonruter westonruter

Labels
P3: When Possible Type: Feature Request WG: runtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

✨Add amp-iframe support for Pym.js width and height resize messages westonruter/amphtml
10 participants
@cramforce @westonruter @amedina @dvoytenko @thomaswilburn @jeffersonrabb @newsroomdev @benlk @aghassemi @dreamofabear