From 8541477fb6c9b553b87f744f67f93aaa24028af4 Mon Sep 17 00:00:00 2001 From: Ali Ghassemi Date: Wed, 4 Oct 2017 11:45:53 -0700 Subject: [PATCH] Fix broken system social share in Webviews and Opera UA detection (#11556) --- .../amp-social-share/0.1/amp-social-share.js | 23 +++++++++++++--- src/service/platform-impl.js | 5 +++- src/service/viewer-impl.js | 8 ++++++ test/functional/test-platform.js | 12 +++++++++ test/functional/test-viewer.js | 26 +++++++++++++++++++ 5 files changed, 69 insertions(+), 5 deletions(-) diff --git a/extensions/amp-social-share/0.1/amp-social-share.js b/extensions/amp-social-share/0.1/amp-social-share.js index 2ac61a52ec1f..6cb47696adbf 100644 --- a/extensions/amp-social-share/0.1/amp-social-share.js +++ b/extensions/amp-social-share/0.1/amp-social-share.js @@ -41,6 +41,9 @@ class AmpSocialShare extends AMP.BaseElement { /** @private {?../../../src/service/platform-impl.Platform} */ this.platform_ = null; + /** @private {?../../../src/service/viewer-impl.Viewer} */ + this.viewer_ = null; + /** @private {?string} */ this.href_ = null; @@ -60,15 +63,19 @@ class AmpSocialShare extends AMP.BaseElement { user().assert(!/\s/.test(typeAttr), 'Space characters are not allowed in type attribute value. %s', this.element); + + this.platform_ = Services.platformFor(this.win); + this.viewer_ = Services.viewerForDoc(this.element); + if (typeAttr === 'system') { // Hide/ignore system component if navigator.share unavailable - if (!('share' in navigator)) { + if (!this.systemShareSupported_()) { setStyle(this.element, 'display', 'none'); return; } } else { // Hide/ignore non-system component if system share wants to be unique - const systemOnly = ('share' in navigator) && + const systemOnly = this.systemShareSupported_() && !!this.win.document.querySelectorAll( 'amp-social-share[type=system][data-mode=replace]').length; if (systemOnly) { @@ -83,7 +90,6 @@ class AmpSocialShare extends AMP.BaseElement { 'The data-share-endpoint attribute is required. %s', this.element); Object.assign(this.params_, typeConfig['defaultParams'], getDataParamsFromAttributes(this.element)); - this.platform_ = Services.platformFor(this.win); const hrefWithVars = addParamsToUrl(this.shareEndpoint_, this.params_); const urlReplacements = Services.urlReplacementsForDoc(this.getAmpDoc()); @@ -158,8 +164,17 @@ class AmpSocialShare extends AMP.BaseElement { openWindowDialog(this.win, href, target, windowFeatures); } } -} + /** @private */ + systemShareSupported_() { + // Chrome exports navigator.share in WebView but does not implement it. + // See https://bugs.chromium.org/p/chromium/issues/detail?id=765923 + const isChromeWebview = this.viewer_.isWebviewEmbedded() && + this.platform_.isChrome(); + + return ('share' in navigator) && !isChromeWebview; + } +} AMP.extension('amp-social-share', '0.1', AMP => { AMP.registerElement('amp-social-share', AmpSocialShare, CSS); diff --git a/src/service/platform-impl.js b/src/service/platform-impl.js index 6f99cafe0188..a5ac822baeb6 100644 --- a/src/service/platform-impl.js +++ b/src/service/platform-impl.js @@ -80,7 +80,10 @@ export class Platform { * @return {boolean} */ isOpera() { - return /OPR|Opera|OPiOS/i.test(this.navigator_.userAgent); + // Chrome mobile UA includes OPR stating with Chrome 60, however real + // Opera puts put a / after OPR and that's the only tell, so + // we check for OPR/ instead of OPR + return /OPR\/|Opera|OPiOS/i.test(this.navigator_.userAgent); } /** diff --git a/src/service/viewer-impl.js b/src/service/viewer-impl.js index 5a253917aa94..a5985fae712a 100644 --- a/src/service/viewer-impl.js +++ b/src/service/viewer-impl.js @@ -485,6 +485,14 @@ export class Viewer { return this.isEmbedded_; } + /** + * Whether the document is embedded in a webview. + * @return {boolean} + */ + isWebviewEmbedded() { + return this.isWebviewEmbedded_; + } + /** * @return {boolean} */ diff --git a/test/functional/test-platform.js b/test/functional/test-platform.js index a32b48d3d20a..b310a1cac315 100644 --- a/test/functional/test-platform.js +++ b/test/functional/test-platform.js @@ -198,6 +198,18 @@ describe('Platform', () => { testStandalone(userAgent, isStandalone); }); + it('Pixel Chrome 61', () => { + isAndroid = true; + isChrome = true; + isWebKit = true; + majorVersion = 61; + userAgent = 'Mozilla/5.0 (Linux; Android 8.0.0; Pixel XL Build/OPR6.' + + '170623.011) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163' + + '.98 Mobile Safari/537.36'; + testUserAgent(userAgent); + testStandalone(userAgent, isStandalone); + }); + it('Firefox', () => { isFirefox = true; majorVersion = 40; diff --git a/test/functional/test-viewer.js b/test/functional/test-viewer.js index c1939d958165..ea7bae64eb81 100644 --- a/test/functional/test-viewer.js +++ b/test/functional/test-viewer.js @@ -780,6 +780,32 @@ describe('Viewer', () => { }); }); + describe('isWebviewEmbedded', () => { + it('should be webview w/ "webview=1"', () => { + windowApi.parent = windowApi; + windowApi.location.hash = '#webview=1'; + expect(new Viewer(ampdoc).isWebviewEmbedded()).to.be.true; + }); + + it('should NOT be webview w/o "webview=1"', () => { + windowApi.parent = windowApi; + windowApi.location.hash = '#foo=1'; + expect(new Viewer(ampdoc).isWebviewEmbedded()).to.be.false; + }); + + it('should NOT be webview w/ "webview=0"', () => { + windowApi.parent = windowApi; + windowApi.location.hash = '#webview=0'; + expect(new Viewer(ampdoc).isWebviewEmbedded()).to.be.false; + }); + + it('should NOT be webview if iframed regardless of "webview=1"', () => { + windowApi.parent = {}; + windowApi.location.hash = '#webview=1'; + expect(new Viewer(ampdoc).isEmbedded()).to.be.false; + }); + }); + describe('isTrustedViewer', () => { it('should consider non-trusted when not iframed', () => {