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

Control event propagation and default action. Implements #5545 #14509

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Sep 27, 2019

This PR is the runtime part of #5545, which is hopefully the vast majority of it. The remaining tooling work is to add directive attributes for @oneventname:stopPropagation and @oneventname:preventDefault.

Design notes

This closely follows the plan described here.

The first major decision point here was to make this entirely a feature of .Web/.Web.JS and not part of .Components core at all. I think that's the right thing to do, because the vast majority of the implementation is in .Web.JS and there's no reason to say that some other non-web UI platform would have any meaning for "prevent default", and another platform need not necessarily have a concept of event bubbling/propagation. The semantics of both are extremely closely tied to DOM semantics. As such, the rendertree APIs for attaching this information are given as extension methods that live in .Web, so the compiler is only going to be able to use them if you're referencing .Web.

The second major decision point was how to represent these flags in the rendertree. Note that these flags behave like attributes:

  • You can have multiple of them on an element (e.g., for different events)
  • They are independent of other attributes (e.g., you can stop "click" propagation on some element without necessarily also having a "click" handler on that same element)
  • They can be added and removed dynamically

I think eventually we will want to introduce a concept of "attribute type". Today there are several types of attributes (regular string/bool attributes, event handler attributes, component parameters, and now stopPropagation/preventDefault ones), but we don't explicitly declare their types anywhere, and instead handle things based on conventions around where they are used and what their value types are. There is a slot in RenderTreeFrame where we could put an AttributeType enum value and hence be more explicit about all this. At that point, it would make sense for a "stop propagation" attribute to have AttributeType.StopPropagation and name=nameOfTheEvent. Given the attribute type discriminator, we could overload the memory slots with the "attribute" frame type to have different meanings for different types, letting us scale this whole system up further without consuming more memory.

However, I don't think attribute types are useful for this purpose until we also solve "allow multiple same-named attributes" (#14365). So in the short term, my plan is:

  • Keep the internal representation of stopPropagation/preventDefault information as a private implementation detail. The APIs you (or the compiler) call are builder.AddEventPreventDefaultAttribute and builder.AddEventStopBubblingAttribute, but you don't get to know what they are doing to set that information. How they represent themselves in the tree can change in the future non-breakingly, as long as the APIs and their resulting behavior remains unchanged.
  • The actual thing they do today is the simplest thing: they just set magic-named attributes. Again, it's an internal implementation detail.

If/when we later do #14365, we could change the internal behavior of AddEventPreventDefaultAttribute and AddEventStopBubblingAttribute to make use of it.

Review notes

Most of the complicated-looking noise in EventDelegator.ts is just refactoring. Previously it used a very loosely-typed scheme for tracking event data, with lots of duplicated checks like Object.hasOwnProperty('something'). Now it's more explicitly typed. I did this just because I'm storing more information here now, and didn't like to expand the ad-hoc loose-typeness.

I'm not trying to override or change DOM semantics for event propagation and its interaction with navigation. So,

  • events affect each other, e.g., if you preventDefault on a checkbox click that will prevent the subsequent 'change'
  • you can only preventDefault on an element (or one of its ancestors) that actually listens for that event
  • you can only receive onfocus events on elements that have a tabindex attribute, and it doesn't bubble anyway
  • preventDefault does nothing on onfocus

These are all browser behaviors and not controlled by Blazor.

Similarly, stopPropagation does not block navigation. You might think that stopping a "click" from propagating up to an <a href...> would mean the browser doesn't navigate. But you'd be wrong: the native behavior is that it navigates anyway. So I've reproduced the same with Blazor's synthetic event bubbling mechanism and its link click interception. There are E2E tests for this.

@SteveSandersonMS SteveSandersonMS added the area-blazor Includes: Blazor, Razor Components label Sep 27, 2019
@SteveSandersonMS
Copy link
Member Author

Also, the E2E tests hard-code the magic-named attributes. This can be replaced with @oneventname:stopPropagation and @oneventname:preventDefault once they are implemented.

@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/control-event-bubbling-and-default-action branch from dc10e71 to 395e2f2 Compare September 29, 2019 21:30
@SteveSandersonMS SteveSandersonMS added this to the 3.1.0-preview2 milestone Sep 30, 2019
@SteveSandersonMS
Copy link
Member Author

Ping to reviewers @rynowak @pranavkm.

Even though the tooling side won't be in preview 2, it would be valuable to have the runtime part in, so that (1) it doesn't get out of sync and need more work later and (2) for extra verification that my refactoring hasn't broken anybody. There's no drawback to shipping the runtime part that I'm aware of.

/// </summary>
public static class RenderTreeBuilderExtensions
{
// The "prevent default" and "stop propagation" flags behave like attributes, in that:
Copy link
Member

Choose a reason for hiding this comment

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

FYI the compiler support for this will need to do feature detection to see if these APIs are defined. Also the compiler won't likely codegen them as extension methods just because it's not something we tend to do - I'll leave that up to @ajaybhargavb though.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rynowak @SteveSandersonMS, do we plan to do the compiler work for this in preview2?

Copy link
Member

Choose a reason for hiding this comment

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

3

/// <param name="value">True if the default action is to be prevented, otherwise false.</param>
public static void AddEventPreventDefaultAttribute(this RenderTreeBuilder builder, int sequence, string eventName, bool value)
{
builder.AddAttribute(sequence, $"__internal_preventDefault_{eventName}", value);
Copy link
Member

Choose a reason for hiding this comment

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

Removing this repeated allocation is another good reason to build a feature for this (if we ever get around to it).

<div __internal_preventDefault_onclick="@ancestorPreventDefault">
<p>
<label>
<input type="checkbox" id="checkbox-with-preventDefault-true" __internal_preventDefault_onclick @onclick="@(() => LogEvent("Checkbox click"))" @onchange="@(() => LogEvent("Checkbox change"))" />
Copy link
Member

Choose a reason for hiding this comment

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

Scrolling through these - it looks like this is the only coverage of the minimized form. @ajaybhargavb - let's make sure we preserve the semantics of this.

@SteveSandersonMS SteveSandersonMS changed the base branch from release/3.1 to release/3.1-preview1 October 1, 2019 17:06
@mkArtakMSFT mkArtakMSFT merged commit 2fc6041 into release/3.1-preview1 Oct 1, 2019
@ghost ghost deleted the stevesa/control-event-bubbling-and-default-action branch October 1, 2019 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants