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

Autocomplete: Improve menu behavior #8787

Merged

Conversation

danielchalmers
Copy link
Contributor

@danielchalmers danielchalmers commented Apr 23, 2024

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

How Has This Been Tested?

unit

Type of Changes

  • 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

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

@github-actions github-actions bot added the bug Something does not work as intended/expected label Apr 23, 2024
@danielchalmers
Copy link
Contributor Author

danielchalmers commented Apr 23, 2024

Fixed

Glitchy open on firefox

bug3

Multiple opens during long search

loadasync.mp4

Menu re-opening on item select

video2.mp4

Menu opens after clicking away

video2.mp4

@danielchalmers
Copy link
Contributor Author

danielchalmers commented Apr 23, 2024

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

@danielchalmers danielchalmers changed the title Autocomplete: Prevent race conditions Autocomplete: Fix menu opening behavior Apr 23, 2024
@danielchalmers danielchalmers changed the title Autocomplete: Fix menu opening behavior Autocomplete: Improve menu opening behavior Apr 23, 2024
@danielchalmers danielchalmers changed the title Autocomplete: Improve menu opening behavior Autocomplete: Improve menu behavior Apr 23, 2024
@danielchalmers
Copy link
Contributor Author

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

Logger.LogWarning("The search function failed to return results: " + e.Message);
with this PR on the Strict Mode example on the docs?

@henon
Copy link
Collaborator

henon commented Apr 23, 2024

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

@danielchalmers
Copy link
Contributor Author

@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
Copy link
Member

ScarletKuro commented Apr 23, 2024

@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 😳 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
Copy link
Contributor Author

@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
Copy link
Collaborator

henon commented Apr 23, 2024

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
Copy link
Collaborator

henon commented Apr 23, 2024

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

@Yomodo
Copy link
Contributor

Yomodo commented Apr 23, 2024

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
Copy link
Contributor Author

danielchalmers commented Apr 23, 2024

@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
Copy link
Collaborator

henon commented Apr 23, 2024

Does it even work without your changes?

@danielchalmers
Copy link
Contributor Author

danielchalmers commented Apr 23, 2024

Does it even work without your changes?

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

image

@jperson2000
Copy link
Sponsor Contributor

@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 danielchalmers marked this pull request as ready for review April 24, 2024 00:42
Copy link

codecov bot commented Apr 24, 2024

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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@henon
Copy link
Collaborator

henon commented Apr 24, 2024

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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Collaborator

henon commented Apr 24, 2024

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
Copy link
Contributor

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.

@danielchalmers danielchalmers changed the title Autocomplete: Improve menu behavior Autocomplete: Improve menu behavior, Remove Strict mode Apr 24, 2024
@danielchalmers danielchalmers changed the title Autocomplete: Improve menu behavior, Remove Strict mode Autocomplete: Improve menu behavior, Remove non-strict mode Apr 24, 2024
@henon
Copy link
Collaborator

henon commented Apr 24, 2024

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

@henon henon changed the title Autocomplete: Improve menu behavior, Remove non-strict mode Autocomplete: Improve menu behavior, remove Strict mode Apr 24, 2024
@danielchalmers danielchalmers changed the title Autocomplete: Improve menu behavior, remove Strict mode Autocomplete: Improve menu behavior Apr 24, 2024
Copy link
Collaborator

@henon henon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good. I don't see any red flags. Of course, you can never know with an unstable component like this which was in a delicate balance before. Let's merge and hope for the best. If this introduces issues we can always revert.

not typical behavior and you can just use tab now

kind of stops you from closing dialogs with double escape though
@danielchalmers
Copy link
Contributor Author

danielchalmers commented Apr 25, 2024

@Mr-Technician @ScarletKuro Looks good?

EDIT: Wait for Mr-Technician before merging

@Mr-Technician Mr-Technician merged commit a61421b into MudBlazor:dev Apr 27, 2024
3 of 4 checks passed
@danielchalmers danielchalmers deleted the autocomplete-race-condition branch April 27, 2024 20:52
@henon henon mentioned this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autocomplete - Drop down opens even though it no longer has focus
7 participants