✨Add amp-iframe support for Pym.js width and height resize messages #24917

westonruter · 2019-10-05T06:24:49Z

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.

Demo video (YouTube):

The code can be tested using this Glitch: https://amp-iframe-pymjs-resizable.glitch.me/

westonruter · 2019-10-05T06:27:12Z

extensions/amp-iframe/0.1/amp-iframe.js

@@ -466,6 +466,8 @@ export class AmpIframe extends AMP.BaseElement {
      /*opt_allowOpaqueOrigin*/ true
    );

+    addEventListener('message', this.listenForPymMessage_.bind(this));


This seems not ideal. There should rather be one single message event handler that all of the iframes share.

It seems rather that listenFor should be used, but the problem is that the Pym.js event.data is a string and not an object with any sentinel. So there isn't an easy way to integrate with this existing event listener system.

westonruter · 2019-10-05T06:56:22Z

extensions/amp-iframe/0.1/amp-iframe.js

+      this.updateSize_(parseInt(args[1], 10), undefined);
+    } else if ('width' === args[0]) {
+      this.updateSize_(undefined, parseInt(args[1], 10));
+    }


Perhaps a message should be logged out for other message types so that a developer doesn't get confused about why it is not working.

Sure. We could do a user().warn() for this.

Done in 197c110

extensions/amp-iframe/0.1/amp-iframe.js

dvoytenko

Looks good. Couple minor comments. And this will need tests.

dvoytenko · 2019-10-16T23:08:59Z

extensions/amp-iframe/0.1/amp-iframe.js

@@ -103,6 +103,9 @@ export class AmpIframe extends AMP.BaseElement {
    /** @private {string} */
    this.sandbox_ = '';

+    /** @private {Function} */
+    this.unlisten_ = null;


Let's name it unlistenPym_.

Done in 0b31cca

dvoytenko · 2019-10-16T23:09:17Z

extensions/amp-iframe/0.1/amp-iframe.js

@@ -466,6 +469,13 @@ export class AmpIframe extends AMP.BaseElement {
      /*opt_allowOpaqueOrigin*/ true
    );

+    // Listen for resize messages sent by Pym.js.
+    this.unlisten_ = listen(
+      window,


s/window/this.win

Done in 89dcadb

dvoytenko · 2019-10-16T23:10:11Z

extensions/amp-iframe/0.1/amp-iframe.js

+   * Listen for Pym.js messages for 'height' and 'width'.
+   *
+   * @see http://blog.apps.npr.org/pym.js/
+   * @param {MessageEvent} event


!MessageEvent

Done in 8dce048 and 761267d

dvoytenko · 2019-10-16T23:11:53Z

extensions/amp-iframe/0.1/amp-iframe.js

+      this.updateSize_(parseInt(args[1], 10), undefined);
+    } else if ('width' === args[0]) {
+      this.updateSize_(undefined, parseInt(args[1], 10));
+    }


Sure. We could do a user().warn() for this.

westonruter · 2019-11-21T21:28:45Z

And this will need tests.

Tests added in 3a726ba.

westonruter · 2019-11-21T22:10:04Z

I'm not sure why the tests are failing, as the failures don't seem related to my code. Nevertheless, I wasn't able to get the tests working locally either (even on master).

Cf. Slack thread

dvoytenko

Code LGTM. The test failures seem superficially relevant. But could be something else. I'll take a look as well.

extensions/amp-iframe/0.1/test/test-amp-iframe.js

westonruter · 2019-11-23T01:26:36Z

test/fixtures/served/iframe.html

+    } else if (event.data.type === 'requestPymjsHeight') {
+      var msg = [ 'pym', 'height', event.data.height ].join( 'xPYMx' );
+      getAmpWindow(sentinel)./*OK*/postMessage( msg, '*');
+    } else if (event.data.type === 'requestPymjsWidth') {
+      var msg = [ 'pym', 'width', event.data.height ].join( 'xPYMx' );
+      getAmpWindow(sentinel)./*OK*/postMessage( msg, '*');


This sentinel I believe is undefined here, so that's not right. Nevertheless, I've tried populating sentinel here I tried populating with event.data.sentinel and 'amp' and 'amp-test' but no success.

Hmm. I'll try to take a look tomorrow, but I'm running out of time a bit. Could you ping @jridgewell to see if he might have advise on messaging specifically for tests?

The sentinel variable was indeed undefined. This seems to be part of the problem; the var was hoisting so the undefinedness wasn't causing an error. Fixed in c640f88.

jridgewell · 2019-11-26T01:14:33Z

extensions/amp-iframe/0.1/amp-iframe.js

+      return;
+    }
+    const data = getData(event);
+    if (typeof data !== 'string' || 'pym' !== data.substr(0, 3)) {


We have a startsWith helper

Utilized in af98c22

jridgewell · 2019-11-26T01:19:03Z

extensions/amp-iframe/0.1/amp-iframe.js

+      return;
+    }
+
+    // The format of the message takes the form of `pym${id}xPYMx${type}xPYMx${message}`.


