Skip to content

Commit

Permalink
🐛 [amp-story-page-outlink] Force page outlinks to use target='_top' i…
Browse files Browse the repository at this point in the history
…n order to prevent navigation from breaking on Safari (ampproject#36428)

* Force page outlinks to use target='_top' in order to prevent them from breaking on Safari

* Lint

* Add test-amp-story-page-attachment.js with basic attachment and outlink tests, along with a test verifying that outlink anchor elements always have a value of '_top'

* Lint
  • Loading branch information
coreymasanto authored Oct 21, 2021
1 parent e7602e5 commit 44a18b0
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 4 deletions.
15 changes: 11 additions & 4 deletions extensions/amp-story/1.0/amp-story-page-attachment.js
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,18 @@ export class AmpStoryPageAttachment extends DraggableDrawer {
<svg class="i-amphtml-story-page-attachment-remote-icon" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"><path d="M38 38H10V10h14V6H10c-2.21 0-4 1.79-4 4v28c0 2.21 1.79 4 4 4h28c2.21 0 4-1.79 4-4V24h-4v14zM28 6v4h7.17L15.51 29.66l2.83 2.83L38 12.83V20h4V6H28z"></path></svg>
</a>`;

const isPageOutlink = this.element.tagName === 'AMP-STORY-PAGE-OUTLINK';
if (isPageOutlink) {
// The target must be '_top' for page outlinks, which will result in the
// link opening in the current tab. Opening links in a new tab requires a
// trusted event, and Safari does not consider swiping up to be trusted.
this.element.querySelector('a').setAttribute('target', '_top');
}

// For backwards compatibility if element is amp-story-page-outlink.
const hrefAttr =
this.element.tagName === 'AMP-STORY-PAGE-OUTLINK'
? this.element.querySelector('a').getAttribute('href')
: this.element.getAttribute('href');
const hrefAttr = isPageOutlink
? this.element.querySelector('a').getAttribute('href')
: this.element.getAttribute('href');

// URL will be validated and resolved based on the canonical URL if relative
// when navigating.
Expand Down
56 changes: 56 additions & 0 deletions extensions/amp-story/1.0/test/test-amp-story-page-attachment.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import {AmpDocSingle} from '#service/ampdoc-impl';
import {AmpStoryPageAttachment} from '../amp-story-page-attachment';

describes.realWin('amp-story-page-attachment', {amp: true}, (env) => {
let attachmentEl;
let attachment;
let outlinkEl;
let outlink;

beforeEach(() => {
const {win} = env;

// Set up the story.
const storyEl = win.document.createElement('amp-story');
storyEl.getAmpDoc = () => new AmpDocSingle(win);
win.document.body.appendChild(storyEl);

// Set up the attachment element for inline attachment testing.
attachmentEl = win.document.createElement('amp-story-page-attachment');
attachmentEl.getAmpDoc = () => new AmpDocSingle(win);
storyEl.appendChild(attachmentEl);
attachment = new AmpStoryPageAttachment(attachmentEl);

// Set up the outlink element for outlink testing.
outlinkEl = win.document.createElement('amp-story-page-outlink');
outlinkEl.getAmpDoc = () => new AmpDocSingle(win);
storyEl.appendChild(outlinkEl);
outlinkEl.appendChild(win.document.createElement('a'));
outlink = new AmpStoryPageAttachment(outlinkEl);
});

afterEach(() => {
attachmentEl.remove();
outlinkEl.remove();
});

it('should build an attachment', async () => {
attachment.buildCallback();
return attachment.layoutCallback();
});

it('should build an outlink', async () => {
outlink.buildCallback();
return outlink.layoutCallback();
});

it('should build amp-story-page-outlink with target="_top" even when the publisher has specified a different value', async () => {
const anchorEl = outlinkEl.querySelector('amp-story-page-outlink a');
anchorEl.setAttribute('target', '_blank');

outlink.buildCallback();
await outlink.layoutCallback();

expect(anchorEl.getAttribute('target')).to.eql('_top');
});
});

0 comments on commit 44a18b0

Please sign in to comment.