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

Open in New Window option for links does not exist #55

Closed
Quirksmode opened this issue Oct 22, 2015 · 39 comments
Closed

Open in New Window option for links does not exist #55

Quirksmode opened this issue Oct 22, 2015 · 39 comments

Comments

@Quirksmode
Copy link

No description provided.

@jmz-b
Copy link

jmz-b commented Oct 22, 2015

+1

@codeclown
Copy link

In the context menu? Shows up for me in Safari 8.0.8.

Edit: I'm not a smart person.

@adamschwartz
Copy link

@codeclown I’m guessing @Quirksmode means an option in the editor to add target="_blank" to a link.

@Quirksmode
Copy link
Author

That's correct @adamschwartz, whilst I don't agree it's best practice, I am yet to meet a client who doesn't want the ability to open their links in a new window. A simple checkbox below the input where you add the url would suffice :-)

@adamschwartz
Copy link

@Quirksmode Absolutely I think it’s a very reasonable suggestion. 👍

@hrishimittal
Copy link

Yes please!

@sstephenson
Copy link
Contributor

Thanks for the feature request, but I don’t think we’ll be adding this to Trix. If you need to add a target attribute to links, I’d suggest post-processing the HTML when you render it in your application.

@ryan-kimber
Copy link

This would be a great, and trivial feature to add, while adding post-processing to any application that would like this feature is definitely more complex.

Would you reconsider?

@javan
Copy link
Contributor

javan commented Aug 10, 2016

You could also handle this client side instead of post-processing your HTML:

// Open all external links in a new window
addEventListener("click", function(event) {
  var el = event.target

  if (el.tagName === "A" && !el.isContentEditable && el.host !== window.location.host) {
    el.setAttribute("target", "_blank")
  }
}, true)

@fsamir
Copy link

fsamir commented Nov 2, 2016

IMO, Workaround suggest to post-process is poor. I might want to create links to external content and internal content.

@bostondevin
Copy link

+1 yeah - i'll need this one too

@ksarna
Copy link

ksarna commented Oct 11, 2017

If anyone is looking for a quick way to fix this in rails. Here's what you can do:
(needs nokogiri)

module TrixHelper
  # found at stack overflow: https://stackoverflow.com/a/41557234/244089
  def change_a_target(body)
    doc = Nokogiri::HTML(body)
    doc.css('a').each do |link|
      link['target'] = '_blank'
      # To avoid window.opener attack when target blank is used
      # https://mathiasbynens.github.io/rel-noopener/
      link['rel'] = 'noopener'
    end
    doc.to_s
  end

  def sanitize_trix_html(html)
    sanitize(change_a_target(html), tags: %w(strong a), attributes: %w(href target rel))
  end
end

Then in your view you can:
<%= sanitize_trix_html(@post.body_html) %>

@goravbhootra
Copy link

as the usage is growing, I am facing limitations with trix and this is one of them. The other one is inability to set constraints like required for the input boxes.

@elsurudo
Copy link

+1 on this one... it's a very common use-case, and doing this outside the editor (post-processing) seems like a hack. It seems a a strange decision to reject it outright like this.

@crimson-knight
Copy link

Adding my +1 to make this feature part of Trix.

@lewaabahmad
Copy link

+1

Is there a philosophical reason this is not to be developed? As in, if a PR were to come in for this, would it be rejected?

@RicottaZhang
Copy link

+1

Post process only handle trix html after, we also need it to target blank when editing.

@RicottaZhang
Copy link

RicottaZhang commented Jul 8, 2020

FYI, at last I add it works for editor and show trix content.

$(document).ready(function() {
    $(".trix-content a").click(function(e) {
        $(this).attr("target","_blank");
    });

  // For those content inject by javascript, use an event handler for your trix container may be better.
  $(YourTrixContainerSelector).on('click', '.trix-content a', function(e) {
        $(this).attr("target","_blank");
   });
});

P.S. I prefer stick the solution to Javascript. So it can turn on case by case for each view.

