Implementing Relative Links/Images/Emails in MarkdownTextBlock #1639

avknaidu · 2017-11-20T19:09:00Z

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build or CI related changes
[ ] Documentation content changes
[ ] Sample app changes
[ ] Other... Please describe:

What is the current behavior?

Images are not rendered when set to MarkdownTextBlock as (/Assets/Image.png)
Masking Emails was not an option before.
Relative Links were not masked properly before. See Missing support for relative reference link and svg image in MarkdownTextBlock #1594

PR Checklist

Please check if your PR fulfills the following requirements:

Tested code with current supported SDKs
Docs have been added / updated (for bug fixes / features)
Sample in sample app has been added / updated (for bug fixes / features)
Tests for the changes have been added (for bug fixes / features) (if applicable)

What is the new behavior?

Images will be rendered now if used with Masked Reference
eg: ![Toolkit Logo](/Assets/ToolkitLogo.png) will render the image in MarkdownTextBlock
Email Links will be rendered as Masked Links and when clicked will open default mail app like it does with unmasked email link.
Feature update for Missing support for relative reference link and svg image in MarkdownTextBlock #1594 related to Relative Links added. Note: Relative Links needs to be handled in LinkClicked event in MarkdownTextBlock.
Sample updated with changes, Note about Handling Relative Links.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Consider Line feed and Carriage Return

Fixes #1607 #1594

This reverts commit 7aad125.

This reverts commit 3201148.

avknaidu · 2017-11-20T20:18:30Z

ping @nmetulev

avknaidu · 2017-11-21T09:56:19Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/MarkdownTextBlock/InitialContent.md

+
+>[Relative Link 2](../Photos/Photos.json)
+
+**Note:** Relative Links has to be Manually Handled in `LinkClicked` Event.


@michael-hawker I thought that was for control documentation. This is the initial content that loads when MarkdownTextblock loads

michael-hawker · 2017-11-21T16:35:37Z

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/Parse/Inlines/MarkdownLinkInline.cs

