✨amp-consent: support external consent flow #15805

zhouyx · 2018-06-04T19:35:29Z

Support external consent flow using <amp-iframe> and send consent response back using postMessages.

zhouyx · 2018-06-04T19:38:52Z

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

@@ -76,6 +79,9 @@ describes.realWin('amp-iframe', {
        }
      });
      setTrackingIframeTimeoutForTesting(20);
+      setTrackingIframeCountForTesting(0);
+      sandbox.stub(error, 'reportError');


@rsimha I encountered the issue in #15658 as well.
However the test is intentionally trying to test the misconfiguration. I used allowConsoleError, but that doesn't work because user().assert() call #reportError(), and that will rethrow the error again async.
This is my temporarily solution to fix this, stub #reportError(), I think a more general solution would be to stub #reportError() for all tests. WDYT

You can use expectAsyncConsoleError(<message>) for async errors. Did you try that?

It's explained in the failure message.

I hesitate to stub out reportError since several tests actually check if it fires. However, we could make it so that the error is thrown synchronously during tests.

And finally, if a mocha test does something asynchronous, it is responsible for calling the done() callback to indicate that the test has completed. Unfortunately, a lot of our tests fail to do so.

You can use expectAsyncConsoleError() for async errors. Did you try that?

I tried that. It didn't work because I didn't call done() and the test ended earlier.

I think it will work if I call the #done() callback. But the problem is that I don't know where to call the done() callback if I can't stub the #reportError(). Can I do yield expectAsyncConsoleError(message).

The test logic is super simple, check if a misconfiguration results in a function not being called eventually. I don't think it makes much sense to have these unit tests figure out the logic behind #assert() and call #done() callback to not break tests.

Talked offline. Will leave the stub there until we figure out a nice general solution.

Meanwhile, #16005 should fix the layoutCallback issues we were discussing here.

zhouyx · 2018-06-04T19:42:04Z

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

@@ -821,7 +861,8 @@ describes.realWin('amp-iframe', {
        expect(addEventListener).to.not.be.called;
      });