@yshmarov
Copy link

@RicottaZhang this workaround works well. Thank you. All Trix links are opened in a new tab.

@JoshCheek
Copy link

@RicottaZhang's solution didn't work for me b/c in some places, our content gets put on the page by JavaScript, so it isn't there when the page loads.

This is the best I've come up with:

# file: app/helpers/trix_helper.rb
module TrixHelper
  SCRUBBER = Loofah::Scrubber.new do |node|
    node[:target] = '_blank' if node.name == 'a'
  end

  # Overriding https://github.com/rails/rails/blob/da795a678b341adc8031c4d9cf0b5ef2176e2a5a/actiontext/app/helpers/action_text/content_helper.rb#L17
  def sanitize_action_text_content(content)
    original_scrubber, self.scrubber = self.scrubber, SCRUBBER
    super
  ensure
    self.scrubber = original_scrubber
  end
end

@PQALAB
Copy link

PQALAB commented May 27, 2021

I was able to easily serialize my data coming from ActionText to accomplish this. Maybe it'll help someone else out

app/serializers/blah_serializer.rb
def content
    unless object.content.body.links.empty?
      html = Nokogiri.parse(object.content.body.to_html)
      html.css('a').each do |link|
        link["target"] = "_blank"
        link['rel'] = 'noopener'
      end
      html.to_s
    else
      object.content.to_s
    end
  end

@asecondwill
Copy link

For some use cases (all the time for me), the user needs to be able to choose new window. Post processing with ruby or js to open in new window for all links or for external links is to broad brush. needs option to choose.

If I make my own button to add links (simple enough) how can I stop trix stripping my target=_blank ?

@garytaylor
Copy link

Adding a +1 - this is a requirement for my project too. Whilst I can do a workaround for now, I can see that we will need to ask as part of the link dialog as to whether the link should open in a new window or not

@siax84
Copy link

siax84 commented Oct 27, 2022

It is very hard to believe this is not an option on a per-link basis and is not even considered after people suggest it.

@SebSwan
Copy link

SebSwan commented Dec 29, 2022

+1

1 similar comment
@marcelosantos89
Copy link

+1

@spaquet
Copy link

spaquet commented Apr 4, 2023

For those of you wondering if there is a Stimulus approach to this problem I found the following answer on StackOverflow:

  1. Add a Stimulus controller richtext_controller.js (rails g stimulus richtext)
import { Controller } from "stimulus"

export default class extends Controller {
  connect() {
    this.element.querySelectorAll('a').forEach(function(link) {
      if (link.host !== window.location.host) {
        link.target = "_blank"
      }
    })
  }
}

And wrap your rich text display as follow:

<div data-controller="richtext">
  <%= post.rich_text_content %>
</div>

Source: https://stackoverflow.com/questions/65985735/how-to-open-action-text-trix-attachments-or-links-in-new-window-target-blank

@crespire
Copy link

crespire commented May 8, 2023

Yeah, I ended up taking the stimulus approach as well, but what a poor thing to have to do for such a basic feature.

@StanBarrows
Copy link

+1

@wawer77
Copy link

wawer77 commented Jun 22, 2023

Rather pointless, but anyway - I'd like to emphasise that proposed post processing solutions work only for internally rendered links. It would be much cleaner to hold Rich Text with everything we need, so it can be reused and rendered elsewhere.

EDIT:
Actually, I have found a more generic workaround for Rails app. Based on this article.

Create a config/initializers/action_text.rb file and allow target attribute

ActionText::ContentHelper.allowed_attributes += ['target']

Then in your model, which has_rich_text :attribute create a callback before saving, which will add the attribute:

content.gsub('<a href', '<a target="_blank" href')

@binarygit
Copy link

Thank You @spaquet for the snippet but it is disappointing that we need to use a hack for such a basic thing.

@cheesegrits
Copy link

Another +1 for this feature. Hard to believe it was simply dismissed out of hand back in 2015, and never reconsidered. Literally every project I work on, the client want to be able to optionally open embedded links in a new tab.

