fix: Setting Loading in FluentButton bound to FluentInputFile breaks it

[Leon99]

🐛 Bug Report

After setting Loading in FluentButton bound to FluentInputFile, it won't open the file browse dialog anymore.

💻 Repro or Code Sample

                    <FluentButton
                        Id="form-browse-button"
                        Loading="_formUploading">
                        Upload...
                    </FluentButton>
                <FluentInputFile
                    Accept="application/pdf"
                    AnchorId="form-browse-button"
                    DragDropZoneVisible="false"
                    Id="form-file-uploader"
                    Mode="InputFileMode.SaveToTemporaryFolder"
                    OnCompleted="OnFormUploadCompleted"
                    OnProgressChange="OnFormUploading"/>
@code {
    bool _formUploading;
    void OnFormUploading()
    {
        _formUploading = true;
    }
    async Task OnFormUploadCompleted(IEnumerable<FluentInputFileEventArgs> files)
    {
        _formUploading = false;
    }
}

🌍 Your Environment

• OS & Device: Windows on PC
• Browser: Microsoft Edge, Mozilla FireFox
• .NET and Fluent UI Blazor library Version: 8.0.400/4.10.1

[vnbaaij]

I can't find what is causing this. As an alternative I would use the Disabled property instead. Tested that and can confirm it works.

Will leave this open for a bit but it is not going to be treated as a high priority issue.

[Leon99]

Thanks @vnbaaij, Disabled plus a separate ProgressRing is what I ended up doing as a workaround. Would be good to, at least, make a note of this limitation in the documentation, to make sure others don't spend as much time as I did trying to localize the issue upon testing a number of changes. I'd say it's not an unexpected use case, since file uploading always requires a progress indicator and using Loading on a button seems to be the easiest way to do it.

[vnbaaij]

Thanks @vnbaaij, Disabled plus a separate ProgressRing is what I ended up doing as a workaround. Would be good to, at least, make a note of this limitation in the documentation, to make sure others don't spend as much time as I did trying to localize the issue upon testing a number of changes.

Yes, I'll make a note of that. This is supper 'low hanging fruit' where you can help the community yourself (an us) by creating a simple PR for it.

I'd say it's not an unexpected use case, since file uploading always requires a progress indicator and using Loading on a button seems to be the easiest way to do it.

It is not an unexpected use case indeed, but you are the first person that has reported this in the ~2.5 years we've had this component.

I'm adding the 'community:good-first-issue' and 'community:contribution' labels with the hope that someone is willing to dive into this deeper.

[MarvinKlein1508]

Hi @vnbaaij

After further investigation, I believe I have identified the main cause of this issue. In my opinion, the problem is related to FluentInputFile rather than FluentButton.

The FluentButton is defined like this:

@if (LoadingOverlay)
{
    <span class="@($"{Class} loading-button")" style="@Style">
        @_renderButton
        <FluentProgressRing />
    </span>
}
else
{
    @_renderButton
}

When the loading overlay is rendered, the initially rendered button is removed from the DOM. As a result, the event registered for the original button is lost. This happens because FluentInputFile registers the event only once.

    protected override async Task OnAfterRenderAsync(bool firstRender)
    {
        if (firstRender)
        {
            Module ??= await JSRuntime.InvokeAsync<IJSObjectReference>("import", JAVASCRIPT_FILE.FormatCollocatedUrl(LibraryConfiguration));

            if (!string.IsNullOrEmpty(AnchorId))
            {
                await Module.InvokeVoidAsync("attachClickHandler", AnchorId, Id);
            }

            _containerInstance = await Module.InvokeAsync<IJSObjectReference>("initializeFileDropZone", _containerElement, _inputFile!.Element);
        } 
    }

When I move this block outside of the firstRender section like this:

protected override async Task OnAfterRenderAsync(bool firstRender)
{
    if (firstRender)
    {
        Module ??= await JSRuntime.InvokeAsync<IJSObjectReference>("import", JAVASCRIPT_FILE.FormatCollocatedUrl(LibraryConfiguration));

        _containerInstance = await Module.InvokeAsync<IJSObjectReference>("initializeFileDropZone", _containerElement, _inputFile!.Element);
    }

    if (!string.IsNullOrEmpty(AnchorId) && Module is not null)
    {
        await Module.InvokeVoidAsync("attachClickHandler", AnchorId, Id);
    }
}

After making this change, it works perfectly for me. Please let me know your thoughts, and I will proceed to open a pull request for this.

[dvoituron]

I don't think you can do that: each time the page will be rendered, this JS code will be executed and a click event will be added.