-      it('should receive "message" events from <iframe>', function*() {
+      // TODO (@choumx): Unskip the test
+      it.skip('should receive "message" events from <iframe>', function*() {


@choumx I can't fix this test : ( Skip it for now

Thanks for the heads up.

aghassemi · 2018-06-05T18:28:58Z

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

@@ -163,6 +172,11 @@ export class AmpIframe extends AMP.BaseElement {

  /** @private */
  assertPosition_() {
+    if (this.consentContainer_) {


We should keep amp-iframe empty of any logic related to other elements.
Two suggestions:
1- Is there an issue with using the normal must provide a placeholder instead of changing the assertPosition logic? Presence of placeholder can be enforced by amp-consent validation rules (e.g. have a rule that amp-iframe child of amp-consent must have a placeholder)
2- Created a generic list of messages to forward in amp-iframe. Have amp-iframe blindly dispatch an event when it receives one of those messages. For now the list would only contain external-consent-flow.

I'm not sure about the mandatory placeholder. This is a pop up iframe, it doesn't much sense to require a placeholder for it.
I can blindly dispatch the event, but it still needs to be laid out to do so. I would still need to check position of the iframe, which requires me to check if the iframe is within an <amp-consent> component.

let's keep dependencies one-way and only from amp-consent -> amp-iframe. If mandatory placeholder does not make sense, alternatively amp-consent can inject a placeholder ( if non exists ) during its build callback into decesendant amp-iframe.

Apologies for pushing this, intertwined dependencies can really make the code complex and as AMP codebase grows we need to watch out for code complexity as much as possible

Talked offline. We need a smarter API to tell <amp-iframe> whether it is OK to be placed in first viewport. For example, it is sort of a bug for <amp-iframe> within <amp-lightbox>. Even though, it requires user interaction to open the lightbox, an <amp-iframe> within it still requires a placeholder to be laid out.

For this PR. We agreed that we should always require a placeholder for <amp-iframe> within <amp-consent>, because these iframes can be prompted without user interaction, and it makes sense to always include a placeholder to prevent blank space.

@aghassemi dependency removed. <amp-iframe> will only listen to consent-response msg, and blindly dispatch event to the window.

Unfortunately I can't have <amp-consent> directly listen to that postMessage, because it requires <amp-consent> to know the exact iframe to listen to. And there's not API method for that.

zhouyx · 2018-06-09T00:55:28Z

@aghassemi

@lannka has an even stronger opinion about the dependency here 😄....
He didn't want me to add a single line of code to amp-iframe. After we talked offline, here's what he proposed. @lannka Could you confirm this is what you want?
Having amp-consent do

win.addEventListener('message', event => {
  triggerWindow = event.source
  iframes = ampConsent.element.querySelectorAll('iframes')
  for each iframe:
    if (iframe.contentWindow == triggerWindow) {
     handleAction(event.action) 
     return
   }
}

My only concern is that register event listener within individual component is not optional and might be expensive, especially given that amp-consent needs to query all its child iframe elements here to check source. But if everyone is happy with this approach, I am happy to switch : )

zhouyx · 2018-06-16T00:40:06Z

@lannka @aghassemi
While I was making the change, I realize one thing.
At somepoint, the <amp-iframe> will require the consentId from the parent <amp-consent> element even though we don't support it right away. What would be the best way to support the two way messages then? Would it influence the decision we made here?

aghassemi · 2018-06-19T16:32:10Z

@zhouyx I assume the iframe would first request the consentId and then receive it? In which case, amp-consent can keep a reference to the iframe window when it receives a request message (there will be a pointer to the window on the message event) and post directly do it with the response.

lannka

As discussed offline, please remove consentId and sentinel in post message

lannka · 2018-06-18T23:35:08Z

test/manual/diy-consent.html

+}
+
+document.querySelector('#a').onclick = function () {
+		// parent.postMessage("message to be sent", "http://the-website-that-will-receive-the-msg.com")


bad indentation?

zhouyx · 2018-06-20T20:00:02Z

As discussed offline. It's really difficult to define the most ideal behaviors here. We decided to add minimal support also put minimum restrictions at first stage. Will expand the functionality if advanced features are required later on.

zhouyx requested review from aghassemi and lannka June 4, 2018 19:35

googlebot added the cla: yes label Jun 4, 2018

zhouyx commented Jun 4, 2018

View reviewed changes

zhouyx force-pushed the amp-consent-external branch 2 times, most recently from a75eb18 to 346cc1f Compare June 4, 2018 19:41

zhouyx commented Jun 4, 2018

View reviewed changes

aghassemi suggested changes Jun 5, 2018

View reviewed changes

zhouyx force-pushed the amp-consent-external branch from 669e550 to ee2d97d Compare June 8, 2018 20:48

aghassemi approved these changes Jun 9, 2018

View reviewed changes

rsimha mentioned this pull request Jun 13, 2018

🐛✅ Make amp-iframe tests synchronous and expect known errors #16005

Merged

zhouyx added 4 commits June 18, 2018 14:10

external consent flow

ddfeaf6

remove dependency

3048f2c

move logic to amp-consent

48bab28

add tests

84c0227

zhouyx force-pushed the amp-consent-external branch from ee2d97d to 84c0227 Compare June 18, 2018 22:20

lannka approved these changes Jun 19, 2018

View reviewed changes

address comment

1e4083a

zhouyx merged commit a996ec0 into ampproject:master Jun 20, 2018

zhouyx mentioned this pull request Jul 3, 2018

External consent dialog iframe example ampproject/amp-by-example#1336

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨amp-consent: support external consent flow #15805

✨amp-consent: support external consent flow #15805

zhouyx commented Jun 4, 2018

zhouyx Jun 4, 2018

rsimha Jun 4, 2018

rsimha Jun 4, 2018

rsimha Jun 4, 2018

zhouyx Jun 4, 2018 •

edited

zhouyx Jun 8, 2018

rsimha Jun 13, 2018

zhouyx Jun 4, 2018

dreamofabear Jun 6, 2018

aghassemi Jun 5, 2018

zhouyx Jun 5, 2018

aghassemi Jun 7, 2018 •

edited

zhouyx Jun 7, 2018

zhouyx Jun 8, 2018

zhouyx commented Jun 9, 2018

zhouyx commented Jun 16, 2018

aghassemi commented Jun 19, 2018

lannka left a comment

lannka Jun 18, 2018

zhouyx Jun 20, 2018

zhouyx commented Jun 20, 2018

✨amp-consent: support external consent flow #15805

✨amp-consent: support external consent flow #15805

Conversation

zhouyx commented Jun 4, 2018

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

zhouyx Jun 4, 2018 • edited

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

aghassemi Jun 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx commented Jun 9, 2018

zhouyx commented Jun 16, 2018

aghassemi commented Jun 19, 2018

lannka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx commented Jun 20, 2018

zhouyx Jun 4, 2018 •

edited

aghassemi Jun 7, 2018 •

edited