@jromainkrupa
Copy link

A mix of @wawer77 and @PQALAB approaches with a concern to include in your model. adding a before_save on all has_rich_text attributes of the class.

# app/models/concerns/rich_text_target_blank.rb
module RichTextTargetBlank
  extend ActiveSupport::Concern

  class_methods do
    # Override has_rich_text to include target="_blank" functionality
    def has_rich_text(name)
      super(name) # Call the original has_rich_text to set up the rich text association

      # Define the before_save callback to modify the links
      before_save do
        rich_text_attribute = self.send(name)
        if rich_text_attribute.present?
          doc = Nokogiri::HTML::DocumentFragment.parse(rich_text_attribute.body.to_html)
          doc.css('a').each { |a| a['target'] = '_blank' }
          rich_text_attribute.body = doc.to_html
        end
      end
    end
  end
end

@volstas
Copy link

volstas commented Oct 16, 2024

I'm working on a fork of trix, and added code in the library to control this. Curious if PR is worth it and approach like this would be accepted?
I didn't like modifying the end result manually or on rendering and wanted my output to be correct from beginning.

The implementation is simple, you can set attributes property like this:
Trix.config.textAttributes.href.attributes = { target: "_blank" }

The only actual code change in the library is in piece_view.js:

  createContainerElement() {
    for (const key in this.attributes) {
      const value = this.attributes[key]
      const config = getTextConfig(key)
      if (config) {
        if (config.groupTagName) {
          const attributes = {}
          attributes[key] = value

          if (config.attributes) { // <--- This is new 
            Object.assign(attributes, config.attributes)  // <--- This is new 
          }  // <--- This is new 

          return makeElement(config.groupTagName, attributes)
        }
      }
    }
  }

So basically those 3 lines, one of which one is a closing brace, allows the users to add whatever custom attributes they want purely through a config.
Now this won't really work if you want it to be dynamic, where you allow users to change it by themselves, but it works well as an entire document default values type of thing.

You could technically change the config programmatically with some checkbox, but once you undo/redo it will use the latest config value. But even with this issue, I think the solution is clean and will allow library consumers to at least somewhat control this?

@brendon
Copy link

brendon commented Nov 7, 2024

Something needs to be done I think :) I'll have a play and see if there's a straightforward way to do this in a way that allows users to choose each time :)

@brendon
Copy link

brendon commented Nov 7, 2024

I think the fundamental hard thing about this is that the toolbar module wasn't built to do more than update a single attribute gleaned from a form input:

setAttribute(dialogElement) {
const attributeName = getAttributeName(dialogElement)
const input = getInputForDialog(dialogElement, attributeName)
if (input.willValidate && !input.checkValidity()) {
input.setAttribute("data-trix-validate", "")
input.classList.add("trix-validate")
return input.focus()
} else {
this.delegate?.toolbarDidUpdateAttribute(attributeName, input.value)
return this.hideDialog()
}
}

It's 'abstracted' to some point but has only been created for this one (built in) use case given there's no other toolbar interfaces like the link insertion one.

I'll have a go at extending these methods to cope with more than one attribute at a time. The fallback will be to just replace the default link insertion dialog with one that kicks off a stimulus controller. I've done this for YouTube videos.

@brendon
Copy link

brendon commented Nov 11, 2024

I've created a PR, please lend your support if you are able to figure out the last little bits.

#1197

@silva96
Copy link

silva96 commented Nov 28, 2024

I did this in an initializer, it's better than the stimulus approach because it works for search engines to prevent following links in user generated content .

config.after_initialize do
  ActionText::ContentHelper.scrubber = Loofah::Scrubber.new do |node|
    if node.name == 'a'
      node[:target] = '_blank'
      node[:rel] = 'noopener ugc nofollow'
    end
end

@brendon
Copy link

brendon commented Nov 28, 2024

That's a good option if you want all links to open in a new tab. Still hoping someone will review my PR that allows the editor user to choose...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests