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

Bento: iframe components attemptChangeHeight #35027

Merged
merged 13 commits into from
Jul 9, 2021
17 changes: 16 additions & 1 deletion docs/building-a-bento-iframe-component.md
Original file line number Diff line number Diff line change
Expand Up @@ -243,13 +243,28 @@ class AmpFantasticEmbed extends BaseElement {
init() {
return dict({
'requestResize': (height) => {
this.attemptChangeHeight(height);
this.attemptChangeHeight(height).catch(() => {
if (!this.getOverflowElement()) {
user().warn(TAG, '[overflow] element not found. Provide one to enable resizing to full contents.')
}
});
},
});
}
}
```

For components that request a resize that is denied by the AMP runtime, publishers are recommended to use an `overflow` element to solicit user interaction in order to resize as a layout stability best-practice.

This information can be provided in the component documentation with an exemplary code sample:

```html
<amp-fantastic-embed layout="fixed" width="400" height="200">
<button overflow>Click me to load the full iframed content!</button>
</amp-fantastic-embed>
<p>Content below the component.</p>
```

#### Passing or overriding props

In the previous example, props received from the `ProxyIframeEmbed` are implicitly set through `...rest`. If we set each explicitly, we see the `HTMLIframeElement` attributes handled.
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-facebook/1.0/amp-facebook.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class AmpFacebook extends BaseElement {
init() {
return dict({
'onReady': () => this.togglePlaceholder(false),
'requestResize': (height) => this.forceChangeHeight(height),
'requestResize': (height) => this.attemptChangeHeight(height),
});
}

Expand Down
10 changes: 7 additions & 3 deletions extensions/amp-facebook/1.0/test/test-amp-facebook.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/**
* Copyright 2015 The AMP HTML Authors. All Rights Reserved.
* Copyright 2021 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -286,7 +286,11 @@ describes.realWin(
await waitForRender();

const impl = await element.getImpl(false);
const forceChangeHeightStub = env.sandbox.stub(impl, 'forceChangeHeight');
const attemptChangeHeightStub = env.sandbox.stub(
impl,
'attemptChangeHeight'
);
attemptChangeHeightStub.returns(Promise.resolve());

const mockEvent = new CustomEvent('message');
const sentinel = JSON.parse(
Expand All @@ -298,7 +302,7 @@ describes.realWin(
mockEvent.source =
element.shadowRoot.querySelector('iframe').contentWindow;
win.dispatchEvent(mockEvent);
expect(forceChangeHeightStub).to.be.calledOnce.calledWith(1000);
expect(attemptChangeHeightStub).to.be.calledOnce.calledWith(1000);
});
}
);
4 changes: 1 addition & 3 deletions extensions/amp-instagram/1.0/amp-instagram.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,7 @@ class AmpInstagram extends BaseElement {
/** @override */
init() {
return dict({
'requestResize': (height) => {
this.forceChangeHeight(height);
},
'requestResize': (height) => this.attemptChangeHeight(height),
});
}
}
Expand Down
8 changes: 6 additions & 2 deletions extensions/amp-instagram/1.0/test/test-amp-instagram.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,11 @@ describes.realWin(
await waitForRender();

const impl = await element.getImpl(false);
const forceChangeHeightStub = env.sandbox.stub(impl, 'forceChangeHeight');
const attemptChangeHeightStub = env.sandbox.stub(
impl,
'attemptChangeHeight'
);
attemptChangeHeightStub.returns(Promise.resolve());

const mockEvent = new CustomEvent('message');
mockEvent.origin = 'https://www.instagram.com';
Expand All @@ -107,7 +111,7 @@ describes.realWin(
element.shadowRoot.querySelector('iframe').contentWindow;
win.dispatchEvent(mockEvent);

expect(forceChangeHeightStub).to.be.calledOnce.calledWith(1000);
expect(attemptChangeHeightStub).to.be.calledOnce.calledWith(1000);
});
}
);
2 changes: 1 addition & 1 deletion extensions/amp-twitter/1.0/amp-twitter.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class AmpTwitter extends BaseElement {
/** @override */
init() {
return dict({
'requestResize': (height) => this.forceChangeHeight(height),
'requestResize': (height) => this.attemptChangeHeight(height),
});
}

Expand Down
8 changes: 6 additions & 2 deletions extensions/amp-twitter/1.0/test/test-amp-twitter.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,11 @@ describes.realWin(
await waitForRender();

const impl = await element.getImpl(false);
const forceChangeHeightStub = env.sandbox.stub(impl, 'forceChangeHeight');
const attemptChangeHeightStub = env.sandbox.stub(
impl,
'attemptChangeHeight'
);
attemptChangeHeightStub.returns(Promise.resolve());

const mockEvent = new CustomEvent('message');
const sentinel = JSON.parse(
Expand All @@ -86,7 +90,7 @@ describes.realWin(
mockEvent.source =
element.shadowRoot.querySelector('iframe').contentWindow;
win.dispatchEvent(mockEvent);
expect(forceChangeHeightStub).to.be.calledOnce.calledWith(1000);
expect(attemptChangeHeightStub).to.be.calledOnce.calledWith(1000);
});

it('should replace iframe after tweetid mutation', async () => {
Expand Down
14 changes: 14 additions & 0 deletions src/preact/base-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import {WithAmpContext} from './context';
import {CanPlay, CanRender, LoadingProp} from './contextprops';
import {AmpElementPropDef, collectProps} from './parse-props';

import {user} from '../log';
import {getMode} from '../mode';
import {installShadowStyle} from '../shadow-embed';

Expand Down Expand Up @@ -382,6 +383,19 @@ export class PreactBaseElement extends AMP.BaseElement {
}
}

/** @override */
attemptChangeHeight(newHeight) {
super.attemptChangeHeight(newHeight).catch(() => {
if (!this.getOverflowElement()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For auto-envelope.

Suggested change
if (!this.getOverflowElement()) {
if (this.getOverflowElement && !this.getOverflowElement()) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thank you!

const TAG = this.element.tagName;
user().warn(
TAG,
'[overflow] element not found. Provide one to enable resizing to full contents.'
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you mentioned offline, we might want to use console.warn instead.

}
});
}

/**
* @protected
* @param {!JsonObject} props
Expand Down