Skip to content

Conversation

@hypest
Copy link
Contributor

@hypest hypest commented Jun 8, 2018

Fix #460

This PR mirrors the behavior from Calypso to handle email links and links that need the scheme added.

Test 1

  1. Open the demo app and tap on the toolbar to insert a link
  2. In the URL field type: name@company.com, copy the same to the Link text field and tap OK
  3. Switch to html and notice that the href points to: mailto:name@company.com (prepended with mailto:)

Test 2

  1. Open the demo app and tap on the toolbar to insert a link
  2. In the URL field type: #internalanchor, copy the same to the Link text field and tap OK
  3. Switch to html and notice that the href points to: #internalanchor (no http:// added)

Test 3

  1. Open the demo app and tap on the toolbar to insert a link
  2. In the URL field type: wordpress.org, copy the same to the Link text field and tap OK
  3. Switch to html and notice that the href points to: http://wordpress.org (there's an added http://)

@hypest hypest requested a review from daniloercoli June 8, 2018 16:27
}
}

private val REGEXP_EMAIL = Regex("^[A-Z0-9._%+-]+@[A-Z0-9.-]+.[A-Z]{2,4}$",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top level domains can be longer than 4 characters now (e.g. .design)

Copy link
Contributor Author

@hypest hypest Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @koke . Any recommendation for how long to change it to? Some cursory googling has produced info for TLDs that can be 24chars long. Localized TLDs in particular start with XN-- which is 4char long to begin with!

I think I will change it to 20chars.

Copy link
Contributor

@daniloercoli daniloercoli Jun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that there is no length limit set for TLDs. Checked on google, and I think we can safely change it to
"^[A-Z0-9._%+-]+@[A-Z0-9.-]+.[A-Z]{2,}$"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @daniloercoli , lifted the upper limit with 3e9f744.

@daniloercoli daniloercoli self-assigned this Jun 11, 2018
@daniloercoli
Copy link
Contributor

We should also escape the double quote character (" ) when inserted in links, otherwise we're breaking the resulting HTML link.
screen shot 2018-06-11 at 16 38 03

@daniloercoli
Copy link
Contributor

For reference pasting here the link to email tests taken from chromium.

I've also found an interesting post on stackoverflow that could help with Regexps if we decide to investigate this further.

@hypest
Copy link
Contributor Author

hypest commented Jun 11, 2018

We should also escape the double quote character (" ) when inserted in links, otherwise we're breaking the resulting HTML link.

I added code (with 19c40b3) to encode the link text @daniloercoli . Let me know if that's what you had in mind.

By the way, as it currently is, the email regex won't match when there's a doublequote in the link text. How can I reproduce the case in the screenshot you shared above?

For reference pasting here the link to email tests taken from chromium.

I've also found an interesting post on stackoverflow that could help with Regexps if we decide to investigate this further.

Aha, thanks for looking into this! I feel though that an extensive support like that is outside the scope of this PR. Can we do it in a separate PR? If OK with you, can you open a ticket with the support you'd like to see implemented? Thanks!

@daniloercoli
Copy link
Contributor

Aha, thanks for looking into this! I feel though that an extensive support like that i outside the scope if this PR. Can we do it in a separate PR? If OK with you, can you open a ticket with the support you'd like to see implemented? Thanks!

I agree and will open a new ticket.

@daniloercoli
Copy link
Contributor

By the way, as it currently is, the email regex won't match when there's a doublequote in the link text. How can I reproduce the case in the screenshot you shared above?

If you try by inserting dan"@text.com you will see there are 2 errors:

  • The http:// prefix is added to it
  • The double quote is encoded fine. if you switch to HTML -> Visual -> HTML it's then lost (switched it back to plain character, I guess this is the problem with HTML ents we already have, so let's not consider it here for now).

@daniloercoli
Copy link
Contributor

:shipit:

@daniloercoli daniloercoli merged commit 41bf0b2 into develop Jun 12, 2018
@daniloercoli daniloercoli deleted the issue/460-schemeless-urls branch June 12, 2018 08:22
@hypest
Copy link
Contributor Author

hypest commented Jun 12, 2018

Thanks for the merge and for the new ticket @daniloercoli . We can move any regex discussion there I guess, cool?

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

Successfully merging this pull request may close these issues.

4 participants