Perform Chrome version check to enable highlighting using text fragme…

…nts (#35633)
ampproject · Aug 12, 2021 · 481b4e6 · 481b4e6
1 parent 099f23c
commit 481b4e6
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 10 deletions.
diff --git a/extensions/amp-viewer-integration/0.1/highlight-handler.js b/extensions/amp-viewer-integration/0.1/highlight-handler.js
@@ -17,7 +17,6 @@
 import {Services} from '#service';
 import {dict} from '#core/types/object';
 import {findSentences, markTextRangeList} from './findtext';
-import {isExperimentOn} from '#experiments';
 import {listenOnce} from '../../../src/event-helper';
 import {moveLayoutRect} from '#core/dom/layout/rect';
 import {once} from '#core/types/function';
@@ -157,9 +156,22 @@ export class HighlightHandler {
     /** @private {?Array<!Element>} */
     this.highlightedNodes_ = null;
 
+    const platform =
+      /* @type {!./service/platform-impl.Platform} */ Services.platformFor(
+        this.ampdoc_.win
+      );
+
+    // Chrome 81 added support for text fragment proposal. However, it is
+    // not supported across iframes but Chrome 81 thru 92 report
+    // `'fragmentDirective' in document = true` which breaks feature detection.
+    // Chrome 93 supports the proposal that works across iframes, hence this
+    // version check.
+    // TODO(dmanek): remove `ifChrome()` from unit tests when we remove
+    // Chrome version detection below
     if (
       'fragmentDirective' in document &&
-      isExperimentOn(ampdoc.win, 'use-text-fragments-for-highlights')
+      platform.isChrome() &&
+      platform.getMajorVersion() >= 93
     ) {
       ampdoc
         .whenFirstVisible()

diff --git a/extensions/amp-viewer-integration/0.1/test/test-highlight-handler.js b/extensions/amp-viewer-integration/0.1/test/test-highlight-handler.js
@@ -20,7 +20,6 @@ import {Messaging, WindowPortEmulator} from '../messaging/messaging';
 import {Services} from '#service';
 import {VisibilityState} from '#core/constants/visibility-state';
 import {layoutRectLtwh} from '#core/dom/layout/rect';
-import {toggleExperiment} from '#experiments';
 
 describes.fakeWin(
   'getHighlightParam',
@@ -424,13 +423,14 @@ describes.realWin(
       expect(setScrollTopStub.firstCall.args[0]).to.equal(350);
     });
 
-    // TODO(dmanek): remove `ifChrome` once other major browsers support
-    // text fragments (i.e. 'fragmentDirective' in document = true)
+    // TODO(dmanek): remove `ifChrome` once we remove Chrome version detection
     it.configure()
       .ifChrome()
-      .run('should highlight using text fragments', async () => {
-        toggleExperiment(env.win, 'use-text-fragments-for-highlights', true);
+      .run('should highlight using text fragments for Chrome 93', async () => {
         const {ampdoc} = env;
+        const platform = Services.platformFor(ampdoc.win);
+        env.sandbox.stub(platform, 'isChrome').returns(true);
+        env.sandbox.stub(platform, 'getMajorVersion').returns(93);
         let whenFirstVisiblePromiseResolve;
         const whenFirstVisiblePromise = new Promise((resolve) => {
           whenFirstVisiblePromiseResolve = resolve;
@@ -456,15 +456,49 @@ describes.realWin(
         );
       });
 
-    // TODO(dmanek): remove `ifChrome` once other major browsers support
-    // text fragments (i.e. 'fragmentDirective' in document = true)
+    // TODO(dmanek): remove `ifChrome` once we remove Chrome version detection
+    it.configure()
+      .ifChrome()
+      .run(
+        'should not highlight using text fragments for Chrome 92',
+        async () => {
+          const {ampdoc} = env;
+          const platform = Services.platformFor(ampdoc.win);
+          env.sandbox.stub(platform, 'isChrome').returns(true);
+          env.sandbox.stub(platform, 'getMajorVersion').returns(92);
+          let whenFirstVisiblePromiseResolve;
+          const whenFirstVisiblePromise = new Promise((resolve) => {
+            whenFirstVisiblePromiseResolve = resolve;
+          });
+          env.sandbox
+            .stub(ampdoc, 'whenFirstVisible')
+            .returns(whenFirstVisiblePromise);
+
+          const highlightHandler = new HighlightHandler(ampdoc, {
+            sentences: ['amp', 'highlight'],
+          });
+
+          const updateUrlWithTextFragmentSpy = env.sandbox.spy();
+          highlightHandler.updateUrlWithTextFragment_ =
+            updateUrlWithTextFragmentSpy;
+
+          whenFirstVisiblePromiseResolve();
+          await whenFirstVisiblePromise;
+
+          expect(updateUrlWithTextFragmentSpy).not.to.be.called;
+        }
+      );
+
+    // TODO(dmanek): remove `ifChrome` once we remove Chrome version detection
     it.configure()
       .ifChrome()
       .run(
         'should not highlight if highlightInfo.sentences is empty',
         async () => {
-          toggleExperiment(env.win, 'use-text-fragments-for-highlights', true);
           const {ampdoc} = env;
+          const platform = Services.platformFor(ampdoc.win);
+          env.sandbox.stub(platform, 'isChrome').returns(true);
+          env.sandbox.stub(platform, 'getMajorVersion').returns(93);
           let whenFirstVisiblePromiseResolve;
           const whenFirstVisiblePromise = new Promise((resolve) => {
             whenFirstVisiblePromiseResolve = resolve;