dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.36k stars 9.99k forks source link

InputText component should provide a FocusAsync() method #24446

Closed meziantou closed 3 years ago

meziantou commented 4 years ago

The PR #23316 introduced ElementReference.FocusAsync() which is convenient to focus an html element. In my case I need to focus an <InputText> component in a form. There are 2 cases where I want to focus the element:

I currently use a custom component that inherits from InputText and provide a FocusAsync method.

@inherits InputText
@inject IJSRuntime JSRuntime

<input @ref="inputElement"
       @attributes="AdditionalAttributes"
       class="@CssClass"
       value="@CurrentValue"
       @oninput="EventCallback.Factory.CreateBinder<string>(this, __value => CurrentValueAsString = __value, CurrentValueAsString)" />

@code {
    private ElementReference inputElement;

    public ValueTask FocusAsync()
    {
        // could be replaced with inputElement.FocusAsync() with the next release
        return JSRuntime.InvokeVoidAsync("BlazorFocus", inputElement);
    }
}

In the parent component I can use FocusAsync when needed which works well.

I think InputBase<T> or each component should have a FocusAsync method, so I don't need to override components:

namespace Microsoft.AspNetCore.Components.Forms
{
    public class InputBase<T>
    {
+        public ValueTask FocusAsync();
    }
}
ghost commented 4 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

meziantou commented 4 years ago

My issue is about doing the same for the <InputXXX> components such as <InputText> or <InputNumber>. How do you focus the <input> element produced by the <InputText> component in the following example:

<EditForm Model="@exampleModel">
    <InputText id="name" @bind-Value="exampleModel.Name" /> @* How to focus this component *@
    <button type="submit">Submit</button>
</EditForm>

@code {
    private ExampleModel exampleModel = new ExampleModel();
}

I don't think ElementReference.FocusAsync is of any help here. That's why I'm suggesting adding a FocusAsync method to the InputBase<T> class.

uabarahona commented 4 years ago

Yes, I have misunderstood the issue and rushed the answer 😃

I would like to work on this so I will check again and will back with a proposal

zHaytam commented 4 years ago

Doesn't this mean that all InputX components will have an ElementReference by default?

meziantou commented 4 years ago

Indeed, you need to add an ElementReference somewhere. The hard part is making it inheritance friendly. Indeed the child component can override the RenderTreeAsync method so the expected ElementReference is not set and the FocusAsync method doesn't work anymore. I'm not sure there is a way to enforce the child class to set the ElementReference or override the FocusAsync method.

uabarahona commented 4 years ago

Yes @zHaytam, altrought I don't think the ElementReference need to be public but we need that to implement it, here is my proposal:

Starting with a very straightforward approach we could make the following changes

public abstract class InputBase<TValue> : ComponentBase, IDisposable
{
    ----
    protected ElementReference? BaseElementReference { get; set; } = default!;

    ----
    public ValueTask FocusAsync() => BaseElementReference?.FocusAsync() ?? default;
}
protected override void BuildRenderTree(RenderTreeBuilder builder)
{
    ----
    builder.AddElementReferenceCapture(5, __elementReference => BaseElementReference = __elementReference);
    ----
}

⚠ Note: Unfortunately InputRadio is not a InputBase so we will need to reproduce this logic only for that component

Looking forward

I have noticed a bunch of calls are duplicated on each Input component

builder.AddMultipleAttributes(1, AdditionalAttributes);
builder.AddAttribute(2, "class", CssClass);

and now the new addition will too

builder.AddElementReferenceCapture(5, __elementReference => BaseElementReference = __elementReference);

Not sure what you think but we can have a method on InputBase that receives the current sequence , do the common calls for the builder and finally return nextSequence. I thought this can be a method on InputExtensions but some attributes are not public so it will not work there.

@javiercn @mkArtakMSFT Please when you have time let me know what you think about the proposal and if you are agree I can continue with the implementation.

uabarahona commented 4 years ago

@meziantou and for inheritance I believe you will need to do this:

@inherits InputText

<input @attributes="AdditionalAttributes"
       class="@CssClass"
       value="@CurrentValue"
       @ref="@BaseElementReference"
       @oninput="EventCallback.Factory.CreateBinder<string>(
         this, __value => CurrentValueAsString = __value,
         CurrentValueAsString)" />

