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

Fix markdown-it linkify blocking Firefox #5574

Merged
merged 1 commit into from Jan 19, 2015

Conversation

@svbergerem
Copy link
Member

commented Jan 18, 2015

No description provided.

// Replace link-like texts with link nodes.
//
'use strict';

var urlRegex = require('url-regex');
var LINK_SCAN_RE = /www|@|\:\/\//;
var urlRegex = /(?:^|\s)(?:(?:https?|ftp):\/\/|mailto:|xmpp:|www[^.\s]*\.)\S+/gi;

This comment has been minimized.

Copy link
@svbergerem

svbergerem Jan 18, 2015

Author Member

please review especially this line

This comment has been minimized.

Copy link
@jhass

This comment has been minimized.

Copy link
@svbergerem

svbergerem Jan 18, 2015

Author Member

That doesn't support mailto:, xmpp: and wwwb etc. It also doesn't support parenthesis inside of URLs.

This comment has been minimized.

Copy link
@jhass

jhass Jan 18, 2015

Member

I fear this one is a bit too open though: https://www.debuggex.com/r/8859oQ_AME577XSr

We're currently using twitter-text-rb in the backend for URL recognition, it also has a JS implementation, I think it might be worth investigating what they use or maybe even utilizing the library.

This comment has been minimized.

Copy link
@svbergerem

svbergerem Jan 18, 2015

Author Member

I am not sure if this is really much worse than the regex we had before: https://www.debuggex.com/r/4eJ2IFhc7VXnEOKu

I could add a mandatory @ for mailto: and xmpp: in the regex. (mailto:\S+@\S+)
Which protocols does twitter-text-rb support?

This comment has been minimized.

Copy link
@jhass

jhass Jan 18, 2015

Member

The matching is generic (JS, JS, Rb, Rb) but for some reason demands the :// part and is then later restricted to https? (JS, Rb) unfortunately, so we would probably need to fork it.

This comment has been minimized.

Copy link
@Raven24

Raven24 Jan 19, 2015

Member

I remember we had a browser lock-up issue with the old regex in chrome with some greedy expressions, which was a pain to debug.
so any out-of-the-box solution we can find is preferrable ;)

This comment has been minimized.

Copy link
@svbergerem

svbergerem Jan 19, 2015

Author Member

The last out-of-the-box solution was a disaster. (and is the reason why this PR exists) I found no existing solutions so far that had satisfying results.

This comment has been minimized.

Copy link
@svbergerem

svbergerem Jan 19, 2015

Author Member

Please also note that the regex you commented on is not the current one for this PR.

@svbergerem svbergerem force-pushed the svbergerem:fix-linkify branch 2 times, most recently from 8c7784e to b63d8fc Jan 19, 2015

@jhass jhass added this to the next-major milestone Jan 19, 2015

@jhass

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

Let's try this one out for some time.

jhass added a commit that referenced this pull request Jan 19, 2015
Merge pull request #5574 from svbergerem/fix-linkify
Fix markdown-it linkify blocking Firefox

@jhass jhass merged commit 6502000 into diaspora:develop Jan 19, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@svbergerem svbergerem deleted the svbergerem:fix-linkify branch Jan 19, 2015

@jhass jhass referenced this pull request Jan 21, 2015
2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.