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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SVCS-423] Links in pdfs should no longer open in the iframe. #273

Conversation

AddisonSchiller
Copy link
Contributor

@AddisonSchiller AddisonSchiller commented Aug 18, 2017

Instead, they should open in a new tab/window

https://openscience.atlassian.net/browse/SVCS-423

Purpose

Fix opening links in an iframe.
Update tests to match fix.

Summary of changes

added default behavior of links to open in new tab, instead of on same page (in iframe)
Changed MFR tests to match this behavior.

QA Notes

Open a pdf that has a link in it (example on jira ticket)
Click on the link
It should redirect you in a new tab, instead of in the iframe.

Mfr pdf tests should pass

@AddisonSchiller AddisonSchiller changed the title Links in pdfs should no longer open in the iframe. [SVCS-423] Links in pdfs should no longer open in the iframe. Oct 5, 2017
Copy link
Contributor

@cslzchen cslzchen left a comment

Choose a reason for hiding this comment

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

Looks good and works as expected. 🎆 Move to PCR.

@cslzchen
Copy link
Contributor

cslzchen commented Oct 10, 2017

@AddisonSchiller FYI, not sure if relevant since I am not familiar with PDF rendering in HTML. I did two tests. I had had doubt in the base tag in the following cases but it turned out I was wrong.

First, I tested a PDF file with bookmark links. It worked as expected (no new tab).
Second, I tested a PDF file with absolute web page link. It worked as expected as well (new tab).

Here is the file that I used: MFR_Test_2.pdf

Copy link
Member

@felliott felliott left a comment

Choose a reason for hiding this comment

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

Hey @AddisonSchiller,

I happened to merge the iframe sandboxing PR first, which has caused this to stop working. Can you play around with the other sandboxing toggles to see if you can get it working again? Thanks.

Cheers,
@felliott

Instead, they should open in a new tab/window

Changing test to match pdf template change/fix
@AddisonSchiller
Copy link
Contributor Author

fixed. Sorta killed the commit for it when merging. Anyway, sandboxing just needed 'allow-popups'.

https://stackoverflow.com/questions/21236616/sandboxed-iframe-with-target-blank-doesnt-open-new-tab-or-window

@coveralls
Copy link

coveralls commented Nov 13, 2017

Coverage Status

Changes Unknown when pulling 7021488 on AddisonSchiller:feature/link-in-pdf-fix into ** on CenterForOpenScience:develop**.

@felliott
Copy link
Member

yaaaaaaaaaaaaaaaaaaaay. merged.

@felliott felliott closed this in 9e9fc26 Nov 14, 2017
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.

None yet

4 participants