button.addEventListener("click", function (e) {
    fileInput.click();
});

[MarvinKlein1508]

@dvoituron yea but you can prevent this by removing the handler first.

export function attachClickHandler(buttonId, fileInputId) {
    var button = document.getElementById(buttonId);
    var fileInput = document.getElementById(fileInputId);

    if (button && fileInput) {
        button.removeEventListener("click");
        button.addEventListener("click", function (e) {
            fileInput.click();
        });
    }
}

[dvoituron]

The removeEventListener method requires both the type of the event (e.g., "click") and a reference to the exact function that was added as an event listener. Without providing the function reference, the browser doesn’t know which event listener to remove.

https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/removeEventListener

[MarvinKlein1508]

@dvoituron my bad. How about:

export function attachClickHandler(buttonId, fileInputId) {
    var button = document.getElementById(buttonId);
    var fileInput = document.getElementById(fileInputId);

    if (button && fileInput) {
        const handler = function (e) {
            fileInput.click();
        };

        button.removeEventListener("click", handler);
        button.addEventListener("click", handler);
    }
}

[MarvinKlein1508]

Or we can simplify it even more:

export function attachClickHandler(buttonId, fileInputId) {
    var button = document.getElementById(buttonId);
    var fileInput = document.getElementById(fileInputId);

    if (button && fileInput && !button.onclick) {
        button.onclick = function (e) {
            fileInput.click();
        };
    }
}

[dvoituron]

To verify :-)

I'm not sure:

• The handler function is defined each time attachClickHandler is called. This creates a new function reference every time the method is invoked.
• However, since handler is a new function instance, it won't match any previously attached event listeners, even if their functionality is identical.

Using !button.onclick is better, but if the dev added a manual click on this button (for another stuff), that will not work. Sorry :-)

I understand, but not easy... may be, adding a "flag" to detect this "addEventListener".

[MarvinKlein1508]

@dvoituron damn I hate JS. But you were right. I think the only way is by setting a data attribute then.

export function attachClickHandler(buttonId, fileInputId) {
    var button = document.getElementById(buttonId);
    var fileInput = document.getElementById(fileInputId);

    if (button && fileInput && !button.hasAttribute("data-click-handler-attached")) {

        button.addEventListener("click", function (e) {
            fileInput.click();
        });

        button.setAttribute("data-click-handler-attached", "true");
    }
}

[dvoituron]

Argg... not using setAttribute, because you will have this info in the HTML code. You can dynamically create a property button.fluentuiBlazorFileInputHandlerAttached = true ;-)

But that is the idea.

[MarvinKlein1508]

Damn I didn't even know that's possible! But it works fine for me. Guess you'll never stop learning.

export function attachClickHandler(buttonId, fileInputId) {
    var button = document.getElementById(buttonId);
    var fileInput = document.getElementById(fileInputId);

    if (button && fileInput && !button.fluentuiBlazorFileInputHandlerAttached) {

        button.addEventListener("click", function (e) {
            fileInput.click();
        });

        button.fluentuiBlazorFileInputHandlerAttached = true;
    }
}

[vnbaaij]

Like the dynamic property!

Although, how bad would it be to have the data attribute in the HTML. That is what they are there for, right?

[dvoituron]

Although, how bad would it be to have the data attribute in the HTML. That is what they are there for, right?

If this information is useful for the user interface or needs to be known by the developer. But not for “internal” use.

[MarvinKlein1508]

@vnbaaij if I have ever learnt one thing in Blazor than it is you should almost prevent manipulating the DOM using JS. Makes many stuff much harder to maintain in the longrun. In this case I thought it wouldn't be an issue since the attribute will be removed everytime the loading spinner occurs which is excatly what this problem is about.

But if we can prevent using it, why not.

If you both agree with this approach then I will create a PR for this and add a new example for this. Not sure about tests though. I'Ve never created any for blazor components.

[dvoituron]

But I'd rather find a way to “detect” this behavior from C#. Otherwise, calling this method won't always do the job. That is not "logical". Is it possible to keep a similar "flag" in C#?

await Module.InvokeVoidAsync("attachClickHandler", AnchorId, Id);

[MarvinKlein1508]

@dvoituron I think this will be pretty difficult because the issue is a combination of 2 different components. How is the FluentInputFile component supposed to know that the FluentButton has altered it's rendering.

The only logical way I can think of is checking the property before calling the method. But I find it much harder to understand than simply repeatly calling the method to register the handler. IMO it isn't a bad design either because methods are designed to return when nothing has to be done.

[dvoituron]

You probably right... Let'go :-)