Autocomplete: Improve menu behavior

danielchalmers commented 3 weeks ago

Description

Follows up on #8758: New API, Refactor, Prevent race conditions, Fix menu flashing, Update tests. This is a must-have for v7-preview1 because of the menu flashing bug in the docs on Firefox.

Goal

To make the Autocomplete's menu more stable, extensible, and predictable.

Improvements to API & Behaviour

Replaces ChangeMenuAsync with clearly defined public OpenMenuAsync and CloseMenuAsync
Adds classes to input adornment icon and clear button for simpler targeting
Renames SelectOnClick to SelectOnActivation for clarity
Modifies KeyUp/KeyDown handlers to have protected access instead of internal
Enables holding arrow keys to navigate between menu items (KeyDown instead of KeyUp)

Resolved Issues

Prevented multiple menu openings on repeated clicks during search
Fixed menu flashing issue in Firefox caused by onclick and onfocus conflict
Resolves #6961

How Has This Been Tested?

unit

Type of Changes

[X] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Documentation (fix or improvement to the website or code docs)

Checklist

[X] The PR is submitted to the correct branch (dev).
[X] My code follows the code style of this project.
[X] I've added relevant tests.

danielchalmers commented 3 weeks ago

Fixed

Glitchy open on firefox

bug3

danielchalmers commented 3 weeks ago

@ScarletKuro Could you try your scenarios with this PR ~~and help me with the remaining tests~~?

danielchalmers commented 3 weeks ago

@henon @ScarletKuro Why does the SearchFunc throw an exception here https://github.com/MudBlazor/MudBlazor/blob/bdc937f4eef34b2f13b387605429ea3606efaf6c/src/MudBlazor/Components/Autocomplete/MudAutocomplete.razor.cs#L660 with this PR on the Strict Mode example on the docs?

henon commented 3 weeks ago

Please set a breakpoint and find out what the exception and the stacktrace are and post it here.

danielchalmers commented 3 weeks ago

@henon

''<' is an invalid start of a value. Path: $ | LineNumber: 0 | BytePositionInLine: 0.'

' at System.Text.Json.ThrowHelper.ReThrowWithPath(ReadStack& state, JsonReaderException ex)\n at System.Text.Json.Serialization.JsonConverter`1[[System.Collections.Generic.List`1[[MudBlazor.Examples.Data.Models.Element, MudBlazor.Examples.Data, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]], System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)\n at System.Text.Jso…xample.Search(String value, CancellationToken token) in C:\Users\danie\source\repos\MudBlazor\src\MudBlazor.Docs\Pages\Components\Autocomplete\Examples\AutocompleteStrictFalseExample.razor:line 25\n at MudBlazor.MudAutocomplete`1.d__188[[MudBlazor.Examples.Data.Models.Element, MudBlazor.Examples.Data, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].MoveNext() in C:\Users\danie\source\repos\MudBlazor\src\MudBlazor\Components\Autocomplete\MudAutocomplete.razor.cs:line 649'

ScarletKuro commented 3 weeks ago

@henon @ScarletKuro Why does the SearchFunc throw an exception here

Sorry, but I really don't know. I have no codebase knowledge on how this component functions at slightest. @henon @mckaragoz has more experience with this component. It's also hard for me to adequately review this when I don't know the nuances and you changed many places. I'd say this component code resembles delicate paper, susceptible to crumbling with the slightest disturbance :flushed: Because it has many race conditions and my async void -> Task change is a proof of this (nvm, it was MudSelect but they have the same problems). I appropriate your effort on trying to fix it tho, but almost feels like it would be better to make a parallel component called "MudAutocompleteX" and do minimum at least for the mudblazor docs search, battle test it there and then extend the functionality.

danielchalmers commented 3 weeks ago

@ScarletKuro I think I've definitely hardened the code and made it less brittle and susceptible to race conditions. It does need some heavy testing though. I would hate to discard these changes from the BCL because it has a lot of improvements for all users.

henon commented 3 weeks ago

The exception means that the json coming from the web api is invalid. For whatever reason. I can't see why your changes would cause that

henon commented 3 weeks ago

Maybe you can find out what the api call returns that can't be parsed?

Yomodo commented 3 weeks ago

Maybe you can find out what the api call returns that can't be parsed?

Probably the result is returned as plain text (server error) while json is expected.

danielchalmers commented 3 weeks ago

@henon @Yomodo Seeing it with this call:

await SearchFunc(null, _cancellationTokenSrc.Token);

In AutocompleteStrictFalseExample.razor:

    private async Task<IEnumerable<Element>> Search(string value, CancellationToken token)
    {
        return await httpClient.GetFromJsonAsync<List<Element>>($"webapi/periodictable/{value}", token);
    }