Where @ref="@BaseElementReference" is the key, that way FocusAsync can continue working

pablopioli commented 4 years ago

IF there were an additional level in the "form helpers" like InputText

InputBase --> FormInputs --> InputText

I would implement this in the theoretical class FormInputs, as they are abstractions to build things fasts. If you are overriding InputBase you could be doing a lot of different things.

But, as this additional level does not exists I believe the proposed solution is a good trade off.

MackinnonBuck commented 4 years ago

I like the proposal @barahonajm is suggesting. A few thoughts:

uabarahona commented 4 years ago

Thanks for your answers @MackinnonBuck @pablopioli !, I will look at sequence number more deeply.

The reasons behind having BaseElementReference public sounds promising indeed it may solve the conflict I was having, see, from what I saw the Focus() is not function limited to inputs it can be used everywhere so, does it make sense to have BaseElementReference public into the ComponentBase? and then let the user somehow define their BaseElementReference so they can have access to all these helper methods like FocusAsync

MackinnonBuck commented 4 years ago

@barahonajm I acknowledge the advantages of moving BaseElementReference to ComponentBase as you've outlined, but I would argue that it doesn't belong there for a couple reasons:

Something else to consider - InputRadioGroup derives from InputBase, but it doesn't actually render any HTML elements explicitly, so BaseElementReference doesn't even make sense in that context. There are a few ideas that come to my head about how to handle this (in no particular order):

  1. Make BaseElementReference an abstract property and have InputRadioGroup throw a meaningful exception when someone attempts to access it (this is preferable to a nondescript NullReferenceException that would otherwise occur).
  2. Introduce an interface for InputBase types that wrap a single HTML element. If we want to share implementation between classes deriving from the interface, we can do so via an extension class.
  3. Add BaseElementReference to each component where it makes sense, and don't introduce any new types.

@SteveSandersonMS @pranavkm Do you have any opinions about the best direction to take this?

uabarahona commented 4 years ago

@MackinnonBuck I am agree, give Focus ability to every Component seems overkill ( HTMLElement.Focus() blinded me a bit, we might discover later why it is available for every html element and why is needed)

pranavkm commented 4 years ago

Do these abstractions really add anything of value when implementations can wildly differ? Can individual implementations that support this feature just have a FocusAsync method?

uabarahona commented 4 years ago

Indeed, that is the straightforward solution but 90% of the current input components will do have the same implementation for FocusAsync and I believe FocusAsync will always have the same implementation, what is going to change is how BaseElementReference is set alternatively we can just follow the idea of making BaseElementReference public and let the users call [RefToInputHere].BaseElementReference.FocusAsync()

I can make a PR with the straightforward solution and continue the discussion from there or maybe this can be labeled as something to talk about on your local discussion 🧑‍🏭

MackinnonBuck commented 4 years ago

I think I agree with @pranavkm on this (I was personally leaning toward #3 in my previous comment). The implementations between various input components might be similar, but introducing new types and sharing code for what would otherwise be about 2 LOC per component does seem a bit overkill. There's also no guarantee that future changes will fit the mold we apply here, so I think having separate implementations is a good choice.

@pranavkm Do you think exposing an ElementReference to the wrapped HTML element could be preferable to adding FocusAsync since it

  1. Allows users to add their own functionality via JS interop if needed, and
  2. will allow input components to adapt new extension methods added to ElementReference without us having to change the API?
pranavkm commented 4 years ago

Do you think exposing an ElementReference to the wrapped HTML element

I'm slightly partial to not exposing the implementation details of components, but it's unlikely we would have ever an InputText that does not use an input element. I think it should be fine to expose it. Could we call it something other thanBaseElementReference? e.g.InputElementorElementReference(does that mean it's calledSelectElementforInputSelect`?)

class InputText
{
+ ElementReference InputElement { get; }
}

Usage:

@code{
  InputText inputText;
  protected override async Task OnAfterRenderAsync(bool firstRender)
  {
       if(firstRender)
            await inputText.InputElement.FocusAsync();
   }
}
MackinnonBuck commented 4 years ago

I think calling it InputElement for <input>-based elements and SelectElement for InputSelect sounds good.

uabarahona commented 4 years ago

Thank you so much for your help, I am going to proceed according with the discussion