+        /// <returns> <c>true</c> if the URL is valid; <c>false</c> otherwise. </returns>
+        private static bool IsUrlEmail(string url)
+        {
+            if (Regex.IsMatch(url, @"^([\w\.\-]+)@([\w\-]+)((\.(\w){2,})+)$"))


I don't think the RegEx here is sufficient.

If we're on RS3, we should probably add a method on a .NET Standard 2.0 lib that exposes the MailAddress class, but not sure how we could easily check/switch for that in the main code. @nmetulev thoughts?

We can't use .NET standard 2.0 only APIs just yet. But we can use the C# version from here, not sure if that is much different than what you already have?

@nmetulev the regex is same as the one which was used before in RegexTextBox which is now depreciated. See #1192.

Yes, but that wouldn't get an e-mail like blah'-m$&an-1@gmail.com which is valid according to the spec. On the reference page from @nmetulev, the C# one or the full General Email Regex (RFC 53222 Official Standard) both should work in UWP and cover unusual addresses.

We just want to make sure since we're re-adding this sort of thing, that we're grabbing the right source.

@michael-hawker @nmetulev will make change asap.

…rsion

michael-hawker · 2017-11-27T18:33:03Z

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/Parse/Inlines/ImageInline.cs

@@ -117,6 +117,7 @@ internal static Common.InlineParseResult Parse(string markdown, int start, int e
            }

            string url = TextRunInline.ResolveEscapeSequences(markdown, urlStart, pos);
+            url = url.StartsWith("/") ? string.Format("ms-appx://{0}", url) : url;


Should we have a property which lets the developer decide where the base url for relative links should be (and then default to ms-appx://)?

This would let them specify an alternate assets folder (ms-appx://Assets) or an actual url on the web (http://www.mysite.com/Images) as a baseline instead.

@michael-hawker if the URL starts with http:// it renders by default. that is the reason why url checks for / only. IDK how much of a help a separate property will be. Also the LinkClicked events does not apply on Images. It renders if it can or else it wont.

@nmetulev thoughts?

I think @michael-hawker is referring to string.Format("ms-appx://{0}", url), and changing it to somthing like string.Format("{0}://{1}", prefix, url) where prefix can be set by the developer to anything and defaults to ms-appx:/// if not set

Exactly, I was thinking the full prefix string.Format("{0}{1}", prefix, url) where prefix default would be "msappx://"

Thanks for fixing my example :)

Got it. Will add new Property and update commit.

avknaidu · 2017-11-30T17:27:24Z

ping @nmetulev @michael-hawker

avknaidu · 2017-12-12T22:02:48Z

ping @nmetulev @michael-hawker

avknaidu · 2018-01-04T17:57:13Z

@michael-hawker @nmetulev can you guys verify this PR? I would like to use this feature as a Nuget instead of building the code and using the DLL's in my project. Let me know if i missed something.

nmetulev · 2018-01-04T21:50:29Z

Hey @avknaidu, I'll review this soon. Most folks are on a break this time of year, so it might seem a bit slow sometimes :)

nmetulev · 2018-01-17T00:38:22Z

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/MarkdownTextBlock.cs

@@ -1096,6 +1114,7 @@ private void RenderMarkdown()
                // Try to parse the markdown.
                MarkdownDocument markdown = new MarkdownDocument();
                markdown.Parse(Text);
+                Common.ImageLinkPrefix = ImageLinkPrefix;


What's the purpose of having a static ImageLinkPrefix property?

I need this set up the Link/Image prefixes Here so that when links are clicked, they will have full URL. And i thought having a static property in Common will be the easiest way.

My concern is that having a static property means that if I use MarkdownTextBlock in multiple places, all images must be in the same location. Wouldn't it make more sense to make this a dependency property?

@nmetulev This is indeed a dependancy property, See here. Since I want to access this inside parsers, i used it as a static property in Common.cs

Indeed. It just doesn't make sense to also have a static property.

Can you point me on a way to do this?

@nmetulev I see the problem, it's because he has to use the value here which is a helper in a different class.

@WilliamABradley does your refactoring stuff somehow make this easier? Or any suggestions?

It is because Processing of Blocks and Inlines has no context of the MarkdownDocument. That would require some more refactoring.

Instead it should be kept relative at the parser level, and then handled by the Renderer, where you can add it to the RenderContext. (This will be easier with my upcoming changes).

Add ImageLinkPrefix to the RenderContext class, and then in the Constructor of XamlRenderer.

Can the renderer simply add the image prefix before rendering the images? So the parser provides the absolute link (as it was written) and the renderer handles how to render it.

@WilliamABradley I think @nmetulev's idea is better since the rendering requires full URL and we can append it at the renderer level. I will update my PR shortly.

avknaidu · 2018-01-23T21:46:04Z

@nmetulev @michael-hawker please check if there is anything that i need to finish on this PR. I am really looking for using these changes as Nuget packages instead of DLL's.

WilliamABradley

Currently ImageLinkPrefix applies to both Absolute and Relative links, breaking Absolute images. Shouldn't it only affect Relative images? It limits it's usefulness.

WilliamABradley · 2018-01-30T22:57:53Z

ping @avknaidu

avknaidu · 2018-01-30T23:11:21Z

@WilliamABradley I am planning to check if the URL provided can be resolved to image and if not, then see if it resolves after applying prefix. I will try to finish this over the weekend. Let me know if this is not a good way.

WilliamABradley · 2018-01-30T23:13:02Z

You can instead use Uri.TryCreate with Absolute as the Enum set, then if that fails we know it is a Relative Uri.

avknaidu · 2018-02-01T19:06:08Z

ping @WilliamABradley @michael-hawker

WilliamABradley

Looks good

michael-hawker

@nmetulev some thoughts on naming and the E-mail Regex. I can't find the previous discussion we had with the result on it. Must have been in another PR/bug?

michael-hawker · 2018-02-07T19:21:57Z

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/MarkdownTextBlock.cs

@@ -1243,6 +1261,14 @@ public void RegisterNewHyperLink(Hyperlink newHyperlink, string linkUrl)
        /// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
        async Task<ImageSource> IImageResolver.ResolveImageAsync(string url, string tooltip)
        {
+            if (!Uri.TryCreate(url, UriKind.Absolute, out Uri uri))


I'd suggest creating the uri outside of the if block, as if this does succeed then we don't have to recreate the Uri object again to pass to the BitmapImage on line 1281.

Then on line 1268, you can create a Uri object there and reuse the uri instance. Then it just gets passed directly into the BitmapImage constructor.

You should actually be able to just use the uri object outside of this if block, it should still be in scope.

Ah, apparently it does leak into the outer scope, that's a bit odd.

Apparently is works similarly, kind-of, it's a bit odd, but documented here

Yeah, it's by design, to be used in scenarios exactly like this

michael-hawker · 2018-02-07T19:22:10Z

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/MarkdownTextBlock.cs

+        /// <summary>
+        /// Gets or sets the Prefix of Image Link.
+        /// </summary>
+        public string ImageLinkPrefix


Was thinking since these aren't actually links, should this name be ImageUriPrefix? @nmetulev @WilliamABradley thoughts?

That would be a better name.

The reason behind ImageLinkPrefix is because i want to use the same Prefix for actual links also instead of 2 different DP's. While this is still not implemented, I want to implement this in such a way that even a relative link works like a normal link when LinkClicked event is called.

@avknaidu you mean so that the developer doesn't have to add the detection and relative link parsing to their LinkClicked event handler?

In that case, I'd suggest UriPrefix.

@michael-hawker . Done. I will push the commit shortly.

michael-hawker · 2018-02-07T20:01:10Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/MarkdownTextBlock/MarkdownTextBlockPage.xaml.cs

@@ -69,7 +70,14 @@ private void SetInitalText(string text)

        private async void MarkdownText_LinkClicked(object sender, UI.Controls.LinkClickedEventArgs e)
        {
-            await Launcher.LaunchUriAsync(new Uri(e.Link));
+            if (e.Link.StartsWith("../") || e.Link.StartsWith("/"))


Should put the # here as well, maybe we should have a helper method for this?

Where do we put all Helper Methods? I see Common.cs. Should I use that?

Also i see Url's being handled this way in HyperlinkInline.cs. I can change it over there also if Common.cs is the right place.

Well, Common is for Markdown still, and the e-mail regex is in the TextBoxRegEx control as well.

@nmetulev would adding an extensions namespace to Microsoft.Toolkit.Uwp make sense here or use the existing extensions namespace under Microsoft.Toolkit.Uwp.UI?

What's the helper method here?

@nmetulev IsUriRelative to go with the IsEmail below. Thoughts on the namespace choice?

Is it used outside of the controls package? If it's only used here, I'd try to keep it internal.

@nmetulev Issue here is it cannot be internal because the same Link check is Happening on SampleApp Project. Only way we can do this is to move this to Microsoft.Toolkit.UWP.UI so that it can be used in both places.

Ah, i missed that this is in the sample app. Why not use the standard way of checking for a relative URI?

Uri.TryCreate(url, UriKind.Absolute, out Uri result)

Right now for relative URI's we only check if URL's start with /, #, ../. If we check for the above, we are practically allowing anything. I don't think that is a good idea.

Exactly, I like leaving it up to the developer to decide what to check, so I'm good with the current implementation :)

michael-hawker · 2018-02-07T20:04:55Z

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/Parse/Inlines/MarkdownLinkInline.cs

+        /// <returns> <c>true</c> if the URL is valid; <c>false</c> otherwise. </returns>
+        private static bool IsUrlEmail(string url)
+        {
+            return Regex.IsMatch(url, "(?:[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*|\"(?:[\\x01-\\x08\\x0b\\x0c\\x0e-\\x1f\\x21\\x23-\\x5b\\x5d-\\x7f]|\\\\[\\x01-\\x09\\x0b\\x0c\\x0e-\\x7f])*\")@(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?|\\[(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?|[a-z0-9-]*[a-z0-9]:(?:[\\x01-\\x08\\x0b\\x0c\\x0e-\\x1f\\x21-\\x5a\\x53-\\x7f]|\\\\[\\x01-\\x09\\x0b\\x0c\\x0e-\\x7f])+)\\])");


@nmetulev didn't we have a discussion on e-mail regex parsing? I was trying to find the link we found with the reference to this. It'd be nice to put the url reference in a comment here. Also, we use this elsewhere, like the TextBoxRegEx (though that didn't seem to be the one we talked about).

So, it might be nice to have a centralized helper for e-mail validation that can be used in all places?

Found it. Yeah, it'd be nice to have a comment with our reference for the future:
//General Email Regex (RFC 5322 Official Standard) from emailregex.com

But since we also use it in TextBoxRegEx, it might be nice to pull it out into its own helper that can be used in both places. Maybe as a string extension?

@nmetulev thoughts?

Agree, @avknaidu, do you think you can create string extensions and add this method to it to be used with this control and the TextBoxRegEx?

@nmetulev Done. Waiting for an answer on the question above to push the commit.

nmetulev · 2018-02-09T20:43:39Z

Microsoft.Toolkit.Uwp.UI/Extensions/Common/StringExtensions.cs

+
+using System.Text.RegularExpressions;
+
+namespace Microsoft.Toolkit.Uwp.UI.Extensions.Common


Should this be in Microsoft.Toolkit.Uwp package instead, it has nothing to do with UI? Similar to the ColorHelper for example. @michael-hawker thoughts?

michael-hawker · 2018-02-09T20:47:41Z

Microsoft.Toolkit.Uwp.UI/Extensions/Common/StringExtensions.cs

+        /// Uses general Email Regex (RFC 5322 Official Standard) from emailregex.com
+        /// </summary>
+        /// <returns><c>true</c> for valid email.<c>false</c> otherwise</returns>
+        public static bool IsEmail(this string str)


The TextBoxRegEx implementation should also be updated to use this new method now too.

https://github.com/Microsoft/UWPCommunityToolkit/blob/master/Microsoft.Toolkit.Uwp.UI/Extensions/TextBoxRegEx/TextBoxRegex.cs#L91

I can do another PR for moving everything related to TextBoxRegEx to this helper method. For now I will update this PR moving the string constants separate.

michael-hawker · 2018-02-09T20:51:05Z

Microsoft.Toolkit.Uwp.UI/Extensions/Common/StringExtensions.cs

+        /// <returns><c>true</c> for valid email.<c>false</c> otherwise</returns>
+        public static bool IsEmail(this string str)
+        {
+            return Regex.IsMatch(str, "(?:[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*|\"(?:[\\x01-\\x08\\x0b\\x0c\\x0e-\\x1f\\x21\\x23-\\x5b\\x5d-\\x7f]|\\\\[\\x01-\\x09\\x0b\\x0c\\x0e-\\x7f])*\")@(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?|\\[(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?|[a-z0-9-]*[a-z0-9]:(?:[\\x01-\\x08\\x0b\\x0c\\x0e-\\x1f\\x21-\\x5a\\x53-\\x7f]|\\\\[\\x01-\\x09\\x0b\\x0c\\x0e-\\x7f])+)\\])");


To make the TextBoxRegEx change easier, you could pull out the regex pattern string as an internal constant and just have the TextBoxRegEx switch statement reference that.

@nmetulev think it'd actually be useful to pull all the RegEx expressions out of the TextBoxRegEx class and put them here? Then we can have nice helper methods for string for each of the different types?

I can do another PR for moving everything related to TextBoxRegEx to this helper method. For now I will update this PR moving the string constants separate.

Makes sense. You should still move the extension to the Microsoft.Toolkit.Uwp package in this PR

… string.

nmetulev · 2018-02-09T22:23:56Z

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/Parse/Inlines/MarkdownLinkInline.cs

 namespace Microsoft.Toolkit.Uwp.UI.Controls.Markdown.Parse
 {
+    using System;


These using statements should be outside of the namespace block.

nmetulev · 2018-02-10T00:47:24Z

Thanks @avknaidu. Up to @michael-hawker now for his review :)

michael-hawker · 2018-02-10T01:32:40Z

Think my last comment is that StringExtensions.cs should probably live in an Extensions folder rather than Helpers.

Basically keep the folder structure Extensions\Common you had before but in the Microsoft.Toolkit.Uwp project, to mimic what we've done in the Microsoft.Toolkit.Uwp.UI project.

@nmetulev would it make sense to put this in Microsoft.Toolkit as it's not UWP specific?

WilliamABradley · 2018-02-10T02:18:47Z

I vote for Microsoft.Toolkit, since that will be going to Microsoft.Toolkit.Parsers anyway.

avknaidu · 2018-02-10T17:16:22Z

@michael-hawker done.

windowstoolkitbot · 2018-02-10T21:26:24Z

This PR is linked to unclosed issues. Please check if one of these issues should be closed: #1594, #1607

avknaidu added 5 commits October 22, 2017 12:27

#Fixes 1516

3201148

Consider Line feed and Carriage Return

Fixes #1607 #1594

7aad125

Fixes #1607 #1594

Revert "Fixes #1607 #1594"

c9404c1

This reverts commit 7aad125.

Fixes #1594 #1607

7e10f81

Revert "#Fixes 1516"

a98b757

This reverts commit 3201148.

avknaidu commented Nov 21, 2017

View reviewed changes

michael-hawker requested changes Nov 21, 2017

View reviewed changes

Changing Regex to General Email Regex (RFC 5322 Official Standard) Ve…

20d6756

…rsion

michael-hawker reviewed Nov 27, 2017

View reviewed changes

ImageLinkPrefix Implementation

0fbfba7

avknaidu mentioned this pull request Jan 11, 2018

Relative links in sample app documentation panel is not working #1740

Closed

nmetulev reviewed Jan 17, 2018

View reviewed changes

avknaidu changed the title ~~Markdown text block fix~~ Implementing Relative Links/Images/Emails in MarkdownTextBlock Jan 23, 2018

removed unwanted static property

e8a978b

WilliamABradley suggested changes Jan 25, 2018

View reviewed changes

WilliamABradley mentioned this pull request Jan 31, 2018

Markdown + RSS Parser for .NET Standard, MarkdownTextBlock Style Improvements, Syntax Highlighting #1650

Merged

12 tasks

Check for Absolute URI.

e164840

WilliamABradley approved these changes Feb 1, 2018

View reviewed changes

WilliamABradley added this to the v2.2 milestone Feb 1, 2018

WilliamABradley added the controls 🎛️ label Feb 1, 2018

WilliamABradley added the improvements ✨ label Feb 1, 2018

michael-hawker requested changes Feb 7, 2018

View reviewed changes

avknaidu added 3 commits February 9, 2018 15:09

Moved Email Regex check to UI.Extensions

909cc1e

Change property from "ImageLinkPrefix" to "UriPrefix

7948c42

Updated Masked Relative Links, cleaned Initial Content

1c6809f

nmetulev reviewed Feb 9, 2018

View reviewed changes

michael-hawker requested changes Feb 9, 2018

View reviewed changes

Moved Extension to Microsoft.Toolkit.Uwp, Extracted Regex as separate…

c87de47

… string.

nmetulev reviewed Feb 9, 2018

View reviewed changes

Cleanup Usings

457d0fb

nmetulev approved these changes Feb 10, 2018

View reviewed changes

Moving String Extensions to Micorosoft.Toolkit as suggesred

d5441a8

michael-hawker approved these changes Feb 10, 2018

View reviewed changes

michael-hawker merged commit b63ee18 into CommunityToolkit:master Feb 10, 2018

avknaidu deleted the MarkdownTextBlockFix branch February 10, 2018 23:48

avknaidu mentioned this pull request Feb 12, 2018

TextboxRegex Update with String Extensions #1800

Closed


		>[Relative Link 2](../Photos/Photos.json)

		Note: Relative Links has to be Manually Handled in `LinkClicked` Event.


		using System.Text.RegularExpressions;

		namespace Microsoft.Toolkit.Uwp.UI.Extensions.Common

Implementing Relative Links/Images/Emails in MarkdownTextBlock #1639

Implementing Relative Links/Images/Emails in MarkdownTextBlock #1639

Conversation

avknaidu commented Nov 20, 2017

PR Type

What is the current behavior?

PR Checklist

What is the new behavior?

Does this PR introduce a breaking change?

Other information

avknaidu commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avknaidu commented Nov 30, 2017

avknaidu commented Dec 12, 2017

avknaidu commented Jan 4, 2018

nmetulev commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avknaidu Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamABradley Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avknaidu commented Jan 23, 2018

WilliamABradley left a comment

Choose a reason for hiding this comment

WilliamABradley commented Jan 30, 2018

avknaidu commented Jan 30, 2018

WilliamABradley commented Jan 30, 2018

avknaidu commented Feb 1, 2018

WilliamABradley left a comment

Choose a reason for hiding this comment

michael-hawker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avknaidu Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avknaidu Jan 24, 2018 •

edited

Loading

WilliamABradley Jan 24, 2018 •

edited

Loading

avknaidu Feb 9, 2018 •

edited

Loading