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

amp-app-banner: doesn't open itunes store #6454

Closed
sebastianbenz opened this issue Dec 2, 2016 · 27 comments
Closed

amp-app-banner: doesn't open itunes store #6454

sebastianbenz opened this issue Dec 2, 2016 · 27 comments
Assignees
Labels
Externally Tracked Tracked by Google or other parties P1: High Priority

Comments

@sebastianbenz
Copy link
Contributor

sebastianbenz commented Dec 2, 2016

Clicking on the banner action button doesn't open the iOS App Store if the app isn't installed (deep links into the app work).

How do we reproduce the issue?

  1. Open this link in Safari on an iOS device that hasn't got the Medium app installed.
  2. Click on "View in App"

The following error screen appears:

screen shot 2016-12-02 at 07 23 50

(The screenshot is from the emulator but it's the same behavior on a real device)

@sebastianbenz
Copy link
Contributor Author

//cc @ericlindley-g @mkhatib

@ericlindley-g
Copy link
Contributor

That's bad — we QA'd this pretty extensively. Is this just in the cache context, or outside as well?

@sebastianbenz
Copy link
Contributor Author

Just in the cache context.

@ericlindley-g ericlindley-g added this to the Current milestone Dec 2, 2016
@ericlindley-g
Copy link
Contributor

I can also repro on Chrome and Safari, with the viewer link (as opposed to the AMP Cache link in step 1 of the original repro steps. The AMP Cache link works as expected for me)

Hard to debug in Chrome dev tools because it's not a straightforward anchor link. @mkhatib , can you take a look?

@sebastianbenz
Copy link
Contributor Author

Thanks Eric - I accidentally posted the wrong link. The viewer link is the one that's failing.

@mkhatib
Copy link
Contributor

mkhatib commented Dec 5, 2016

I am able to repro. It looks like unfortunately on Safari the hack we have for redirecting to the app store is broken due to win.top.location.assign being undefined due to a security measure that seems to only happen on webkit.

The quick way to fix this is to use .replace instead of .assign. Which is weirdly available.

Usually browsers blocks access to different origins frames location for read-access only but would still allow write-access, in this case .assign not available is probably a bug in webkit.

Looking some more.

@mkhatib
Copy link
Contributor

mkhatib commented Dec 5, 2016

Filed a bug for webkit.

@dvoytenko are we ok with using normal win.top.location.href = url assignment? This seems to work as expected (for normal URLs, haven't tried it for custom URLs). Otherwise, we'd need to use .replace - which I don't like because it'll kill the back state - although this might be ok in this case since this would just open app-store and not navigate away to a new URL.

@dvoytenko
Copy link
Contributor

I'm ok with direct assignment if it works :) Sort of surprised that would work on cross-origin iframes, but if testing shows that it does - that's all I need.

@ericlindley-g
Copy link
Contributor

@mkhatib You're probably already on top of this, but we'll definitely want to test inside the AMP viewer context when served from the cache — let me know if you need any testing help.

@mkhatib
Copy link
Contributor

mkhatib commented Dec 7, 2016

Unfortunately this is two folds. Assign is only part of the problem. I am testing this and even when using .href = url or .replace I am unable to get the top navigation in a timeout from the iframe.

I could swear we have tested this before and it used to work, but there are many elements here (iframe'd or not, same/cross origin) that I am not sure that we tested all these combinations.

I am still trying to figure out how to work around this 😞 will keep experimenting and testing and report back

@dvoytenko
Copy link
Contributor

We always have an option to create viewer message that will ask viewer to perform this redirect.

@ericlindley-g
Copy link
Contributor

@dvoytenko Agreed, though I would really push to have a full solution in the document, to avoid creating a dependency on viewer functionality.

@dvoytenko
Copy link
Contributor

Sure. It has to be best effort and we can delegate to viewer under a viewer flag.

@mkhatib
Copy link
Contributor

mkhatib commented Dec 8, 2016

Ok, more experimentation results.

Looks like window.top.location.href = assignment works for any normal URLs but fails to do anything when setting to https://itunes.apple.com/us/app/id828256236 URL 😢

The same behavior exists for any custom URL with the app installed (like medium://).

Unfortunately, I've tried lots of combinations but seems like the timeout hack won't work for cross-domain top navigation... The only thing that seems to work in a timeout are:

  1. win.top.location.href = normal-links (e.g. https://example.com) (CROSS ORIGIN script, iframe, top and target link)
  2. win.top.location.replace(appstore) - SAME ORIGIN (script, iframe, top) - This was the reason why we didn't catch this early on in testing, we were testing on same-origin with replace

Unfortunately #1 would have worked except that Safari seems to treat itunes URLs as custom URLs and fail to load it on cross-origin href assignment.

So going forward I only see two paths:

  • Add a viewer dependency message to allow this workflow to work
  • Just drop the redirect-hack on iOS and fallback to two links - install and open

@ericlindley-g how should we proceed?

For completion, these work in a non-timeout Cross Origin navigation to the top:

  • win.open(appstore, _top)
  • win.open(applink, _top) if app installed - alert error if app not installed.

@mkhatib
Copy link
Contributor

mkhatib commented Dec 8, 2016

FYI webkit marked the bug I filed as won't fix - WAI, which makes sense. But I filed a follow up bug on specifically their inability to handle itunes links on iOS Safari.

@mkhatib
Copy link
Contributor

mkhatib commented Dec 12, 2016

Just a quick update, I have not figured out anyway to get this to work. @ericlindley-g let me know if we should proceed with a two-button solution?

@km-newsday
Copy link

I am implementing amp-app-banner (a wrapper and minimal UI for a cross-platform, fixed-position banner showing a call-to-action to install an app) element on AMP pages.

There is this comment from Dec. 15, 2016 on following page.
https://github.com/ampproject/amphtml/blob/master/extensions/amp-app-banner/amp-app-banner.md

(Dec. 15, 2016) Note: We have discovered an issue with going into the holiday code freeze. Banner links are not forwarding correctly to the app store on iOS inside the AMP Viewer context. We have issued a temporary mitigation to hide the banner in iOS inside the AMP Viewer, until we can launch a proper fix after the code freeze. Thanks for your patience!

I would like to know if the fix is made for the issue where banner links are not forwarding correctly to the app store on iOS.

@mkhatib
Copy link
Contributor

mkhatib commented Jan 20, 2017

@km-newsday unfortunately we still haven't fixed this on iOS, so if the app is not installed it won't redirect to the appstore.

We're still working on an implementation to fix this, no ETA yet.

@km-newsday
Copy link

km-newsday commented Jan 20, 2017 via email

@km-newsday
Copy link

km-newsday commented May 11, 2017 via email

@ericlindley-g
Copy link
Contributor

/cc @muxin @aghassemi

@km-newsday , can you tell us what context you're testing in? Is this on the origin (pub.com), through an AMP cache (cdn.ampproject.org), or through an AMP viewer (e.g. google.com/amp/pub.com)?

Also, if it's in an AMP viewer, can you say if it's on the web or in an app? (e.g. Google News & Weather or Google Web Search)

@ericlindley-g ericlindley-g reopened this May 11, 2017
@km-newsday
Copy link

km-newsday commented May 11, 2017 via email

@aghassemi
Copy link
Contributor

@ericlindley-g dup of #9077

@ericlindley-g
Copy link
Contributor

Thanks @km-newsday and @aghassemi — closing this bug as a duplicate of #9077, but we're looking into it and will update there when a fix is in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Externally Tracked Tracked by Google or other parties P1: High Priority
Projects
None yet
Development

No branches or pull requests

10 participants