Add event handlers as tag helpers #498

rynowak · 2018-04-06T02:58:02Z

This change adds support for mapping DOM event handlers as tag helpers
that function in a bi-modal way.

This is a new first-class feature for DOM events, and replaces a few
workarounds like using @onclick(...) or click=@{ ... }. I haven't
removed those things yet, this is a first pass to get the new support
in, we'll remove those things when we're totally satisfied.

When used with a string like <button onclick="foo" /> the result is
a simple HTML attribute .

But when used with an implicit expression like
<button onclick="@Foo" /> or
<button onclick="@(x => Clicked = true)" /> a C# function is bound to
the click event from the DOM.

rynowak · 2018-04-06T03:00:11Z

src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BindLoweringPass.cs

+            expression.Children.Add(new IntermediateToken()
+            {
+                Content = ")",
+                Kind = TokenKind.CSharp,


I had to change how this worked because I realized that we broke completion.

What would happen is that you'd get black text inside the attribute, no colorization, no completion. This is a more involved approach that I was hoping to avoid (and has knock on effects elsewhere, but it's necessary).

The root cause is that if you take C# code that the user wrote, and don't map it to a location in the output document, then they won't get C# editor features for that location.

I have an idea how we could write tests for this and I'm going to try it out in Razor.

rynowak · 2018-04-06T03:02:39Z

src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BindLoweringPass.cs

@@ -230,20 +240,19 @@ private void RewriteUsage(TagHelperIntermediateNode node, int index, ComponentAt
            // BindMethods.SetValueHandler(__value => <code> = __value, <code>) OR
            // BindMethods.SetValueHandler(__value => <code> = __value, <code>, <format>)
            //
-            // For now, the way this is done isn't debuggable. But since the expression
-            // passed here must be an LValue, it's probably not important.
+            // Note that the linemappings here are applied to the value attribute, not the change attribute.


We might get feedback on this and it might be worth rethinking. Currently the change handler will be written out without any line mappings, which means that it's not debuggable. I don't think it's a big concern for bind since you can't really write a lambda here.

OK. We'll have to see how it works in practice when we have debugging. I'm sure it's fine for now.

rynowak · 2018-04-06T03:08:18Z

src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorRuntimeNodeWriter.cs

        private IDictionary<string, PendingAttribute> _currentElementAttributes = new Dictionary<string, PendingAttribute>();
        private IList<PendingAttributeToken> _currentElementAttributeTokens = new List<PendingAttributeToken>();
        private int _sourceSequence = 0;

        private struct PendingAttribute
        {
-            public object AttributeValue;
+            public List<IntermediateToken> Values { get; set; }


OK so what I did here was just treat everything as tokens since we already have that information. This makes the tracking of what's what very simple because it's all tokens.

Sounds good

rynowak · 2018-04-06T03:11:14Z

src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorRuntimeNodeWriter.cs

+        // Only the mixed case is complicated, we want to turn it into code that will concatenate
+        // the values into a string at runtime.
+
+        private static void WriteAttributeValue(CodeWriter writer, IList<IntermediateToken> tokens)


I hadn't realized that you had support for these cases 😆

I had to revisit this code because it wasn't handling a few cases correctly, most notably when you have consecutive CSharp tokens, which happens now for bind and event handlers.

rynowak · 2018-04-06T03:12:10Z

src/Microsoft.AspNetCore.Blazor.Razor.Extensions/ComponentLoweringPass.cs

-            }
-
-            return false;
-        }


All of the removals here are dead code. I wrote another pass earlier that sanitizes things, and I forgot to remove it from here.

rynowak · 2018-04-06T03:15:24Z

src/Microsoft.AspNetCore.Blazor/Components/BindMethods.cs

+        public static UIEventHandler GetEventHandlerValue<T>(Action<T> value)
+            where T : UIEventArgs
+        {
+            return e => value((T)e);


Do you have concerns about perf with this?

If I understand correctly, this is going to create a new delegate each time, which means that any usage of onclick with an event handler will need to edit the dom, and will register/unregister and event handler.

I think we could avoid this if we could avoid the need to create a UIEventHandler instance that wraps Action<UIMouseEventArgs>.

Do you see any good alternatives?

Well, I am concerned that we want to minimise both the GC churn and register/unregister costs of event handlers, however I can't guess whether (a) what you're doing here definitely makes it worse vs what we had before, or (b) whether there's a simple good alternative.

Perhaps we can go ahead with what you have here right now, and then when it's in I'd like to have a look in ildasm at what's happening as a result. If it turns out that we can avoid allocations either by caching the delegates in some way or by not having the UIEventHandler type at all, that would be a good subsequent enhancement.

After offline discussion we're going to try a slightly different approach with this. Will follow up in a separate PR.

rynowak · 2018-04-06T03:16:22Z

src/Microsoft.AspNetCore.Blazor/Components/EventHandlers.cs

+namespace Microsoft.AspNetCore.Blazor.Components
+{
+    [EventHandler("onchange", typeof(UIChangeEventArgs))]
+    [EventHandler("onclick", typeof(UIMouseEventArgs))]


For now I just coded up the thing you had support for. Do you have a good idea of what events we should put in the box or do you think I should try to come up with that list.

Are the ones here the only ones that will compile correctly, or for unrecognized event names, do we fall back on some default type? It would be ideal if we did have a default type (that supplies no info other than that the event happened) for arbitrary event names. I know there's also work on the JS side to make arbitrary events work but that's separate from the compile-time work.

For the actual list of event names, we could use this list as inspiration: https://github.com/Microsoft/TypeScript/blob/master/lib/lib.dom.d.ts#L3475, which also gives hints about the possible event arg types. Assuming we have support for default fallback for unknown events, we could probably reduce our set of explicitly-typed ones down to the most common ones:

"activate": UIEvent; "beforeactivate": UIEvent; "beforedeactivate": UIEvent; "blur": FocusEvent; "change": Event; "click": MouseEvent; "contextmenu": PointerEvent; "dblclick": MouseEvent; "deactivate": UIEvent; "drag": DragEvent; "dragend": DragEvent; "dragenter": DragEvent; "dragleave": DragEvent; "dragover": DragEvent; "dragstart": DragEvent; "drop": DragEvent; "focus": FocusEvent; "input": Event; "invalid": Event; "keydown": KeyboardEvent; "keypress": KeyboardEvent; "keyup": KeyboardEvent; "mousedown": MouseEvent; "mousemove": MouseEvent; "mouseout": MouseEvent; "mouseover": MouseEvent; "mouseup": MouseEvent; "mousewheel": WheelEvent; "pause": Event; "play": Event; "playing": Event; "progress": ProgressEvent; "readystatechange": Event; "scroll": UIEvent; "seeked": Event; "seeking": Event; "select": UIEvent; "selectionchange": Event; "selectstart": Event; "stop": Event; "submit": Event; "touchcancel": TouchEvent; "touchend": TouchEvent; "touchmove": TouchEvent; "touchstart": TouchEvent; "volumechange": Event;

I know you might not want to go declaring new UIEventArgs subtypes for all these (and dozens of E2E tests) during this PR, so I think it would be fine to regard "detailed support for loads of DOM event types" as a separate work item.

rynowak · 2018-04-06T03:19:09Z

test/testapps/BasicTestApp/PropertiesChangedHandlerParent.cshtml

@@ -1,6 +1,6 @@
 @addTagHelper "*, BasicTestApp"
 <PropertiesChangedHandlerChild SuppliedValue=@valueToSupply />
-<button onclick=@{ valueToSupply++; }>Increment</button>
+<button onclick=@(x => valueToSupply++)>Increment</button>


I also wasn't aware that you had added this. Do you think there's still a need for this or were you planning to get rid of it as part of this change?

I'd be totally happy to drop support for the onevent=@{ statement; } syntax. It's a throwback to the very earliest Blazor prototypes, and no longer appears to be a preferable syntax in any scenario. It's always better to use the lambda syntax because without it, there's no way to receive event args.

SteveSandersonMS

Looks fantastic!

TBH I'm not claiming to follow the reasoning behind every change in BlazorRuntimeNodeWriter.cs (without mentally stepping through every chain of execution) but it all looks sensible.

It will be great to get this in for 0.2.0.

SteveSandersonMS · 2018-04-06T10:06:35Z

src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BindLoweringPass.cs

 using Microsoft.AspNetCore.Razor.Language.Intermediate;

 namespace Microsoft.AspNetCore.Blazor.Razor
 {
    internal class BindLoweringPass : IntermediateNodePassBase, IRazorOptimizationPass
    {
+        // Run after event handler pass
+        public override int Order => base.Order + 50;


No need to change this now, but longer term I wonder if it would be helpful to move all these ordering values out into a central location somewhere. Then we'd be able to visualize all the steps and the order they run in.

Or another idea, we could just list them in order where they are registered

SteveSandersonMS · 2018-04-06T10:08:05Z

src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BindLoweringPass.cs

@@ -230,20 +240,19 @@ private void RewriteUsage(TagHelperIntermediateNode node, int index, ComponentAt
            // BindMethods.SetValueHandler(__value => <code> = __value, <code>) OR
            // BindMethods.SetValueHandler(__value => <code> = __value, <code>, <format>)
            //
-            // For now, the way this is done isn't debuggable. But since the expression
-            // passed here must be an LValue, it's probably not important.
+            // Note that the linemappings here are applied to the value attribute, not the change attribute.


OK. We'll have to see how it works in practice when we have debugging. I'm sure it's fine for now.

SteveSandersonMS · 2018-04-06T10:09:36Z

src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorRuntimeNodeWriter.cs

        private IDictionary<string, PendingAttribute> _currentElementAttributes = new Dictionary<string, PendingAttribute>();
        private IList<PendingAttributeToken> _currentElementAttributeTokens = new List<PendingAttributeToken>();
        private int _sourceSequence = 0;

        private struct PendingAttribute
        {
-            public object AttributeValue;
+            public List<IntermediateToken> Values { get; set; }


Sounds good

SteveSandersonMS · 2018-04-06T10:11:20Z

src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorRuntimeNodeWriter.cs

+                // If it's a C# expression, we have to wrap it in parens, otherwise things like ternary 
+                // expressions don't compose with concatenation. However, this is a little complicated
+                // because C# tokens themselves aren't guaranteed to be distinct expressions. We want
+                // to treat all contiguous C# tokens as a sincle expression.


Typo: sincle

SteveSandersonMS · 2018-04-06T10:18:40Z

src/Microsoft.AspNetCore.Blazor/Components/BindMethods.cs

+        public static UIEventHandler GetEventHandlerValue<T>(Action<T> value)
+            where T : UIEventArgs
+        {
+            return e => value((T)e);


Well, I am concerned that we want to minimise both the GC churn and register/unregister costs of event handlers, however I can't guess whether (a) what you're doing here definitely makes it worse vs what we had before, or (b) whether there's a simple good alternative.

Perhaps we can go ahead with what you have here right now, and then when it's in I'd like to have a look in ildasm at what's happening as a result. If it turns out that we can avoid allocations either by caching the delegates in some way or by not having the UIEventHandler type at all, that would be a good subsequent enhancement.

SteveSandersonMS · 2018-04-06T10:29:21Z

src/Microsoft.AspNetCore.Blazor/Components/EventHandlers.cs

+namespace Microsoft.AspNetCore.Blazor.Components
+{
+    [EventHandler("onchange", typeof(UIChangeEventArgs))]
+    [EventHandler("onclick", typeof(UIMouseEventArgs))]


Are the ones here the only ones that will compile correctly, or for unrecognized event names, do we fall back on some default type? It would be ideal if we did have a default type (that supplies no info other than that the event happened) for arbitrary event names. I know there's also work on the JS side to make arbitrary events work but that's separate from the compile-time work.

For the actual list of event names, we could use this list as inspiration: https://github.com/Microsoft/TypeScript/blob/master/lib/lib.dom.d.ts#L3475, which also gives hints about the possible event arg types. Assuming we have support for default fallback for unknown events, we could probably reduce our set of explicitly-typed ones down to the most common ones:

"activate": UIEvent; "beforeactivate": UIEvent; "beforedeactivate": UIEvent; "blur": FocusEvent; "change": Event; "click": MouseEvent; "contextmenu": PointerEvent; "dblclick": MouseEvent; "deactivate": UIEvent; "drag": DragEvent; "dragend": DragEvent; "dragenter": DragEvent; "dragleave": DragEvent; "dragover": DragEvent; "dragstart": DragEvent; "drop": DragEvent; "focus": FocusEvent; "input": Event; "invalid": Event; "keydown": KeyboardEvent; "keypress": KeyboardEvent; "keyup": KeyboardEvent; "mousedown": MouseEvent; "mousemove": MouseEvent; "mouseout": MouseEvent; "mouseover": MouseEvent; "mouseup": MouseEvent; "mousewheel": WheelEvent; "pause": Event; "play": Event; "playing": Event; "progress": ProgressEvent; "readystatechange": Event; "scroll": UIEvent; "seeked": Event; "seeking": Event; "select": UIEvent; "selectionchange": Event; "selectstart": Event; "stop": Event; "submit": Event; "touchcancel": TouchEvent; "touchend": TouchEvent; "touchmove": TouchEvent; "touchstart": TouchEvent; "volumechange": Event;

I know you might not want to go declaring new UIEventArgs subtypes for all these (and dozens of E2E tests) during this PR, so I think it would be fine to regard "detailed support for loads of DOM event types" as a separate work item.

SteveSandersonMS · 2018-04-06T10:32:47Z

test/testapps/BasicTestApp/PropertiesChangedHandlerParent.cshtml

@@ -1,6 +1,6 @@
 @addTagHelper "*, BasicTestApp"
 <PropertiesChangedHandlerChild SuppliedValue=@valueToSupply />
-<button onclick=@{ valueToSupply++; }>Increment</button>
+<button onclick=@(x => valueToSupply++)>Increment</button>


I'd be totally happy to drop support for the onevent=@{ statement; } syntax. It's a throwback to the very earliest Blazor prototypes, and no longer appears to be a preferable syntax in any scenario. It's always better to use the lambda syntax because without it, there's no way to receive event args.

SteveSandersonMS · 2018-04-06T10:33:56Z

test/testapps/BasicTestApp/RenderFragmentToggler.cshtml

@@ -7,7 +7,7 @@
        @ExampleFragment
    }

-    <button onclick=@{ showFragment = !showFragment; }>Toggle</button>
+    <button onclick=@(x => showFragment = !showFragment)>Toggle</button>


Super minor nitpicky preference: for anyone reading this code, the x might look a bit mysterious. I had the impression that _ was the convention for "I don't care about this arg".

rynowak · 2018-04-06T16:32:08Z

I've addressed the minor feedback here, I'm going to follow up on three things:

Add a fallback for arbitrary events
Expand the list of known events and types
Use a different strategy for delegates that will result in fewer allocations and smaller diffs

@foo

This change adds support for mapping DOM event handlers as tag helpers that function in a bi-modal way. This is a new first-class feature for DOM events, and replaces a few workarounds like using `@onclick(...)` or `click=@{ ... }`. I haven't removed those things yet, this is a first pass to get the new support in, we'll remove those things when we're totally satisfied. When used with a string like `<button onclick="foo" />` the result is a simple HTML attribute . But when used with an implicit expression like `<button onclick="@foo" />` or `<button onclick="@(x => Clicked = true)" />` a C# function is bound to the click event from the DOM.

rynowak requested a review from SteveSandersonMS April 6, 2018 02:58

rynowak commented Apr 6, 2018

View reviewed changes

rynowak force-pushed the rynowak/event-handlers branch from e548ee2 to 5b7801c Compare April 6, 2018 03:22

SteveSandersonMS approved these changes Apr 6, 2018

View reviewed changes

rynowak force-pushed the rynowak/event-handlers branch from 5b7801c to 0c6e6f4 Compare April 6, 2018 16:29

rynowak force-pushed the rynowak/event-handlers branch from 0c6e6f4 to b40ba41 Compare April 6, 2018 18:15

rynowak force-pushed the rynowak/event-handlers branch from b40ba41 to 2f14d39 Compare April 6, 2018 19:36

rynowak merged commit ce5741f into dev Apr 6, 2018

rynowak deleted the rynowak/event-handlers branch April 6, 2018 20:00

SteveSandersonMS added enhancement 3 - Done labels Apr 16, 2018

SteveSandersonMS added this to the 0.2.0 milestone Apr 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add event handlers as tag helpers #498

Add event handlers as tag helpers #498

rynowak commented Apr 6, 2018

rynowak Apr 6, 2018

rynowak Apr 6, 2018

rynowak Apr 6, 2018

SteveSandersonMS Apr 6, 2018

rynowak Apr 6, 2018

SteveSandersonMS Apr 6, 2018

rynowak Apr 6, 2018

rynowak Apr 6, 2018

rynowak Apr 6, 2018

SteveSandersonMS Apr 6, 2018

rynowak Apr 6, 2018

rynowak Apr 6, 2018

SteveSandersonMS Apr 6, 2018

rynowak Apr 6, 2018

SteveSandersonMS Apr 6, 2018

SteveSandersonMS left a comment

SteveSandersonMS Apr 6, 2018

rynowak Apr 6, 2018

SteveSandersonMS Apr 6, 2018

SteveSandersonMS Apr 6, 2018

SteveSandersonMS Apr 6, 2018

SteveSandersonMS Apr 6, 2018

SteveSandersonMS Apr 6, 2018

SteveSandersonMS Apr 6, 2018

SteveSandersonMS Apr 6, 2018

rynowak commented Apr 6, 2018

Add event handlers as tag helpers #498

Add event handlers as tag helpers #498

Conversation

rynowak commented Apr 6, 2018

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

SteveSandersonMS 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

rynowak commented Apr 6, 2018