Skip to content

🐛 [amp-story-page-outlink] Force page outlinks to use target='_top' in order to prevent navigation from breaking on Safari#36428

Merged
coreymasanto merged 5 commits intoampproject:mainfrom
coreymasanto:outlink
Oct 21, 2021
Merged

🐛 [amp-story-page-outlink] Force page outlinks to use target='_top' in order to prevent navigation from breaking on Safari#36428
coreymasanto merged 5 commits intoampproject:mainfrom
coreymasanto:outlink

Conversation

@coreymasanto
Copy link
Copy Markdown
Contributor

@coreymasanto coreymasanto commented Oct 20, 2021

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.

Before this PR, swiping up an amp-story-page-outlink with target='_blank' on iOS Safari would result in a failed navigation and a broken story page. After this PR, any amp-story-page-outlink will successfully open in the same tab as its story.

@gmajoulet gmajoulet marked this pull request as ready for review October 20, 2021 16:28
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Oct 20, 2021

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story-page-attachment.js
extensions/amp-story/1.0/test/test-amp-story-page-attachment.js

@gmajoulet
Copy link
Copy Markdown
Contributor

gmajoulet commented Oct 20, 2021

If you have time to add a unit test that'd be cool :))

…nk tests, along with a test verifying that outlink anchor elements always have a value of '_top'
@coreymasanto
Copy link
Copy Markdown
Contributor Author

If you have time to add a unit test that'd be cool :))

Added a new test file!

Copy link
Copy Markdown
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants