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

EventHandlerAttribute: enableStopPropagation and enablePreventDefault are swapped #10497

Open
FStolte opened this issue Jun 17, 2024 · 2 comments
Assignees
Labels
area-compiler Umbrella for all compiler issues
Milestone

Comments

@FStolte
Copy link

FStolte commented Jun 17, 2024

Summary

The parameters enableStopPropagation and enablePreventDefault of the EventHandlerAttribute are swapped by the razor compiler, so that setting enableStopPropagation = true actually enables preventDefault, and vice versa.

Steps to reproduce

I added the following class to my project in order to add support for the "transitionend" event to blazor:

[EventHandler("ontransitionend", typeof(EventArgs), enableStopPropagation: true, enablePreventDefault: false)]
public static class EventHandlers { }

When I set @ontransitionend=SomeCallback on a div in one of my components, this is recognized immediately by the compiler, according to the syntax highlighting.
However, when I try to set @ontransitionend:stopPropagation on another div, this is not recognized (the whole string was red like an html-attribute). When I then start my app, I get an exception complaining about a malformed string, i.e. the unprocessed "@" in the html-attribute name.
Furthermore, adding @ontransitionend:preventDefault is recognized and handled by the compiler, when it should be disabled. Swapping the values for both parameters results in the exact opposite behavior.

Possible cause

enablePreventDefault = (bool)attribute.ConstructorArguments[2].Value;
enableStopPropagation = (bool)attribute.ConstructorArguments[3].Value;

In EventHandlerTagHelperDescriptorProvider, the constructor arguments of the EventHandlerAttribute are accessed in the wrong order. The variable enablePreventDefault is being assigned the argument with index 2, when that actually is the enableStopPropagation parameter; the same goes for the enableStopPropagation variable being assigned the argument with index 3.

As far as I can see, this is the only place that accesses the values of these constructor arguments or the properties they are assigned to.

@davidwengier davidwengier added the area-compiler Umbrella for all compiler issues label Jun 17, 2024
@jjonescz
Copy link
Member

Thanks for reporting this issue. Looks like you found the cause correctly.

I guess no-one noticed this issue because most EventHandlers defined in aspnetcore use true for both: https://github.com/dotnet/aspnetcore/blob/99f2ba5fd1d668fecbff1e1a9ba6a541c99bec79/src/Components/Web/src/Web/EventHandlers.cs

@chsienki chsienki self-assigned this Jul 11, 2024
@chsienki chsienki added this to the 17.12 Preview 1 milestone Jul 11, 2024
@DustinCampbell
Copy link
Member

I just noticed the bug while fixing nullability warnings in that code. 😄

DustinCampbell added a commit to DustinCampbell/razor that referenced this issue Aug 8, 2024
- Enable nullability
- Mark all as sealed
- Use `Lazy<T>` for singleton `ITagHelperDescriptorProviders`
- Introduce `TagHelperDescriptorProviderBase`
- Inherit from `TagHelperDescriptorProviderBase`
- Remove unnecessary properties
- Remove unnecessary property setters

In addition, I spotted a bug in `EventHandlerTagDescriptorProvider` while addressing nullability warnings. The buggy code is right here:

https://github.com/dotnet/razor/blob/fb84ae5d9bb8132972c2c23cf209721161f81deb/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/CSharp/EventHandlerTagHelperDescriptorProvider.cs#L70-L76

When reading the constructor arguments of an `EventHandlerAttribute` it appears that we swap the last two arguments of the 4-argument variant. The constructor is defined like so:

```C#
EventHandlerAttribute(string attributeName, Type eventArgsType, bool enableStopPropagation, bool enablePreventDefault);
```

Unfortunately, the compiler reads the third parameter as `enablePreventDefaeult` and the fourth parameter as `enableStopPropagation`.

This has been broken since support for the 4-argument constructor variant was added in dotnet@7635bba. Fixing this bug is tracked by dotnet#10497.
DustinCampbell added a commit that referenced this issue Aug 12, 2024
Every `TagHelperDescriptorProviderContext` creates an `ItemCollection`
to hold onto, at most, two objects: a `Compilation` and a target
`ISymbol`. This is enormously wasteful because an `ItemCollection`
internally creates a `ConcurrentDictionary<object, object>`. So, I've
changed `TagHelperDescriptorProviderContext` to just hold onto a
compilation and a target symbol and avoid the `ItemCollection`
altogether.

I recommend reviewing commit-by-commit.

Also, I bumped into a long standing compiler bug in
`EventHandlerTagHelperDescriptorProvider` that was recently filed by a
customer: #10497. I opted to stay
on target and not fix this issue, but I made it painfully obvious where
the fix would go. 😄
jaredpar pushed a commit that referenced this issue Aug 12, 2024
Every `TagHelperDescriptorProviderContext` creates an `ItemCollection`
to hold onto, at most, two objects: a `Compilation` and a target
`ISymbol`. This is enormously wasteful because an `ItemCollection`
internally creates a `ConcurrentDictionary<object, object>`. So, I've
changed `TagHelperDescriptorProviderContext` to just hold onto a
compilation and a target symbol and avoid the `ItemCollection`
altogether.

I recommend reviewing commit-by-commit.

Also, I bumped into a long standing compiler bug in
`EventHandlerTagHelperDescriptorProvider` that was recently filed by a
customer: #10497. I opted to stay
on target and not fix this issue, but I made it painfully obvious where
the fix would go. 😄
@jaredpar jaredpar modified the milestones: 17.12 Preview 1, Backlog Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-compiler Umbrella for all compiler issues
Projects
None yet
Development

No branches or pull requests

6 participants