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

Improve JS-side event handling code #16293

Closed
SteveSandersonMS opened this issue Mar 29, 2018 · 11 comments
Closed

Improve JS-side event handling code #16293

SteveSandersonMS opened this issue Mar 29, 2018 · 11 comments
Labels
area-blazor Includes: Blazor, Razor Components

Comments

@SteveSandersonMS
Copy link
Member

Currently the event handler registration logic in BrowserRenderer.ts is not very good. It only supports having a single event handler per element for a given event name, and it doesn't use event delegation, so it's creating new native handlers for every element that responds to events. Also it's hard-coded to only work with onclick, onchange, and onkeypress. It was just put in quickly with the intention of being replaced.

We should replace this with a simple event delegation system, and add tests to show that it copes with (for example) both bind and onchange on the same element, plus even custom event names.

@grahamehorner
Copy link
Contributor

@SteveSandersonMS I've been looking into creating an EventBroker using Rx.DOM and have a simple working POC where a developer can register an event handler (observer) if the source element event hasn't been registered previously then the EventBroker creates an observable using the fromEvent holding onto the source, which is subscribed to internal EventBroker methods that in turn raises events to the observer that subscribed to the EventBroker events using a filter.

@bdparrish
Copy link
Contributor

@grahamehorner @SteveSandersonMS

I am looking into how the approach would be for this to replace what is already in BrowserRenderer.ts and this is what I came up with so far, but looking to bounce off ideas.

Replace the switch statement here with ....

if (attributeName.startsWith('on')) {
    let callback: EventListener = evt => raiseEvent(evt, browserRendererId, componentId, eventHandlerId);

    if (toDomElement['_blazorDeregisterEvent'] && toDomElement['_blazorDeregisterEvent'][attributeName]) {
      toDomElement['_blazorDeregisterEvent'][attributeName]();
    }
      
    toDomElement['_blazorDeregisterEvent'][attributeName] = () => this.addEventListener(toDomElement, attributeName, callback);
}

I want to pull out the non generic event object and move it to new interfaces/classes in order to determine how to construct it so that the C# code on BrowserRendererEventDispatcher.cs can handle it. It looks like everything must be tightly coupled here in regards to C# has to understand the JS object and vice versa. Is there a better way to approach this so that JS and C# code can be decoupled?

A more robust option might be to provide default event parsing libraries to convert JSON objects to a C# object. For example, convert a MouseEvent (JS) to a UIMouseEvent (C#). This way we can utilize what is already built into JS as to minimize the maintenance on that side.

var event = EventJSON.TryParse<UIMouseEvent>(event);

This way you can utilize the JS Event that gets passed into raiseEvent().

@grahamehorner
Copy link
Contributor

@bdparrish I'm starting to come to a similar conclusion around capturing the DOM event raise and serializing the event into a json payload that is then raised to the .NET EventBroker (if required) in essence you have an EventBroker on each side that has a strong contract passing the events via the interop layer. I was initially looking at RxJs DOM and Rx.NET as the core for the EventBroker and have a POC working; but I'm a noob with the client side scripting stuff

@bdparrish
Copy link
Contributor

@grahamehorner

The RxJS works like a Pub/Sub pattern from everything that I have used it for. You would still need to rewrite the addEventListener() and removeEventListener() parts of the BrowserRenderer.ts. Both parts are going to be tightly coupled for the most part. I think Blazor is only going to be able to eliminate the tight coupling on one side, so I would suggest that BrowserRendererEventDispatcher.cs handles a well defined contract that would come from the JS side.

@grahamehorner
Copy link
Contributor

@bdparrish not sure what you driving at here? Rx/RxJs is an observer pattern on streams of data; UI events are simply streams of data; all the addEventListener/removeEventListener logic would be handled by the using Rx.Observable.fromEvent

var input = $('#input');
var source = Rx.Observable.fromEvent(input, 'click');

the resulting observable (source) can be subscribe to by one or more observers; the scoping of the observable would be controlled by the EventBroker with a signal observer bound to the events of observables; and filtered/dispatched to EventBroker registered handlers using promise/async calls

@grahamehorner
Copy link
Contributor

RxJs can also be used to throttle events/aggregate etc. should it be needed to prevent performance issues with interop calls aka backpressure

@bdparrish
Copy link
Contributor

bdparrish commented Mar 30, 2018

@grahamehorner

I am saying Pub/Sub, but synonymous with Observable in my mind, hence the subscribe() in RxJS. It just seems like you are putting an extra layer (dependency) into the system by using RxJS to register event listeners. The other performance optimization seems like premature optimization right now. The issue is how do we go from hard-coded events to dynamic events unless I am not seeing the whole picture that is trying to be solved with this ticket. The fromEvent() seems to be extra overhead due to the fact that you have to 1) add the event listener, 2) create the event, 3) fire the event, and 4) subscribe to the event to do something with the event data or not subscribe if you want to truncate the chain before the subscribe at step 2. The only benefit is that RxJS makes it easy for you to subscribe multiple click events to the same input if you so choose to do that.

I still think the below is a more sensible approach so that Blazor can fluctuate easily with JS changes in the future to take the event that JavaScript already builds for you and then send that over to C# to process.

platform.callMethod(raiseEventMethod, null, [
    platform.toDotNetString(JSON.stringify(eventDescriptor)),
    platform.toDotNetString(JSON.stringify(event))
]);

and then (there is a way better way to do this but getting thought on "paper") ...

private static UIEventArgs ParseEventArgsJson(string event)
{
    if (JsonUtil.TryParse<UIMouseEventArgs>(event, out var mouseEvent))
    {
        return mouseEvent;
    }
    else if (JsonUtil.TryParse<UIKeyboardEentArgs>(event, out var keyboardEvent))
    {
        return keyboardEvent;
    }
    ... etc ...
}

This approach limits the JS code and lets the Blazor system handle it the way it wants to and you still get known data.

@grahamehorner
Copy link
Contributor

Please consider adding WheelEvent to allow triggering of .net code from js when the mouse wheel is used to scroll, eg. scrolling could be used to preload/process data from a service which updates the DOM elements

SteveSandersonMS referenced this issue in SteveSandersonMS/BlazorMigration Nov 27, 2018
@ChrisMyrick
Copy link

Please consider adding the ability to handle BOM window events. An example use case would be in creating a custom install dialog for a Blazor PWA by leveraging the "beforeinstallprompt' event.

    window.addEventListener('beforeinstallprompt', (e) => {
        console.log('beforeinstallprompt', e);
        // prevent Chrome 67 and earlier from automatically showing the prompt
        e.preventDefault();
        // save the event so it can be triggered later.
        window.deferredPrompt = e;
        ...

@danroth27
Copy link
Member

@ChrisMyrick Could you please open an issue for this request on https://github.com/aspnet/aspnetcore/issues? That's where all the Blazor work happening now.

@ChrisMyrick
Copy link

@danroth27 - Yes, thank you. Done.

@mkArtakMSFT mkArtakMSFT transferred this issue from dotnet/blazor Oct 27, 2019
@mkArtakMSFT mkArtakMSFT added the area-blazor Includes: Blazor, Razor Components label Oct 27, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

No branches or pull requests

6 participants