and AutocompleteStrictFalseSelectedHighlight.razor:

    public async Task<IEnumerable<string>> Search(string text, CancellationToken token)
    {
        await Task.Delay(100, token);

        if (string.IsNullOrWhiteSpace(text))
        {
            return Fruits;
        }

        return Fruits.Where(x => x.Contains(text, StringComparison.InvariantCulture));
    }

Can you see any reason it would stop working?

henon commented 3 weeks ago

Does it even work without your changes?

danielchalmers commented 3 weeks ago

Does it even work without your changes?

@henon Works on https://dev.mudblazor.com/components/autocomplete#strict-mode

jperson2000 commented 3 weeks ago

@henon

''<' is an invalid start of a value. Path: $ | LineNumber: 0 | BytePositionInLine: 0.' ' at System.Text.Json.ThrowHelper.ReThrowWithPath(ReadStack& state, JsonReaderException ex)\n at System.Text.Json.Serialization.JsonConverter1[[System.Collections.Generic.List1[[MudBlazor.Examples.Data.Models.Element, MudBlazor.Examples.Data, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]], System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)\n at System.Text.Jso…xample.Search(String value, CancellationToken token) in C:\Users\danie\source\repos\MudBlazor\src\MudBlazor.Docs\Pages\Components\Autocomplete\Examples\AutocompleteStrictFalseExample.razor:line 25\n at MudBlazor.MudAutocomplete`1.d__188[[MudBlazor.Examples.Data.Models.Element, MudBlazor.Examples.Data, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].MoveNext() in C:\Users\danie\source\repos\MudBlazor\src\MudBlazor\Components\Autocomplete\MudAutocomplete.razor.cs:line 649'

@danielchalmers I've seen this error before when I expect JSON but get HTML, such as when I forget to authenticate an HttpClient request. If we can get the raw response from the HttpClient, then we can look at the actual response content and dig deeper. Here's some temporary code which should do the trick:

  private async Task<IEnumerable<Element>> Search(string value, CancellationToken token)
    {
        // return await httpClient.GetFromJsonAsync<List<Element>>($"webapi/periodictable/{value}", token);
        var response = await httpClient.GetAsync($"webapi/periodictable/{value}", token);
        var content = await response.Content.ReadAsStringAsync();
        Debugger.Break(); // Examine the content variable here for issues, maybe an auth error?
        return JsonSerializer.Deserialize<List<Element>>(content);
    }

danielchalmers commented 3 weeks ago

@jperson2000 Thanks for the great tip, it made me discover that the webapi returns 404, so I guess it's not available while debugging WASM and was never a regression @henon

The rest was just some weird logic that should be fixed and ready to review now.

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 90.12%. Comparing base (28bc599) to head (6451e72). Report is 117 commits behind head on dev.

Files	Patch %	Lines
...r/Components/Autocomplete/MudAutocomplete.razor.cs	80.00%	2 Missing and 1 partial :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## dev #8787 +/- ## ========================================== + Coverage 89.82% 90.12% +0.29% ========================================== Files 412 421 +9 Lines 11878 12213 +335 Branches 2364 2410 +46 ========================================== + Hits 10670 11007 +337 + Misses 681 664 -17 - Partials 527 542 +15 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

henon commented 3 weeks ago

Does it even work without your changes?

@henon Works on https://dev.mudblazor.com/components/autocomplete#strict-mode

Yes, but try to execute it locally instead from dev.mudblazor.ocm. It may well be the cause of the error.

danielchalmers commented 3 weeks ago

@henon Should have pinged you above but I got it all working except the example because the webapi returns 404 in debug mode which is not a regression. PR should be ready to review

ScarletKuro commented 3 weeks ago

Thanks for the great tip, it made me discover that the webapi returns 404

Are you starting docs as wasm standalone? There is WasmHost then the API should be available.

henon commented 3 weeks ago

The Strict parameter and example troubles me. I can't think of a use-case where you want the Strict="true" behavior because it will show an empty dropdown when clicking the autocomplete which is just nonsense. I remember that this behavior was the default in the beginning. We added the parameter to avoid a breaking change because users wanted the Strict="false" behavior instead.

Unless somebody can think of a valid use-case for Strict="true" I would now consider taking the opportunity and making the breaking change of just removing this parameter.

mikes-gh commented 3 weeks ago

Thanks for the great tip, it made me discover that the webapi returns 404

Are you starting docs as wasm standalone? There is WasmHost then the API should be available.

Yes this. wasm standalone doesnt have any mvc controllers to provide the endpoints for the examples data.

henon commented 3 weeks ago

We want to retain the Strict==false behavior and remove the Strict==true behavior.

danielchalmers commented 2 weeks ago

@Mr-Technician @ScarletKuro Looks good?

EDIT: Wait for Mr-Technician before merging

MudBlazor / MudBlazor