Skip to content

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 force-pushed the feature/link-in-pdf-fix branch from bca79ce to 106f60b Compare August 23, 2017 18:59
@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

4 participants