Fix broken system social share in Webviews and Opera UA detection (#1…

…1556)
ampproject · Oct 4, 2017 · 8541477 · 8541477
1 parent a08b1a3
commit 8541477
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 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
@@ -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
@@ -80,7 +80,10 @@ export class Platform {
    * @return {boolean}
    */
   isOpera() {
-    return /OPR|Opera|OPiOS/i.test(this.navigator_.userAgent);
+    // Chrome mobile UA includes OPR<v> 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
@@ -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
@@ -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
@@ -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', () => {