The format has a "xPYMx" between "pymand${id}`, too. That was confusing me since you were splicing 2 elements from the front of the array (without the "xPYMx" there, you would be splicing too much).

Good catch. Fixed in eafb57a

jridgewell · 2019-11-26T01:21:09Z

extensions/amp-iframe/0.1/amp-iframe.js

+    // 'height', 'width', 'parentPositionInfo', 'navigateTo', and  'scrollToChildPos'.
+    // Only the 'height' and 'width' messages are currently supported.
+    // See <https://github.com/nprapps/pym.js/blob/57feb68/src/pym.js#L85-L102>
+    const args = data.split(/xPYMx/).splice(2);


Nit: no need to splice, just reference indexes 2 and 3.

Fixed in eafb57a

jridgewell · 2019-11-26T01:23:50Z

test/fixtures/served/iframe.html

@@ -52,6 +52,12 @@
            sentinel: sentinel,
            type: 'send-embed-state',
          }, '*');
+    } else if (event.data.type === 'requestPymjsHeight') {
+      var msg = [ 'pym', 'height', event.data.height ].join( 'xPYMx' );


You need to add some bogus id after pym and before height.

Fixed in 2d578da

jridgewell · 2019-11-26T01:23:56Z

test/fixtures/served/iframe.html

+      var msg = [ 'pym', 'height', event.data.height ].join( 'xPYMx' );
+      getAmpWindow(sentinel)./*OK*/postMessage( msg, '*');
+    } else if (event.data.type === 'requestPymjsWidth') {
+      var msg = [ 'pym', 'width', event.data.height ].join( 'xPYMx' );


Fixed in 2d578da

extensions/amp-iframe/0.1/test/test-amp-iframe.js

jridgewell · 2020-01-28T22:33:39Z

Ready for review?

westonruter · 2020-01-28T22:34:29Z

Ready for review?

Yes, I was just waiting for the jobs to finish before re-requesting. But yes!

westonruter · 2020-02-06T16:09:41Z

Rebased to re-trigger build due to infra issue.

googlebot added the cla: yes label Oct 5, 2019

amp-owners-bot bot requested a review from JonEleven October 5, 2019 06:24

westonruter commented Oct 5, 2019

View reviewed changes

westonruter mentioned this pull request Oct 5, 2019

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

Closed

westonruter commented Oct 5, 2019

View reviewed changes

extensions/amp-iframe/0.1/amp-iframe.js Outdated Show resolved Hide resolved

claudiulodro mentioned this pull request Oct 9, 2019

Add AMP-compatibility to pym shortcodes/blocks/raw embeds Automattic/pym-shortcode#62

Merged

westonruter force-pushed the add/amp-iframe-pymjs-support branch from c15556e to 231d8fe Compare October 10, 2019 13:00

dvoytenko suggested changes Oct 16, 2019

View reviewed changes

westonruter force-pushed the add/amp-iframe-pymjs-support branch 2 times, most recently from c3fa98c to 761267d Compare November 21, 2019 20:10

dvoytenko approved these changes Nov 21, 2019

View reviewed changes

westonruter force-pushed the add/amp-iframe-pymjs-support branch from 6d2fe10 to 92604a4 Compare November 23, 2019 00:49

westonruter commented Nov 23, 2019

View reviewed changes

jridgewell requested changes Nov 26, 2019

View reviewed changes

jridgewell closed this Jan 28, 2020

jridgewell reopened this Jan 28, 2020

westonruter requested a review from jridgewell January 28, 2020 22:34

jridgewell approved these changes Jan 28, 2020

View reviewed changes

westonruter added 7 commits February 6, 2020 08:08

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

7626fad

Use listen event-helper instead of addEventListner directly

9a7ca54

Rename unlisten_ to unlistenPym_

e26df98

Use this.win instead of window

d07949c

Indicate event param is non-nullable MessageEvent

480d2ed

Use getData event helper

8b93c01

Warn user about unsupported Pym.js messages

759c351

westonruter added 7 commits February 6, 2020 08:08

Replace function binding with arrow function to fix JSC_TYPE_MISMATCH

0b32582

Add test for Pym.js messages for height and width

5eaa8e9

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

e3f5ebc

Use startsWith string helper

73fff5a

Replace array splice with direct references; fix format comment doc

e02d0c4

Add missing ID from Pym message

80f84e1

Populate undefined variable sentinel

bc04025

westonruter force-pushed the add/amp-iframe-pymjs-support branch from dc41fe8 to bc04025 Compare February 6, 2020 16:09

alanorozco merged commit 39b1f61 into ampproject:master Feb 6, 2020

ampprojectbot added the PR use: In Beta / Experimental label Feb 14, 2020

ampprojectbot added the PR use: In Stable label Feb 25, 2020

maxlever mentioned this pull request Feb 28, 2020

amp-iframe sends constant errors when not resizable and child has pym loaded #27025

Closed

ampprojectbot mentioned this pull request Jan 20, 2022

🚨 Error: can't access dead object ampproject/error-reporting#140

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨Add amp-iframe support for Pym.js width and height resize messages #24917

✨Add amp-iframe support for Pym.js width and height resize messages #24917

westonruter commented Oct 5, 2019 •

edited

Loading

westonruter Oct 5, 2019

westonruter Oct 5, 2019

dvoytenko Oct 16, 2019

westonruter Nov 21, 2019

dvoytenko left a comment

dvoytenko Oct 16, 2019

westonruter Nov 21, 2019

dvoytenko Oct 16, 2019

westonruter Nov 21, 2019

dvoytenko Oct 16, 2019

westonruter Nov 21, 2019

dvoytenko Oct 16, 2019

westonruter commented Nov 21, 2019

westonruter commented Nov 21, 2019

dvoytenko left a comment

westonruter Nov 23, 2019

dvoytenko Nov 26, 2019

westonruter Jan 28, 2020

jridgewell Nov 26, 2019

westonruter Jan 28, 2020

jridgewell Nov 26, 2019

westonruter Jan 28, 2020

jridgewell Nov 26, 2019

westonruter Jan 28, 2020

jridgewell Nov 26, 2019

westonruter Jan 28, 2020

jridgewell Nov 26, 2019

westonruter Jan 28, 2020

jridgewell commented Jan 28, 2020

westonruter commented Jan 28, 2020

westonruter commented Feb 6, 2020

✨Add amp-iframe support for Pym.js width and height resize messages #24917

✨Add amp-iframe support for Pym.js width and height resize messages #24917

Conversation

westonruter commented Oct 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Nov 21, 2019

westonruter commented Nov 21, 2019

dvoytenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell commented Jan 28, 2020

westonruter commented Jan 28, 2020

westonruter commented Feb 6, 2020

westonruter commented Oct 5, 2019 •

edited

Loading