Add linksInNewTab config option of whether or not to open links in a new tab or not

[drmuey]

I tested locally in the project I wanted to use this with:

• by default it behaves the as it does now: links open in the same tab/window
• setting it to true via setOptions: links opens in a new tab/window
• setting it to false via setOptions: links open in the same tab/window

Also, checked the README entry by looking at it on github.

Unit tests would be nice but I don't have time to grok how the tests work in this repo and other stuff is also not unit tested (e.g. smartLists)

[drmuey]

NTS: when finalized and merged go back to CDN (i.e. reverse g show a82fbb3 -- views/layouts && update version)

[julkue]

👍

[chjj]

target="_blank" is deprecated according to the whatwg spec. In any case, is there a situation where you would want a user to open all links in a new tab? Is there a more contemporary syntax for this?

[julkue]

@chjj Could you please provide a reference? For me this seems to be not deprecated (e.g. on HTML5 Spec or W3Schools.

Well of course there are lots of use cases where you want to open specific links (e.g. that were rendered via marked) in a new tab. For example I'm currently building a webapp where informations about documents are shown. These informations are written in Markdown and parsed with marked. However, if the user clicks on a link in the informations about a document he just wants to open that link in the foreground but don't want to leave the app completely (because he may want to open multiple documents). So in fact, this is a very important standard for me and I think for other too.

[chjj]

@julmot, very interesting, the last time I looked at the whatwg spec (probably 2 or 3 years ago), target=_blank was deprecated. I'll reconsider. As a side node, I'd like to see the mailing list posts that got this back into the spec.

[julkue]

Ok so can we integrate target-attribute on links?

[drmuey]

e.g. page content may not want it, user comments definetly want it. I wish github did it in issues/pull requests (but not README.md) so I don't lose my spot in a thread when checking out a related link.

Since it doesn't change default behavior no one will be surprised and its there if they do end up needing it.

If there is a better way than target="_blank" cool, we can just update this change later once that is discovered and everyone will start rendering the latest hotness :)

[posgarou]

This would be helpful, and what @drmuey says seems true (won't break prior code). 👍

[namxam]

Any updates on this? It is not the first time that this is a requirement for a project.

[julkue]

@chjj

[janneri]

I have some use cases where this would be helpful.

Please read .. And then merge :)
http://stackoverflow.com/questions/4198788/is-it-alright-to-use-target-blank-in-html5

[mileung]

linksInNewTab is not working for me. Is there a later version that I'm supposed to install instead of the default version that comes when you npm install?

[rzukow]

Any chances to get this merged ?

[ocdtrekkie]

So, I need this and will be manually pulling code from this PR into a project.

@chjj, if you need another use case, here's one: I'm working on a Sandstorm app package. Sandstorm runs web apps in a sandbox, and restricts their behavior in the frame in which they live. External links only work in Sandstorm apps if they open in a new tab, therefore, all links for this app must have this behavior.

Code I am integrating from a non-Sandstorm version of this app uses marked.js, so in order for their code to work correctly in the Sandstorm version, I need this option.

[ocdtrekkie]

I have tested this PR in a project, and it works great and solves my problem.

[joshbruce]

