Task Dialog: Improve hyperlink usage

[kpreisser]

:arrow_double_down::arrow_double_down: CLICK HERE TO SCROLL DOWN TO THE FINAL PROPOSAL :arrow_double_down::arrow_double_down:

---

Hi,

as requested by @RussKie in https://github.com/dotnet/winforms/issues/146#issuecomment-502493259, I've opened a new issue for this topic. Note: The listed options don't need to change the currently proposed API in an incompatible way, so this issue can be addressed at a later time.

The native Windows Task Dialog allows to use hyperlinks in certain text elements (text/content, footer, expander) by setting the TDF_ENABLE_HYPERLINKS flag. This allows you to specify a link with the form <a href="mylink">text</a> (similar to HTML). When the user clicks on the link, the TDN_HYPERLINK_CLICKED is sent to the callback with the content of the href attribute as parameter.

This is reflected in the currently proposed API in #146: To use links, you set the TaskDialogPage.HyperlinksEnabled property to true, and then you can set e.g.

page.Text = "This text contains <a href=\"link1\">a link</a>.";

to show a link in the task dialog text:

Then, you have to handle the TaskDialogPage.HyperlinkClicked event to get notified when the user clicks a link. That event will provide you the text of the href attribute, in that case link1.

However, there are some problems with this approach (as is simply a mapping of the native API):

