ElementReference FocusAsync API

[MackinnonBuck]

Summary of the changes:

• Added FocusAsync() extension method to ElementReference that focuses the element associated with the reference.
• Added tests to verify that FocusAsync() works properly.
• Improved API to omit the IJSRuntime argument from FocusAsync()

Addresses #22892

[dnfadmin]

All CLA requirements met.

[SteveSandersonMS]

This looks great, and it's awesome to see the E2E test in there as well!

I'm guessing this is marked draft because you're going on to do the "omit JSRuntime arg" bit, so I'll wait for updates :)

[javiercn]

Looks great!

[pranavkm]

This is really good! Just a few more things to address and we can get this in.

[pranavkm]

Almost there!

[javiercn]

Overall looks great!

I think we can tweak it a bit. I don't love the part where the IServiceProvider is spilled across the application and it feels that we are missing an abstraction.

I would consider creating an ElementReferenceFactory with an implementation on M.A.C.Web that plugs in the JS runtime. The JsonConverter and the Renderer only receive a ElementReferenceFactory and the ElementReference only holds on to whatever it needs on a given platform.

We would do something like

Renderer.ElementFactory.CreateWithUniqueId()

and the remote RemoteJSRuntime would simply instantiate its own ElementReferenceFactory passing itself in the constructor.

[SteveSandersonMS]

I would consider creating an ElementReferenceFactory with an implementation on M.A.C.Web that plugs in the JS runtime

That's a bit awkward because ElementReference is a struct (and needs to be), so there's nowhere to even hold additional information. We could add some kind of object-valued ExtraInfo field onto it, but that's pretty weird and doesn't compose well if we end up needing to get access to more stuff from an ElementReference in the future.

Additionally, although for high-level features it's generally fine to push code through factories and more abstractions, the perf costs are prohibitive for code in the deep guts of rendering. There are very legit cases for building many thousands of ElementReferences per second (e.g., wanting to be able to reference each td in a grid).

I also don't really have any particular objection to referencing the IServiceProvider from ElementReference.

[javiercn]

That's a bit awkward because ElementReference is a struct (and needs to be), so there's nowhere to even hold additional information. We could add some kind of object-valued ExtraInfo field onto it, but that's pretty weird and doesn't compose well if we end up needing to get access to more stuff from an ElementReference in the future.

It's already holding on to the IServiceProvider, so I don't see the difference here. I'm saying, initialize it with the JSRuntime only and through a Factory instead of giving it the IServiceProvider. I think having the IServiceProvider there encourages customers to do ElementReference.ServiceProvider.GetRequiredService<PartyOn>() which can easily get out of hand.

We designed element references to be an opaque abstraction, and we should keep treating them like that. For that reason I would have the factory live on M.A.C.Web, make it responsible for populating the state, and using an internal type that only M.A.C.Web can understand (like an internal class WebElementState) that contains the JSRuntime.

That way, the ElementReference is still opaque and we are still able to implement focus without any details leaking out.

Additionally, although for high-level features it's generally fine to push code through factories and more abstractions, the perf costs are prohibitive for code in the deep guts of rendering. There are very legit cases for building many thousands of ElementReferences per second (e.g., wanting to be able to reference each td in a grid).

I don't think what I'm proposing would have a negative impact on perf, it would be the opposite if anything. The factory is accessed from the renderer or is part of the converter and gives clear and scoped functionality.

Then on the ElementReference we just have state as an internal property (object) that is populated with the IJSRuntime. The helpers then just do a if(state is IJSRuntime jsRuntime){ jsRuntime.InvokeAsync("focus",this)) which is cheaper than asking the container to resolve the jsruntime.

In conclusion:

• The factory makes the intent clear and the functionality scoped.
• Doesn't incurs into any additional performance cost, if anything makes it faster on repeated uses.
• Prevents against pit of failure using the service locator pattern with the given service provider.

[pranavkm]

Thanks @javiercn. Having a factory with a default that just news up the ElementReference seems fine. It would also clean up
ElementReference.CreateWithUniqueId inverting the responsibility to the caller.

a) It would be convenient if there was a default factory that constructed the type without state.

interface IElementReferenceFactory
{
   ElementReference Create();
}

ElementReference
{
  public ElementReference(string id);
  public ElementReference(string id, object? state);
}

Renderer.ElementReferenceFactory { get; } = ServiceProvider.GetService<ElementReferenceFactory> ?? DefaultElementReferenceFactory.Instance;

var newElementReference = Renderer.ElementReferenceFactory.Create();

It might also be useful to also add an extension to Components.Web that does the cast for you:

public static IJSRuntime GetJSRuntime(this ElementReference reference)
{
   return reference.State as IJSRuntime ?? throw "This hasn't been correctly configured.";
}

[SteveSandersonMS]

@pranavkm Let discuss all this at the sync later today! Obviously with @MackinnonBuck too.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[pranavkm]

I'll bring this up during API review on Monday and address any feedback separately. Let's get this in to avoid further merge conflicts