🐛 [Story attachment] Opening outlink error for closeButton being null (…

…#37833) * Added tasts * Undo * Fixed outlink error * Remove comment * Added test to make sure outlink doesn't break when opened * Removed unnecessary attribute * calledOnce
ampproject · Mar 8, 2022 · 435c185 · 435c185
1 parent 78e4f95
commit 435c185
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 3 deletions.
diff --git a/extensions/amp-story-page-attachment/0.1/amp-story-page-attachment.js b/extensions/amp-story-page-attachment/0.1/amp-story-page-attachment.js
@@ -454,7 +454,7 @@ export class AmpStoryPageAttachment extends DraggableDrawer {
 
       const pageAttachmentChild = this.element.parentElement
         ?.querySelector('.i-amphtml-story-page-open-attachment-host')
-        .shadowRoot.querySelector('a.i-amphtml-story-page-open-attachment');
+        .shadowRoot?.querySelector('a.i-amphtml-story-page-open-attachment');
 
       if (pageOutlinkChild) {
         pageOutlinkChild.click();
@@ -559,6 +559,10 @@ export class AmpStoryPageAttachment extends DraggableDrawer {
     const closeButton = this.headerEl.querySelector(
       '.i-amphtml-story-page-attachment-close-button'
     );
+    // If attachment is outlink, there is no close button.
+    if (!closeButton) {
+      return;
+    }
     this.mutateElement(() => {
       if (isActive) {
         closeButton.removeAttribute('tabindex');

diff --git a/extensions/amp-story-page-attachment/0.1/test/test-amp-story-page-attachment.js b/extensions/amp-story-page-attachment/0.1/test/test-amp-story-page-attachment.js
@@ -1,10 +1,16 @@
+import {expect} from 'chai';
+
 import * as Preact from '#core/dom/jsx';
 
 import {Services} from '#service';
 import {AmpDocSingle} from '#service/ampdoc-impl';
 import {LocalizationService} from '#service/localization';
 
-import {AmpStoryStoreService} from 'extensions/amp-story/1.0/amp-story-store-service';
+import {
+  Action,
+  AmpStoryStoreService,
+  UIType,
+} from 'extensions/amp-story/1.0/amp-story-store-service';
 import {registerServiceBuilder} from 'src/service-helpers';
 
 import {StoryAnalyticsService} from '../../../amp-story/1.0/story-analytics';
@@ -15,6 +21,7 @@ describes.realWin('amp-story-page-attachment', {amp: true}, (env) => {
   let attachment;
   let outlinkEl;
   let outlink;
+  let storeService;
 
   beforeEach(() => {
     const {win} = env;
@@ -30,7 +37,7 @@ describes.realWin('amp-story-page-attachment', {amp: true}, (env) => {
       return localizationService;
     });
 
-    const storeService = new AmpStoryStoreService(win);
+    storeService = new AmpStoryStoreService(win);
     registerServiceBuilder(win, 'story-store', function () {
       return storeService;
     });
@@ -65,6 +72,10 @@ describes.realWin('amp-story-page-attachment', {amp: true}, (env) => {
     outlinkEl.getAmpDoc = () => new AmpDocSingle(win);
     pageEl.appendChild(outlinkEl);
     outlink = new AmpStoryPageAttachment(outlinkEl);
+
+    env.sandbox
+      .stub(outlink, 'mutateElement')
+      .callsFake((fn) => Promise.resolve(fn()));
   });
 
   afterEach(() => {
@@ -118,4 +129,18 @@ describes.realWin('amp-story-page-attachment', {amp: true}, (env) => {
 
     expect(closeButtonEl.hasAttribute('tabindex')).to.be.false;
   });
+
+  it('should click on anchor when outlink open method is called', async () => {
+    storeService.dispatch(Action.TOGGLE_UI, UIType.DESKTOP_ONE_PANEL);
+    const anchorEl = outlinkEl.querySelector('amp-story-page-outlink a');
+
+    const clickSpy = env.sandbox.spy(anchorEl, 'click');
+
+    await outlink.buildCallback();
+    await outlink.layoutCallback();
+
+    outlink.open();
+
+    expect(clickSpy).to.be.calledOnce;
+  });
 });