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

Don't open print links in a new window #917

Closed
wants to merge 1 commit into from
Closed

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Feb 27, 2017

Print links should use target attribute not JS if they want to open in a new window

* Target _blank has a vulnerability:
https://mathiasbynens.github.io/rel-noopener/ and
alphagov/whitehall#2942
* Links don’t need to be in a new window
@nickcolley
Copy link
Contributor

@nickcolley nickcolley commented Feb 27, 2017

So I know some of our print pages do funky stuff on a new page.

For example:

  1. Visit https://www.gov.uk/become-lorry-bus-driver
  2. Scroll to the bottom
  3. Click 'Print entire guide'
  4. New page opens, with print dialogue.

Would this break that functionality?

It's also worth noting the print links should not go to an external domain.

@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 27, 2017

@nickcolley Nothing breaks. The link would act like a normal in-page link. The auto-print uses a window.onload function and is unaffected.

  1. Visit https://www.gov.uk/become-lorry-bus-driver
  2. Scroll to the bottom
  3. Click 'Print entire guide'
  4. New page opens, with print dialogue.
@nickcolley
Copy link
Contributor

@nickcolley nickcolley commented Feb 27, 2017

I don't feel so comfortable with this change because I don't have lots of devices to test this against, printing behaves very different depending on the operating system so I worry this would introduce an odd behaviour.

That being said, I've not seen an evidence of that testing to make this change in the first place.

Will let someone else also have a look. 👍

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Mar 3, 2017

My strength of opinion on this is low, 3/10 for context.

  1. Is it still a security issue if the opened link is controlled by us? Or is this about consistently avoiding target="_blank"?

  2. Is there anyone with context of the original change? Or anyone who's observed user research around print since then?

My reckon would be that it opens in a new page to reduce the chance of users getting stuck on the print page - as we know some users have difficulty navigating via the back button.

I'm not sure the reasons given for the change are convincing enough to make it.

There might be other benefits/context i'm not aware of though.

@36degrees
Copy link
Member

@36degrees 36degrees commented Mar 3, 2017

Links don’t need to be in a new window

I get the whole 'give users control over whether things open in a new window or not', but when it comes to print links specifically I think there's a pretty common pattern for doing so. I'm not convinced that we should remove it without further research.

@fofr
Copy link
Contributor Author

@fofr fofr commented Mar 3, 2017

Main focus of this PR should have been:

Print links should use target attribute not JS if they want to open in a new window

Closing this until we have PRs to add attributes where needed.

@fofr fofr closed this Mar 3, 2017
@36degrees 36degrees deleted the dont-open-print-windows branch Mar 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.