Closing as stale, merge conflicts, and not sure if defined by targeted specs (see #956).

[ocdtrekkie]

It wouldn't be stale if it hadn't been ignored. This is a feature multiple projects have had to patch in as part of using this project.

[ocdtrekkie]

This is a key need, and really belongs on some sort of roadmap, even if this PR is now too old to be merged directly.

[joshbruce]

@ocdtrekkie: Not ignored - the Marked project itself also went stale - see #956.

Thank you for the feedback. Would you be willing to create an issue? Maybe link to where in the CommonMark or GFM specifications the target="_blank" capability is handled?

[joshbruce]

ps. I'm also inclined, as appears to be @chjj (the owner of the project - see previous comment), to question why we would modify Markdown, in general, to have that capability. It begins to tighly couple (limit) Markdown to literal HTML output and would want to see the discussion around that decision.

[ocdtrekkie]

chjj's primary concern above was whether or not target="_blank" was deprecated. It's still the way things are done today though, and other commenters satisfactorily responded that it was not.

My point is primarily usability: A significant percentage of people parsing content with Markdown will be doing so inside apps that process user content... where they are likely to want to be able to implement Open in New Tab.

As designed, this feature:

• Significantly improves usefulness of Marked, to the extent that some of us use monkeypatched versions of Marked.
• Does not negatively impact users who do not need or want it, as purely optional functionality.

It is hard to argue that an optional feature "tightly couples" or "limits" the project in any way, especially when there's significant practical use for the feature.

I mean, to be honest, I don't have the time to invest significantly in pursuing and arguing this further, but a "mark as stale and close" something with significant user interest and impact, despite being offered up as a PR that worked as it was, was something that warranted commentary.

[ocdtrekkie]

Out of curiosity, what would you expect Marked users who require this functionality to do to solve this issue other than add this option?

[joshbruce]

@ocdtrekkie: That's fair regarding not wanting to make the time to argue it further; having said that, my natural curiosity requires me to ask: Why open the can then?

• We are currently resolving issues related to Marked complying with the CommonMark and GitHub Flavored Markdown specifications - neither of which have this capability.
• "Marked as stale" was only one reason to close - merge conflicts and non-compliance with specs was also cited.
• As to the significant percentage point, I do not have these figures. Marked gets, roughly, 1M downloads a week - of those, I'm not sure how many individuals that is (as opposed to one person downloading 1M times). Further, of those individuals, I'm not sure how many want this feature to exist. (After culling through hundreds of issues and PRs, I'm not seeing a lot of mentions or requests for this functionality.)
• As a user experience expert, I would also be curious to hear the use cases for the use of target blank (we used to have this debate a lot back in 2005). Facebook, for example, makes sense, because I'm leaving my feed to go somewhere else to possibly come back quickly and paste something; however, many of the use cases I see still revolve around, for lack of a better term, ego...I don't want the user to leave my site. Further, from what I remember (don't have the references), many users know how to open links in new tabs and whatnot - unlike 2005 and prior where almost every link came with instructions.
• As to the final point coupled with the point regarding user input: Some editors allow users to specify that links open in a new window or tab...at which point, those should probably be saved as raw HTML. If a site (like a forum) wants to make all the links target balnk, post-processing would probably be better suited (see V0.3.6,<h1>,<h2>,<h3>,<h4>,<h5>,<h6>sometimes id are same #919). That's what makes Marked a Markdown processor - not a full HTML generator or template engine (again, mentioned in Hi, I'm Josh, I'll be your co-pilot for at least a bit #956).
• As to the final point/question by itself: Markdown allows users to pepper HTML within the Markdown. If the link should open in a new tab - write it in HTML. This PR seems to be an all or nothing solution - all the links are target blank - or none of them are. Using HTML elements within the Markdown is perfectly "legal" according to all the specs - and there is no way, I'm aware of anyway, to write, in Markdown, that the link should be target blank:

[something](/uri "title")

// results in
<a href="uri" title="title">something</a>

If one of the specs had something like a flag to set it might be a different subject.

[something](/uri "title" true)

// does not result in
<a href="uri" title="title" target="_blank">something</a>

But, I don't think that will happen anytime soon because it takes Markdown from being a generic way to markup a document (without HTML) and turns it into a way to write HTML specifically. imho

[christiansimms]

I'm one of those people who had to monkeypatch the marked.Renderer link function to add target="_blank". I can live with my monkeypatch, though of course if the link function is changed significantly then my code will break.

@joshbruce You might have lots of downloads, and not hear much about this issue, because this is sort of a low-level library, so when it's used, it's just kind of buried deep in a project. Not many people rummage around in the config options of all their included libraries to look for one specific option.

Also, about the UI history of _blank -- yes, it can be abused, and has been. However, here's my use-case: I want to use it site-wide on my site where links are for help, and are not the kind of link where I'm possibly sending the user away to read another tangential article from my own.

[joshbruce]

@christiansimms: I concur with the low-level assessment - and why the downloads might be high despite people not even being aware they are using it. So far, the hope is to keep it that way - #965. Of course, that also gives us more reason not to include features that go too far afield of the specs - for better or worse.

All the changes we are making now are done with the hope of maintaining backward compatibility. We also have plans to make Marked a little bit easier to customize - maybe the target blank possibility could be there.

Thank you for the use case. What's the rationale behind using Marked to do that during processing instead of something like this after the fact:

document.getElementsByTagName('a').forEach(function(elem) {
    elem.setAttribute('target', '_blank');
});

Not tested or anything, just asking the question. This way the functionality of setting target blank is completely independent of Marked...you could grab a different Markdown processor and not have to change that bit?? And, of course, you can add exclusion logic and cases.

Again, just curious to gain a better understanding of the rationale behind the choice to use Marked for this functionality.

[ocdtrekkie]

@joshbruce Again, I don't see any sense or reason in your justification of "keeping people unaware they are using it", as a reason to neglect a completely optional feature that has no effect by default. Similarly, there is no backward compatibility concern for a feature that explicitly requires the developer enable it, by setting an option flag that didn't exist before.

I also specified my use case over a year ago, and it is similarly not for "ego" reasons: I ported an app that uses Marked (called Scrumblr) over to Sandstorm.io, a platform for web apps, where the apps operate inside a frame. Sandstorm.io does not permit external links or content in the frame for security reasons, so external links only actually work if they have target="_blank".

Your solution may work, I don't know. I am not super familiar with Scrumblr's code to begin with. This PR was an incredibly easy to use solution to the problem at hand.

[joshbruce]

@ocdtrekkie: My reply was mainly for @christiansimms to gain better understanding for his use case and rationale, apologies for not making that more clear.

Having said that, have I fundamentally wronged you in some way here?

You seem awful antagonistic and I'm trying to figure out why. You appear to be trying to make this personal (happens a lot in our industry, unfortunately). As though I said, "Oh, he wants it, therefore, I'm just gonna close it." And your language choice of "your justification" - "your solution" and so on further seems to bring up feelings of "making it personal". I also never said I wanted to "keep people unaware" - I was agreeing with @christiansimms that most people, in general, just are simply unaware; and that Marked is a low-level library with a single responsibility (SOLID). Further, we do have plans to make it more open to extension; therefore, someone can create a wrapper to customize the output...as opposed to asking Marked to do more than quickly and accurately comply with various Markdown specifications.

I don't know what battles you are fighting but this one doesn't seem to be with me. (I also wasn't here a year ago and have read, reviewed, triaged, closed, and merged over 600 issues and PRs in the last ~45 days with help from some wonderful contributors; so, please afford me - and us - the courtesy of at least a respectful and civil discourse - I don't have to be here trying to help ressurect this project or community - and neither do any of the rest of us. We can all go back to not getting any new releases and having over 700 issues and 300+ PRs. Thank you for your consideration on this matter.)

I don't know anything about Sandstorm.io and how it works; therefore, I am unable to help on that score. Having said that, it's definitely something to consider once we get beyond the hurdles of complying with CommonMark and GFM (0.4.0 milestone) and move into the 0.5.0 milestone. Maybe @Feder1co5oave, @UziTech, or @KostyaTretyak can be of assistance - again, as I don't know Sandstorm.io at all, but definitely understand the security desire...having said that, if a simple target blank can make it happen, I do question how secure that really is.

[ocdtrekkie]

I am not attempting to be antagonistic, but you seem to be repeatedly bringing up justifications for not implementing this feature that are not accurate. (Such as backwards compatibility being a concern with a feature that is 100% backwards compatible with existing users since it does nothing unless explicitly set.)

"Your justification" and "your solution" is hardly an antagonistic language choice, I was referring to your justification and your solution, which hence, uses the pronoun "your". (As opposed to something like "the" solution, which would sound like a definitive result, which does not exist in a discussion.)

The point of target=_blank being required in Sandstorm is that anything inside the Sandstorm frame should be on Sandstorm's server. If an external page could be present inside Sandstorm, it could emulate the app you are using, while actually being on someone else's server. Mind you, the point is less that "target=_blank is important for security", so much that it is "target=_blank is needed for external links to work in Sandstorm", as an explanation for why I need it.

This was important to explain because you seemed to suggest (if I may argue, slightly antagonistically) that the primary use case for target=_blank may be "for lack of a better term, ego".

[ocdtrekkie]

Note that I understand you are trying to get this project back on track, and I sympathize with the amount of work you are going through. I just think this issue should not have been closed, when it significantly deserves revisiting, and while merge conflicts exist for this PR, it is likely this PR would be the base of any resubmission of this feature that is merge-able with the current version.

[ocdtrekkie]

Also, since it's exceedingly small of a patch, I compared the code of this PR to the current version of Marked: The merge conflict is in the readme, specifically that since this PR was made documentation of the xhtml: option was added in the same spot the linksinNewTab option would also be documented.

There is no code conflict.

[ocdtrekkie]

Oh, also where defaults are set, the baseUrl: has been added since, where linksinNewTab: is added as well. But again, these aren't really code conflicts, such as a minor editing issue since the PR is so old.

https://github.com/chjj/marked/blob/master/lib/marked.js#L1333

[christiansimms]

@joshbruce Yes your post-processing code should work. However, I'm using a JavaScript framework (Angular here, but there are similar others like Vue) and wrote a component which runs marked. In general with these frameworks, you're not supposed to directly change the Html (since these frameworks do low-level things to the html inside the browser, like virtual Dom, making intermediate html tags/attributes, etc.).

[joshbruce]

Thank @ocdtrekkie - I understand that you don't believe it should have been closed. Having said that, ultimately, I do and do kind of act as a tie breaker.

1. Our focus is on fixing known issues; this does not fix a known issue with spec compliance. (See Hi, I'm Josh, I'll be your co-pilot for at least a bit #956, [Vision alignment]: Comparative benchmarking - high-speed #963, [Vision alignment]: Comparative benchmarking - lightweight #964, and [Vision alignment]: Comparative benchmarking - low-level #965)
2. Our focus is on complying with two known specifications (CommonMark and GFM); this does not comply with either of those specifications.
3. We require tests be present for all PRs that can have them present to confirm solution validity and mitigate future regression; this PR does not comply with that standard.
4. If there are merge conflicts I cannot push the big, green shiny button; this PR has merge conflicts - even if they are just in the README.

If you would like this functionality, create an Issue and we can consider the proposal. Until then, the decision to close this PR will stand for a combination of reasons. Another submitter of a PR that got closed in the last wave made the comment that he could not justify keeping the branch alive and up to date since there was no movement (notice the submitter, @drmuey, is not here making the arguments or fixes to the PR?).

(Apologies, but now it's my turn to be slightly atagonistic, I thought you didn't have time for this? Yet here we are.)

@christiansimms: Thanks. Yeah. My project uses Angular right now and we are going to be using SimpleMDE as an editor - SimpleMDE uses Marked. I try to adhere to what I call the community principle (if you create or acquire it, you help maintain it) - that's why I came here actually. That sounds like an interesting component. I thought Angular components lived in the shadow DOM (or whatever marketing is calling it these days).

Therefore, we should still have the ability to observe and respond to click events - at which point we don't need target blank in the anchor - instead could use prevent default and go from there. Can you post a Gist?

[ocdtrekkie]

Well, I don't have a way to address point 3, but point 4 is now in #1030.

[joshbruce]

@ocdtrekkie: Saw that. Thank you very much! Will label it shortly.

[ocdtrekkie]

@joshbruce I am trying to figure out how to make a test. Is it just to have a .md file that implements the feature, and then the .html being an output of it?

The Smartypants test (https://raw.githubusercontent.com/chjj/marked/master/test/new/smartypants.md) seems to document how to enable an option, so this should be easy to add.

[drmuey]

Happy to freshen the pull request so it's clean and add tests (I didn't see PR requirements doc, apologies).

Regarding UX:

1. by default there'd be no behavior change so no one would be affected
2. It'd allow anyone w/ a legit use of target to not have to maintain a patched version of marked.

Regarding MD specs:

1. its a config option for rendering not a MD syntax change
2. http://spec.commonmark.org/0.28/#links doesn't prohibit adding a target on rendering
3. https://github.github.com/gfm/#links doesn't prohibit adding a target on rendering

[christiansimms]

@joshbruce I made a simplemde component, it's here: SimpleMDEComponent.

As for your idea of overriding click events -- I don't think I can. My use case is: trusted users create Markdown content using SimpleMDE, and I save that in a database. Later, other users view the Html rendering of that content. That's when I want to change the link behavior. I use marked to dump the html rendering into the browser. That rendering is sort of an opaque blob (I just jam the whole thing as one big string to [innerHTML]), so I cannot intercept click events using Angular. (Unless I use jQuery to post-process displayed Html, which they recommend against in Angular.)

[joshbruce]

@drmuey: Can you confirm, this is an all or nothing solution as well? (Also see #1030, if you haven't already.) Appreciate the verification regarding a config setting - not part of a spec. Our focus right now, per #956, is on complying with specs - not new features; so, this gives us time to consider and weigh the options.

One of the things I noticed from reading all the issues related to why Marked went stale is that it seemed like we started adding a lot of new features - those features started introducing bugs and whatnot. This seems to have overwhelmed the community; so, it makes me slightly concerned about extending too far beyond the specs - none of us really has time to mantain a library that can do all things for all people (for lack of a better way of saying it). Having said that, we've added #1030 to the 0.5.0 milestone, which will begin ramping up after the 0.4.0 milestone is sorted (no known issues with CommonMark and GFM).

@christiansimms: Is this the code (or component) you would like this functionality to exist?

[christiansimms]

@joshbruce Sorry that code is not directly using marked, I just linked it because you said you were thinking about using SimpleMDE in the future.

My code which uses marked is in that same repository, but not easily linked (it's actually just a function and not a real Angular component by itself), and I'm sure you're busy. I tried to explain how I use marked, and believe it's a common way that people use it. Just trying to explain why I hope this feature is eventually added.

[Feder1co5oave]

We seem to be creating conflict over breadcrums guys 😆

@joshbruce is right about one thing: it's better to decouple as mush as possible marked from the generation of html code. We should focus on parsing, and that's what specifications are about. In fact they never dictate how to generate html, but disambiguate how markdown code is to be interpreted into an abstract syntax tree. From that, you then generate output however you want.
Still, marked is a markdown-to-html converted...

Renderers are good. They allow you to change the way marked generates html code. The simpler the default renderer, the easier it is to override it.
I would have suggested to override the link default renderer, but it is in fact quite a piece of code. It takes into consideration the sanitize and baseUrl options. If you want the possibility to use these features, you either reimplement them in the override, or you find a way to call the default renderer and then change its output, as in:

var renderer = new marked.Renderer();
renderer.link = function(href, title, text) {
   var out = marked.Renderer.prototype.link.apply(this, arguments);
   return out.replace(/^<a/, '<a target="_blank"');
}

But this is ugly. So let's say we add the linksInNewTab options. The default link renderer gets even messier, and so harder to override. So, at the next feature request, we'll need to aggravate it even more. Consider this.

Yet, this is a feature being requested since a long time ago. And it doesn't change the default behavior.
(But it does increase the complexity. And it couples marked to html even more.)

I personally would never use it as it is. I hate when new tabs are opened without a good reasons. But it is sometimes logical to open a link in a new tab. For example if it is an external link. So if we decide to merge this, why not empower it and provide a regex as an alternative to a simple boolean, such that when links match the regex, they get opened in a new tab? Too much?