• Users need to know the exact syntax to specify a link (which is similar to HTML), and in source code you need to escape the double quotes.
• There is a risk that when you enable hyperlinks and then show strings from external sources, they might get misinterpreted as hyperlink even if you actually just want it to show as plain text. As noted in the documentation for TDF_ENABLE_HYPERLINKS, this could cause security vulnerabilities, for example if you did something like Process.Start(e.Hyperlink) in the HyperlinkClicked event. (A safer way is not to use links like https://... directly in the href, but instead use keys like linkA, and then in the event, compare the key to open the corresponding link.)
• In order to prevent strings from external sources to be interpreted as links (or when you actually want to display a string like <a href="..."></a> as part of the text directly without interpreting as hyperlink), you need to replace "<a" e.g. with "<\u200Ba" (zero width space), so that the task dialog interprets < as literal character. A minor downside of this is that if you then copy the dialog's text contents with Ctrl+C, that invisible character is also copied. (I didn't find another way for this, as the HTML/XML-like < doesn't seem to work; and while replacing "<a" with "<<a></a>a" (which I thought previously) would fix the copy problem with the invisible space, it cannot be used as it creates empty hyperlinks that are reachable by pressing Tab ↹.)
• When you enable HyperlinksEnabled and there is at least one link (<a ...>) present in the text, an & character will be interpreted to specify a mnemonic char, instead of displaying it directly (which is the behavior when HyperlinksEnabled is not set), although then actually nothing happens when you press Alt and the character. Therefore, you would generally need to replace & with && in the strings when HyperlinksEnabled is set and you have at least one link in the text. (But if you then copy the task dialog's text contents with Ctrl+C, it will copy the escaped form (&&) instead of the displayed form (&).)

Option A

As suggested by @GSPP: Don't change the existing API, but add new methods that can be used to retrieve strings with the correct syntax when EnableHyperlinks is set, for example::

    public static string GetHyperlinkText(string href, string text)
    {
        // Don't allow "</a>" in the link text because we cannot escape that.
        if (text.Contains("</a>", StringComparison.OrdinalIgnoreCase))
            throw new ArgumentException();
        if (href.Contains("\"", StringComparison.OrdinalIgnoreCase))
            throw new ArgumentException();

        return $"<a href=\"{href}\">{text.Replace("&", "&&")}</a>";
    }

    public static string GetNonHyperlinkText(string text)
    {
        return text.Replace("&", "&&").Replace("<a", "<\u200Ba").Replace("<A", "<\u200BA");
    }

Then, you could set the text like this:

    page.Text = TaskDialog.GetHyperlinkText("myLink", "Hello World & others!") +
        TaskDialog.GetNonHyperlinkText(" External text: <a href=\"test\">&</a>");

With the helper methods, you wouldn't need to specify the exact syntax for the link and you wouldn't need to escape special characters like < by yourself.

Option B

As suggested by @jnm2: Also don't change the existing API but add a new method

    public static string FormatWithHyperlinks(FormattableString str)

on the TaskDialog which you then could call with an interpolated string, that takes the href as argument and the link text as format string (or the other way round):

    page.Text = TaskDialog.FormatWithHyperlinks(
        $"Some text, then {"https://foo/":a link} & {"https://bar/":another link}; <a href=\"x&y\">no link</a>");

    // Or:
    page.Text = TaskDialog.FormatWithHyperlinks(
        $"Some text, then {"a link":https://foo/} & {"another link":https://bar/}; <a href=\"x&y\">no link</a>");

(However, with this option there will probably need to be another method that allows to escape strings (like GetNonHyperlinkText() in Option A) if you then want to embed other strings which shouldn't get interpreted as link.)

Thanks!

[jnm2]

One thing to be aware of with FormattableString. If you manually create a variable and don't explicitly type it as FormattableString, C# prefers the type string which means the templated string is rendered to string before getting passed.

var url1 = "https://foo";
var url2 = "https://bar";
var text = $"Some text, then {url1:a link}, then {url2:another link}." 
// text now holds merely the string "Some text, then t, then g."

This is okay so long as there is no overload of FormatWithHyperlinks that accepts a string. The compiler will draw your attention to it by erroring:

// CS1503 Argument 1: cannot convert from 'string' to 'System.FormattableString'
TaskDialog.FormatWithHyperlinks(text);

[jnm2]

It doesn't appear to be possible to manually create and handle hyperlinks. This is something we'll need before we can move away from our custom implementation. Is this issue tracking the ability to respond to link clicks, or should I open a new one?

[RussKie]

We have completely descoped hyperlink-related implementations due to an observed confusion during our user studies with the proposed implementation. Whilst the API followed the Win32 implementation, they felt somewhat foreign to Windows Forms users.

To add these API we need to design a new set of API that feel "more native" to Windows Forms.

[jnm2]

@RussKie Is there enough information on it that the community can help with that, or are we blocked waiting for an internal team?

[RussKie]

This piece of work isn't high on our priority list, so a community contribution is quite welcome. I'm happy to work with the community and help to see it through.

[jnm2]

That's great! Are you looking for API ideas, or do already have some concept of the shape it should be? If the former, do you have notes on the pitfalls that were discovered that should be avoided while looking for ideas?

[RussKie]

I've looked through the notes, and appears @kpreisser has captured them very well in the OP (all that before Option A and B).

I haven't had a chance to spend any meaningful time pondering on API (busy getting .NET 5.0 ready), but I don't think either of the proposed options will meet the bar in their current shape. The TD API has undergone a number revisions from the original proposal, and we'll need to come up with an API that will fit in with the final design, ideally with a minimal learning curve for a general Windows Forms developer.

[jnm2]

@RussKie How about an API roughly like this?

// No need to specify href since it's basically just a note to yourself in the form of a string tag.
// Probably simpler overall to not even expose href as a concept.

var firstLink = new TaskDialogHyperlink("a link");
firstLink.Click += FirstLink_Click;

var secondLink = new TaskDialogHyperlink("another link") { Tag = "some info" };
secondLink.Click += HandlerThatMakesUseOfTheTag;

var page = new TaskDialogPage
{
    TextContent =
    {
        "Plain text (automatically escaped for you if necessary) followed by ", firstLink, ", and then ", secondLink, "."
    }
};

// Or
page.TextContent.Add("Plain text (automatically escaped for you if necessary) followed by ");
page.TextContent.Add(firstLink);
page.TextContent.Add(", and then ");
page.TextContent.Add(secondLink);
page.TextContent.Add(".");

// Text is shorthand for the common case of wanting to set the entire text contents to a single string.
page.Text = "This is a shorthand for TextContent.Clear() followed by TextContent.Add(string).";

It's less winformsy, but maybe to really drive home the point that you're on the hook to handle the click (no automatic link handling), the TaskDialogHyperlink constructor requires both string displayText and Action clickHandler. On the other hand, this benefit could still be had with the Click event if there is an in-the-box analyzer that produces a warning if you create a TaskDialogHyperlink without handling the Click event.

I'd be happy to write up a full API proposal if we start narrowing in on something you like.

[nikeee]

I think it's less practical if one has to translate the text of the dialog into multiple languages. The order of the items might change or some could even get split up.

[jnm2]

@nikeee What would work best for translations, having format strings with required tags?

Plain text (automatically escaped for you if necessary) followed by {0:a link}, and then {1:another link}. Sadly I'm not bilingual, but {1:link B} comes before {0:link A} in this sentence.

[jnm2]

New proposal because I agree that breaking into segments makes localization painful. This localizes the link display text in the same localization unit as the whole message:

var firstLink = new TaskDialogHyperlink();
firstLink.Click += FirstLink_Click;

var secondLink = new TaskDialogHyperlink() { Tag = "some tag" };
secondLink.Click += HandlerThatMakesUseOfTheTag;

var page = new TaskDialogPage
{
    HyperlinkText = new TaskDialogHyperlinkText(
        Resources.SomeMessage, // "Plain auto-escaped text with {0:link A}, an injected value ({1}), and {2:link B}."
        firstLink,
        42,
        secondLink)
};

TaskDialogHyperlinkText would have the signature string format, params object[] args with XML docs explaining how to localize links. The constructor would throw ArgumentException if the format string does not have a formatting component each time a TaskDialogHyperlink argument is used. (Should probably also throw if a TaskDialogHyperlink is used with an alignment component.)

If we go this direction, it would be nice to have a new TaskDialogHyperlink(EventHandler clickHandler) convenience overload:

var page = new TaskDialogPage
{
    HyperlinkText = new TaskDialogHyperlinkText(
        Resources.SomeMessage, // "Plain auto-escaped text with {0:link A}, an injected value ({1}), and {2:link B}."
        new TaskDialogHyperlink((sender, e) => DoSomething()),
        42,
        new TaskDialogHyperlink(HandlerThatMakesUseOfTheTag) { Tag = "some tag" })
};

[jnm2]

However, there will probably people who want to build the text programmatically by segments (I've done this recently), and they shouldn't have to create a format string in order to do so. Perhaps it would be better to blend these two. We could take the first one and add a TextContent.Add(string format, params object[] args) overload.

[RussKie]

Thank you! Keep the ideas coming. I'll set some time aside this week to think about this too.

@kpreisser would love to hear you thoughts too, as well as a feasibility assessment from the API perspective.

[RussKie]

I'm flat out busy with prep'ing the 5.0 release, and I don't have any capacity for this until we ship. Sorry for the delay.

[RussKie]

Apologies for taking more time than I promised. Thank you to everyone who took their time to share their thoughts. Here are my thoughts.

I don't think the Option A would work well because it puts a lot of responsibility on a developer, which in turn makes it rather error-prone. Nonetheless, @GSPP thank you for your proposal.

The Option B has more potential, though we need to iterate on it more.

@jnm2 I like the idea of TaskDialogHyperlink, and agree with your assessment of discoverability and clarity of intent:

var secondLink = new TaskDialogHyperlink("link text") { Tag = "some info" };
secondLink.Click += HandlerThatMakesUseOfTheTag;

It's less winformsy, but maybe to really drive home the point that you're on the hook to handle the click (no automatic link handling), the TaskDialogHyperlink constructor requires both string displayText and Action clickHandler. On the other hand, this benefit could still be had with the Click event if there is an in-the-box analyzer that produces a warning if you create a TaskDialogHyperlink without handling the Click event.

This API should also fit in nicely along the existing TaskDialog API.

I do agree with @nikeee that breaking up a string makes localisation-related work harder, so we may want to avoid this. At the same time this API are is not dissimilar to the StringBuilder API :).

I like the proposal to use text with placeholders (e.g. Plain text (automatically escaped for you if necessary) followed by {0}, and then {1}.). And if TaskDialogHyperlink can be converted to a string, it would work similar to this:

TaskDialogButton result = TaskDialog.ShowDialog(this, new TaskDialogPage()
{
    EnableHyperlinks = true,
    Text = string.Format(fmt, link1, link2),
}

As @kpreisser suggested earlier, handling a click event we'd pass the link ID (or whatever we end up calling it) - being "link1" and "link2" from the sample, so a user can handle it explicitly. This way we'd address use cases where a link is a hyperlink, or a custom in-app link (e.g. click a link to open another dialog in the app).

What do you think?

[jnm2]

@RussKie If TaskDialogHyperlink has a Click event or e.g. Action onClick constructor parameter, then the TaskDialogHyperlink instance itself will have to be added to the task dialog page somehow and not just the text. Then EnableHyperlinks would not need to be a property because it could be determined by the presence of a TaskDialogHyperlink in the page.

If TaskDialogHyperlink doesn't have a Click event, then I like LinkId and LinkText, and I guess there would be a HyperlinkClick event on the page/dialog, and then there would be no way to avoid the EnableHyperlinks property since links are created by setting Text rather than by adding TaskDialogHyperlink instances to the page.

My preference is for the former, where string.Format is not used for localization and instead something like TextContent.Add(string format, params object[] args) is used for localization. That allows TaskDialogHyperlink instances to be added to the page and have Click events or optional Action onClick-style constructor parameters. The former also allows code to programmatically examine the text content and reason about hyperlinks easily without having to parse <a syntax.

[jnm2]

@RussKie Also note that LinkText will almost always need to be localized when any of the text is localized. This suggestion gives a way for you to localize the entire text content at once, if that's desirable: https://github.com/dotnet/winforms/issues/1199#issuecomment-691716098

[RussKie]

We had a brainstorm session with @KlausLoeffelmann and @dreddy-work, exploring various scenarios and developer experience.

I've looked at the original @kpreisser's proposal (through the ILSpy in the old build I happen to have lying around), and the hyperlink click event was implemented like this (a snip from a larger piece):

private int HandleTaskDialogCallback(IntPtr hWnd, Interop.TaskDialog.TASKDIALOG_NOTIFICATIONS notification, IntPtr wParam, IntPtr lParam)
{
    switch (notification)
    {
		case Interop.TaskDialog.TASKDIALOG_NOTIFICATIONS.TDN_HYPERLINK_CLICKED:
		{
			string link = Marshal.PtrToStringUni(lParam);
			TaskDialogHyperlinkClickedEventArgs eventArgs = new TaskDialogHyperlinkClickedEventArgs(link);
			_boundPage.OnHyperlinkClicked(eventArgs);
			break;
		}
    }
}

From Win32 we're only getting the hyperlink of the link clicked (or "executablestring" as the docs call it), and whilst it feels logical to place Click event at a TaskDialogHyperlink type, in reality it is handled at the TaskDialogDialog level. Of course with some plumbing we can maintain a collection of hyperlinks objects, and then raise an event at the original instance, but I fail to see a significant value that justifies all that added complexity. (We actually went down this road in our discussions, failing to check the Win32 API.)

With that the proposed TaskDialogHyperlink type is really a convince type, and strictly speaking it is not necessary for the hyperlinks functionality, but it makes it easier for users to render hyperlink markup:

public class TaskDialogHyperlink
{
    public TaskDialogHyperlink(string text, string hyperlink) { ... }   
    public string Text { get; }
    public string Hyperlink { get; }
    public override string ToString() => /* render <A HREF="Hyperlink">Text</A> with necessary escaping */
}

The rest is plumbing for the most part:

public class TaskDialogHyperlinkClickedEventArgs : EventArgs
{
	internal TaskDialogHyperlinkClickedEventArgs(string hyperlink)
	{
		Hyperlink = hyperlink;
	}

	/// <summary>
	/// The value of the <c>href</c> attribute of the hyperlink that the user clicked.
	/// </summary>
	/// <remarks>
	/// Please note: In order to avoid possible security vulnerabilities when showing content
	/// from unsafe sources in a task dialog, you should always verify the value of this
	/// property before actually opening the link.
	/// </remarks>
	public string Hyperlink { get; }
}

public class TaskDialogPage
{
    protected internal void OnHyperlinkClicked(TaskDialogHyperlinkClickedEventArgs e)
    {
        HyperlinkClicked?.Invoke(this, e);
    }

    internal void Bind(TaskDialog owner, out Interop.TaskDialog.TASKDIALOG_FLAGS flags, ...)
    {
        // ...

        // Set EnableHyperlinks based on whether there is a wired handler
        EventHandler hyperlinkClicked = HyperlinkClicked;
        SetFlag(Interop.TaskDialog.TASKDIALOG_FLAGS.TDF_ENABLE_HYPERLINKS, hyperlinkClicked  is null);

        // ...
    }
}

public class TaskDialogDialog
{
    private int HandleTaskDialogCallback(IntPtr hWnd, Interop.TaskDialog.TASKDIALOG_NOTIFICATIONS notification, IntPtr wParam, IntPtr lParam)
    {
        switch (notification)
        {
            case Interop.TaskDialog.TASKDIALOG_NOTIFICATIONS.TDN_HYPERLINK_CLICKED:
            {
                string hyperlink = Marshal.PtrToStringUni(lParam);
                TaskDialogHyperlinkClickedEventArgs eventArgs = new TaskDialogHyperlinkClickedEventArgs(hyperlink);
                _boundPage.OnHyperlinkClicked(eventArgs);
                break;
            }
        }
    }
}

If TaskDialogHyperlink doesn't have a Click event, then I like LinkId and LinkText, and I guess there would be a HyperlinkClick event on the page/dialog, and then there would be no way to avoid the EnableHyperlinks property since links are created by setting Text rather than by adding TaskDialogHyperlink instances to the page.

I don't think this is a problem. As shown above, we can infer and set the state of EnableHyperlinks based on whether TaskDialogPage.HyperlinkClicked event is wired or not.

My preference is for the former, where string.Format is not used for localization and instead something like TextContent.Add(string format, params object[] args) is used for localization.

I'm not sure I understand the issue here. I don't believe it is uncommon to have a localisable content with placeholders, e.g. "This sting will show {0} text". Each instance of hyperlink can have their own localisable content used for Text property, and then at runtime all of these localised resources come together via string.Format or string interpolation. This approached is used in the Windows Forms SDK, and pretty much the same is employed in Git Extensions project.

[GSPP]

public override string ToString() => /* render <A HREF="Hyperlink">Text</A> with necessary escaping */

Is it really a good idea to make the ToString output HTML? I'd make it a nice debug representation that is useful in the debugger. Then, there should be a separate method GetHTML (or no such public functionality).

[RussKie]

Then, there should be a separate method GetHTML (or no such public functionality).

Without it we lose string interpolation abilities, e.g. this won't be possible page.Text = $"Click my {link1}";, and we'll have something like page.Text = $"Click my {link1.AsHtml()}";

[jnm2]

As shown above, we can infer and set the state of EnableHyperlinks based on whether TaskDialogPage.HyperlinkClicked event is wired or not.

This worries me. I might want to have a helper method that always wires up HyperlinkClicked without knowing whether the text was intended to be interpreted as having hyperlinks. There is not a strong enough dichotomy between regular text that just happens to fall into the pattern of a link (maybe it's quoting user input!) and an actual intended hyperlink.

Of course with some plumbing we can maintain a collection of hyperlinks objects, and then raise an event at the original instance, but I fail to see a significant value that justifies all that added complexity.

The value I see is that there would be an object model at the right conceptual level with full safety and intentionality, rather than a dual state constructed-text-as-data-model Text property.

[jnm2]

Can TaskDialogHyperlink.Hyperlink be renamed Id or Tag to show that there is no interpretation going on that requires the href ID to be a URL or link in any sense? I'm not sure it's self-explanatory that any arbitrary string value can be used here that helps identify which link was clicked.

[KlausLoeffelmann]

This worries me. I might want to have a helper method that always wires up HyperlinkClicked without knowing whether the text was intended to be interpreted as having hyperlinks. There is not a strong enough dichotomy between regular text that just happens to fall into the pattern of a link (maybe it's quoting user input!) and an actual intended hyperlink.

Wouldn't that be a problem in the native Win32 API already? Or do you need to enable Hyperlinks dedicatedly in the Win32 API, also? (I did not read this thread completely, so excuse me if I missed that.)

[jnm2]

@KlausLoeffelmann You do need to specifically enable hyperlinks in the Win32 API using TDF_ENABLE_HYPERLINKS. I would hope that we would not see it as a particular goal to have the object model resemble the Win32 API though. The way the Win32 API requires hyperlinks to be encoded into the text, and the dual meanings of the same text field based on the TDF_ENABLE_HYPERLINKS, are things I was hoping the Windows Forms object model would not replicate.

[KlausLoeffelmann]

So, you mean, only if there was a definition of HyperLinks (as objects), you would implicitly have the TDF_ENABLE_HYPERLINKS flag set?

[KlausLoeffelmann]

Let me just throw out a piece of code (based on a stage we had it yesterday in the middle of a discussion). Not as a statement of intent to do it that way - just to see if the thought goes that way. At least not yet. 😄

namespace TaskDialogApiDiscussion
{
    public class TaskDialogHyperlink
    {
        public TaskDialogHyperlink(string text, string identifier)
        {
            Text = text;
            Identifier = identifier;

            // Not really sure, if this is necessary.
            //HyperlinkId = Guid.NewGuid();
        }

        public string Text { get; }
        public string Identifier { get; }

        public override string ToString()
        {
            return Identifier;
        }
    }

    public static class TaskDialogExtension
    {
        public static TaskDialogHyperlink CreateHyperlink(this TaskDialogPage page, string text, string url)
        {
            var hyperlink = new TaskDialogHyperlink(text, url);
            // page.Hyperlinks.Add(hyperlink);
            return hyperlink;
        }
    }

    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
        }

        private void button1_Click(object sender, EventArgs e)
        {
            var page = new TaskDialogPage();

            // Hyperlink doesn't have a public constructor, so we'd always ensure we know
            // the hyperlinks because they will be bound to the page.
            TaskDialogHyperlink hyperlink = page.CreateHyperlink(text: "Hyperlinktext", url: "URL");
            hyperlink.Click += SomeMethod;

            // {hyperlink} NOW enables TDF_ENABLE_HYPERLINKS, because we can match the hyperlink (object).
            page.Text = $"This is my {hyperlink}";
            var taskDialog = TaskDialog.ShowDialog(page);
        }
    }
}

[RussKie]

Of course with some plumbing we can maintain a collection of hyperlinks objects, and then raise an event at the original instance, but I fail to see a significant value that justifies all that added complexity.

The value I see is that there would be an object model at the right conceptual level with full safety and intentionality, rather than a dual state constructed-text-as-data-model Text property.

This would be at the expense of a) memory consumption, b) more CPU cycles, and c) code maintainability. All of which IMO are costly.

From architectural perspective, there's a single channel to receive notifications for all hyperlinks on the page, that get clicked. We have to maintain a collection of hyperlinks, and an associated collection of handler instances, and route a click notification to the hyperlink instance that it was wired to. A lot of moving parts for an arguable convenience.

Can TaskDialogHyperlink.Hyperlink be renamed Id or Tag

Sure, this isn't set in stone yet, and can be discussed. I've chosen "hyperlink" though "url" is probably more appropriate here. The docs refer to it as executablestring.

[RussKie]

This worries me. I might want to have a helper method that always wires up HyperlinkClicked without knowing whether the text was intended to be interpreted as having hyperlinks. There is not a strong enough dichotomy between regular text that just happens to fall into the pattern of a link (maybe it's quoting user input!) and an actual intended hyperlink.

In this case: page.EnableHyperlinks = true :)

[KlausLoeffelmann]

This would be at the expense of a) memory consumption, b) more CPU cycles, and c) code maintainability. All of which IMO are costly.

Is this relevant here? I mean, it's not that a TaskDialog has 10,000 pages or is called 100,000 times per second.

I simply find the solution we came up with yesterday still more elegant, easy to read and intuitive.

[jnm2]

This would be at the expense of a) memory consumption, b) more CPU cycles, and c) code maintainability. All of which IMO are costly.

I agree with Klaus that the CPU cycles and memory consumption seem unlikely to be an issue.

From architectural perspective, there's a single channel to receive notifications for all hyperlinks on the page, that get clicked. We have to maintain a collection of hyperlinks, and an associated collection of handler instances, and route a click notification to the hyperlink instance that it was wired to. A lot of moving parts for an arguable convenience.

A similar argument could be made about the architecture of WndProc. Windows Forms has not previously had it as a goal to expose the underlying Win32 API architecture. To me, it would even be desirable to avoid it.