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

Navigating to AMP-enabled sites on IE/Edge adds "undefined" to the url #743

Closed
petetnt opened this issue Oct 24, 2015 · 8 comments
Closed
Assignees

Comments

@petetnt
Copy link

petetnt commented Oct 24, 2015

Steps to reproduce:

  1. Navigate to https://ampproject.org/
  2. After loading the page the url / document.location.href is now https://ampproject.org/undefined

Tested on Edge, IE11, Windows Phone 8.1 IE11.

This creates some unwanted effects, such as reloading leading to a 404 page.

@petetnt petetnt changed the title Going to https://ampproject.net/ on IE/Edge adds "undefined" to the url Going to https://ampproject.org/ on IE/Edge adds "undefined" to the url Oct 24, 2015
@erwinmombay erwinmombay self-assigned this Oct 24, 2015
@erwinmombay
Copy link
Member

thanks for reporting! will take a look

@erwinmombay
Copy link
Member

@petetnt confirmed. ANY amp page right now seems to trigger a url change to /undefined on Edge and IE11.

/cc @cramforce @dvoytenko

@erwinmombay
Copy link
Member

looks like this is caused by https://github.com/ampproject/amphtml/blob/master/src/history.js#L285 (call to the browsers native replace state). unsure yet why it's happening

@erwinmombay
Copy link
Member

looks like passing in undefined as the url (3rd argument), even though its marked as optional, really screws it up. not passing in undefined fixes the issue. (looks like its weirdly coerced into a string)

@petetnt petetnt changed the title Going to https://ampproject.org/ on IE/Edge adds "undefined" to the url Navigating to AMP-enabled sites on IE/Edge adds "undefined" to the url Oct 25, 2015
@petetnt
Copy link
Author

petetnt commented Oct 25, 2015

@erwinmombay cool, I though it could be something that affected all the AMP sites but didn't have the time to check yesterday 👍

I noticed something else too: it coerces Booleans to strings too so replaceState({foo: "bar"}, foo, true}) results in foo.org/true 👻

It's not AMP's job to work around browser bugs, but I guess all of those kinds of weird behaviors (like someone accidentally passing true/false as an opt_url could be avoided by extending https://github.com/ampproject/amphtml/pull/746/files#diff-6d1071e9a4a9346d2d306a79be2de0d1R287 with:

if (opt_url && typeof opt_url === "string") {

(Actually it turns out that you can pass pretty much anything as the URL and browsers will comply)

@erwinmombay
Copy link
Member

@petetnt thanks. that scenario should be caught once we get the closure compiler running so the only things that should come through as opt_url is of type {string|undefined} (i'll probably change my code to only check for undefined)

@petetnt
Copy link
Author

petetnt commented Oct 25, 2015

Sounds good to me 👍

@erwinmombay
Copy link
Member

landed in f268934

thanks again @petetnt!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants