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

Consider it as non-viewer mode if there is no "origin" in hash param. #5867

Merged
merged 1 commit into from Oct 28, 2016
Merged

Consider it as non-viewer mode if there is no "origin" in hash param. #5867

merged 1 commit into from Oct 28, 2016

Conversation

lannka
Copy link
Contributor

@lannka lannka commented Oct 27, 2016

Fixes #4183

@jridgewell
Copy link
Contributor

Would visibilityState be a good sentinel? By including it, you've guaranteed that you intend to control the AMP document with a viewer.

Re: #4183 (comment)

@lannka
Copy link
Contributor Author

lannka commented Oct 28, 2016

@jridgewell is visibilityState guaranteed by all kinds of viewers? is it possible to have undefined visibilityState ?

I think at the moment we probably should just make it simple. It will only be nicely solved once we had a formal Viewer API spec.

@lannka lannka merged commit fc30742 into ampproject:master Oct 28, 2016
@lannka lannka deleted the isEmbed_check_viewer_origin branch October 28, 2016 16:09
@jridgewell
Copy link
Contributor

visibilityState implies a messaging channel, as it's necessary to send new visibility states.

But one thing to emphasize: we'd normally never use it at face value. It must be confirmed through handshake before we can reliably say that the origin is what it says it is.

The origin parameter seems like is unnecessary legacy code, I'd rather not codify it.

/cc @dvoytenko

@cramforce
Copy link
Member

@jridgewell Why legacy code? We expect to need it in the future, no? Without it we cannot send messages to our parent in Firefox.

@jridgewell
Copy link
Contributor

Isn't that parent.location.origin? Or ancestorOrigins when it becomes available.

@cramforce
Copy link
Member

@jridgewell That isn't supported by all browsers (including Firefox with no intention to implement).

@jridgewell
Copy link
Contributor

But parent.location.origin is?

@cramforce
Copy link
Member

@jridgewell is it? I don't think so, but would be pleasantly surprised.

@jridgewell
Copy link
Contributor

Well, today I learned two things:

  • Firefox has stupid security restrictions for cross-origin iframes
  • jsbin double iframes their bins, and the second iframe is same origin to the first.

Guess we're stuck with the origin parameter.

samiamorwas pushed a commit to samiamorwas/amphtml that referenced this pull request Oct 31, 2016
Merge branch 'master' of https://github.com/ampproject/amphtml into a…
314d2b1 
…mp_reddit_extension

* 'master' of https://github.com/ampproject/amphtml: (121 commits)
  `#setOwner` should rewrite `resource.owner_` value if it exist (ampproject#5898)
  Defer full upgrade until the element is connected (ampproject#5908)
  Skeleton for an amp-animation component (ampproject#5891)
  Use upgrade callback to pick the correct carousel class. (ampproject#5899)
  Add alert role to user-notification by default (ampproject#5896)
  Fix test-iframe-createIframeWithMessageStub failures on older browsers (ampproject#5895)
  Make screen readers announce changes in the slides carousel. (ampproject#5892)
  Separate out implementaton of document-info and remove dependency of … (ampproject#5864)
  Fixed assertion in key fetch function. (ampproject#5854)
  Track impression on amp landing page (ampproject#5606)
  Backward compatible way of stringifying Uint8Array (ampproject#5881)
  Ensure that a friendly-iframe embed cancels any boilerplate when ready (ampproject#5863)
  Fix amp-forms broken and flakey tests. (ampproject#5835)
  Make SW test properly skip when Request == undefined (ampproject#5876)
  Add Preamble section to AMP Cache Guidelines (ampproject#5873)
  Adding Adverline to amp-ad (ampproject#5829)
  Clarify cache guidelines (ampproject#5874)
  Consider it as non-viewer mode if there is no "origin" in hash param. (ampproject#5867)
  [amp-youtube] autoplay and test suite to run across all video players that implement the video API (ampproject#5765)
  Update Forms Docs to reflect availability. (ampproject#5815)
  ...
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
@lannka
Consider it as non-viewer mode if there is no "origin" in hash param. (…
a3480fd 
…ampproject#5867)
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
@lannka
Consider it as non-viewer mode if there is no "origin" in hash param. (…
fca2f8b 
…ampproject#5867)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cramforce cramforce cramforce approved these changes

Assignees

@cramforce cramforce

@dvoytenko dvoytenko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lannka @jridgewell @cramforce @dvoytenko