Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Oct 21, 2021

Conversation

coreymasanto
Copy link
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

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
Contributor

gmajoulet commented Oct 20, 2021

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

@gmajoulet gmajoulet added this to In progress in wg-stories Sprint via automation Oct 20, 2021
鈥k tests, along with a test verifying that outlink anchor elements always have a value of '_top'
@coreymasanto
Copy link
Contributor Author

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

Added a new test file!

Copy link
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants