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

Online iframe resizing rules with fallback. #790

Merged
merged 1 commit into from
Oct 30, 2015

Conversation

dvoytenko
Copy link
Contributor

Closes #728.

Still working on docs...

win.addEventListener('message', function(event) {
if (event.origin != origin) {
if (origin != '*' && event.origin != origin) {
Copy link
Member

Choose a reason for hiding this comment

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

Why this? We should be able to calculate the origin of the iframe and only accept that.

Copy link
Member

Choose a reason for hiding this comment

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

To avoid races: How about tagging the message here as to whether it came from the active element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about srcdoc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RE: races. Good idea. Will do.

Copy link
Member

Choose a reason for hiding this comment

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

With respect to srcdoc (which we currently don't support, but I'm working on right now) this is probably OK since we do check the source window.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"OK" as in let's keep ''? Or let's get origin from URL assuming that you will always convert srcdoc to a data URL? I likewise judged '' this to be ok since we always check on source window.

@dvoytenko
Copy link
Contributor Author

The latest push includes:

  1. Focus history
  2. A separate listener protocol with origin

@dvoytenko
Copy link
Contributor Author

Scroll adjustment is removed from this PR.

}
};
this.blurCapture_ = e => {
timer.delay(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Nice. You need to document this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@cramforce
Copy link
Member

LGTM

@dvoytenko
Copy link
Contributor Author

The spec is changed to looking for resizable=yes attribute with overflow element required.

@cramforce
Copy link
Member

I'd prefer resizable to be a bare attribute without any value.

Otherwise LGTM

newHeight: newHeight,
force: force,
fallback: fallback
});
Copy link
Member

Choose a reason for hiding this comment

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

probably not a part of our style guide but a nice to know, is ES2015 Object Literal Property Value Shorthand. (when key and the value have the same identifier)

this.changeHeightRequests_.push({
  resource,
  newHeight,
  force,
  fallback
});

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 to know. Do we have a compiler for this?

Copy link
Member

Choose a reason for hiding this comment

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

yep, this should work with babel-core (no helpers required). not sure if closure-compiler has support for it, which is why i hazard supporting us to use it 😸

@dvoytenko
Copy link
Contributor Author

@cramforce Yes, definitely makes sense. Switch to no-value attribute.

@dvoytenko dvoytenko force-pushed the iframe-resize1 branch 2 times, most recently from 8894606 to 339bafc Compare October 30, 2015 23:24
dvoytenko added a commit that referenced this pull request Oct 30, 2015
Online iframe resizing rules with fallback.
@dvoytenko dvoytenko merged commit cbaeb24 into ampproject:master Oct 30, 2015
@dvoytenko dvoytenko deleted the iframe-resize1 branch October 30, 2015 23:47
@antipath
Copy link

Hi,

It seems if we leave out "allow-same-origin" from the sanbox policy, the iframe resize does not work at all.

is there a workaround for this? please help!

Thanks,
Vince

@dvoytenko
Copy link
Contributor Author

@antipath Could you please file a separate bug for this since this is a closed pull request?

@dparikh dparikh mentioned this pull request Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants