[Draft] Introduce KeyInterceptorService withs sync/async support to replace KeyInterceptor #9003

ScarletKuro · 2024-05-18T15:56:57Z

Description

Fixes: #8992
Fises: #8583

NB! The old service is not removed, just slightly refactored. I don't think we should remove it, just obsolete.

The API is quite flexible, thats how you can use it:

// The HandKeyDown / HandleKeyUp can be void or Task

// Only one handle (down)
await KeyInterceptorService.SubscribeAsync(_elementId, keyInterceptorOptions, keyDown: HandleKeyDown);

// Only one handle (up)
await KeyInterceptorService.SubscribeAsync(_elementId, keyInterceptorOptions, keyUp: HandleKeyUp);

// Two handlers at same time (down and up)
await KeyInterceptorService.SubscribeAsync(_elementId, keyInterceptorOptions, keyDown: HandleKeyDown, keyUp: HandleKeyUp); 

// Alternative with KeyObserver utility. Everything internally is using the KeyObserver wrapper
// The reason for this to be exposed is that, unlike the above overloads, it allows you to make the first handler a Task and other void (and vice-versa).
// With the above ones, if you set one handler as Task, then all of them have to be Task, and the same applies to void.
await KeyInterceptorService.SubscribeAsync(_elementId, keyInterceptorOptions, KeyObserver.KeyDown(HandleKeyDown), KeyObserver.KeyUpIgnore());

// Implementing IKeyInterceptorObserver directly on a component
await KeyInterceptorService.SubscribeAsync(this, keyInterceptorOptions);

View MudDialogInstance.razor.cs as example:

MudBlazor/src/MudBlazor/Components/Dialog/MudDialogInstance.razor.cs

Lines 83 to 86 in ba830b1

    
           var keyInterceptorOptions = KeyInterceptorOptions.Create( 
        
               targetClass: "mud-dialog", 
        
               keys: KeyOptions.Of(key: "Escape", subscribeDown: true)); 
        
           await KeyInterceptorService.SubscribeAsync(_elementId, keyInterceptorOptions, keyDown: HandleKeyDown);

How Has This Been Tested?

Not yet, only tested on dialog component that the concept is working

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.

codecov · 2024-05-18T16:37:34Z

Codecov Report

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

Project coverage is 90.34%. Comparing base (28bc599) to head (b1acd85).
Report is 212 commits behind head on dev.

Files	Patch %	Lines
...c/MudBlazor/Services/KeyInterceptor/KeyObserver.cs	45.83%	11 Missing and 2 partials ⚠️
...r/Services/KeyInterceptor/KeyInterceptorService.cs	65.38%	9 Missing ⚠️
...udBlazor/Services/KeyInterceptor/KeyInterceptor.cs	64.70%	5 Missing and 1 partial ⚠️
...Blazor/Services/KeyInterceptor/IKeyDownObserver.cs	0.00%	1 Missing ⚠️
...udBlazor/Services/KeyInterceptor/IKeyUpObserver.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #9003      +/-   ##
==========================================
+ Coverage   89.82%   90.34%   +0.51%     
==========================================
  Files         412      401      -11     
  Lines       11878    12206     +328     
  Branches     2364     2376      +12     
==========================================
+ Hits        10670    11028     +358     
+ Misses        681      643      -38     
- Partials      527      535       +8

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

ScarletKuro · 2024-05-18T23:21:49Z

@danielchalmers
is this KeyInterceptorOptions syntax fine for you?

var keyInterceptorOptions = KeyInterceptorOptions.Create(
	targetClass: "mud-switch-base",
	keys: new[]
	{
		// prevent scrolling page, instead increment
		KeyOptions.Of(key: "ArrowUp", preventDown: "key+none"),
		// prevent scrolling page, instead decrement
		KeyOptions.Of(key: "ArrowDown", preventDown: "key+none"),
		KeyOptions.Of(key: " ", preventDown: "key+none", preventUp: "key+none"),
	});
await KeyInterceptorService.SubscribeAsync(_elementId, keyInterceptorOptions, keyDown: HandleKeyDown);

We can't add constructor to KeyInterceptorOptions \ KeyOptions as it would be a breaking change with existing code, gotta use static method.

danielchalmers · 2024-05-18T23:51:15Z

@ScarletKuro Could we have two constructors?

public KeyOptions()
{
}

public KeyOptions(string? key = null,
    bool subscribeDown = false,
    bool subscribeUp = false,
    string preventDown = "none",
    string preventUp = "none",
    string stopDown = "none",
    string stopUp = "none")
{
    Key = key;
    PreventDown = preventDown;
    PreventUp = preventUp;
    SubscribeDown = subscribeDown;
    SubscribeUp = subscribeUp;
    StopDown = stopDown;
    StopUp = stopUp;
}

public async Task SubscribeAsync(string elementId, string targetClass, bool EnableLogging, params KeyOptions[] keyOptions);

// Constructor could match this syntax.
await SubscribeAsync("my-id", "mud-switch-base",
    enableLogging: false,
    new("ArrowUp", preventDown: "key+none"),
    new("ArrowDown", preventDown: "key+none"),
    new(" ", preventDown: "key+none", preventUp: "key+none"));

ScarletKuro · 2024-05-19T00:19:28Z

public async Task SubscribeAsync(string elementId, string targetClass, bool EnableLogging, params KeyOptions[] keyOptions);

// Constructor could match this syntax.
await SubscribeAsync("my-id", "mud-switch-base",
    enableLogging: false,
    new("ArrowUp", preventDown: "key+none"),
    new("ArrowDown", preventDown: "key+none"),
    new(" ", preventDown: "key+none", preventUp: "key+none"));

I don't think this is a good idea in SubscribeAsync. First of all, you are missing handlers for KeyDown and KeyUp. We would also need to add this to the other overloads, resulting in a total of three new overloads. It would also look too long in my opinion:

await SubscribeAsync("my-id",
    keyDown: KeyDownHandle,
    keyUp: null, // We'd want to have them as the 2nd and 3rd parameters since we wouldn't want to have it between KeyInterceptorOptions entities
    "mud-switch-base",
    enableLogging: false,
    new("ArrowUp", preventDown: "key+none"),
    new("ArrowDown", preventDown: "key+none"),
    new(" ", preventDown: "key+none", preventUp: "key+none"));

You wouldn't be able to avoid having keyUp: null if you want to use params KeyOptions[] without keys: new[] {...}. This doesn't seem as readable to me as it does now.
And it's not breaking change proof
But I guess we can add the two constructors to KeyOptions with one being empty.

danielchalmers · 2024-05-19T00:23:43Z

@ScarletKuro Maybe a fluent syntax with method calls if we wanted to get really fancy.

MudRating - Add `role="radiogroup"` - Format razor MudRatingItem - Add `role="radio"` MudToggleGroup - Add `role="group"` - Format razor - still needs proper keyboard support (Waiting for MudBlazor#9003) MudToggleItem - Add `role="checkbox"`

github-actions bot added enhancement New feature or request PR: needs review labels May 18, 2024

ScarletKuro mentioned this pull request May 18, 2024

KeyInterceptor: Simplify setup #8992

Open

2 tasks

ScarletKuro marked this pull request as draft May 18, 2024 16:07

Introduce KeyInterceptorService

ec5cc36

ScarletKuro force-pushed the key_interceptor branch from 23eef77 to ec5cc36 Compare May 18, 2024 22:49

ScarletKuro added 2 commits May 19, 2024 01:52

Fix formatting

3de8f73

Improve

ba830b1

move picker to KeyInterceptorService

b1acd85

danielchalmers mentioned this pull request May 25, 2024

Rating, ToggleGroup: Improve accessibility #9058

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Draft] Introduce KeyInterceptorService withs sync/async support to replace KeyInterceptor #9003

[Draft] Introduce KeyInterceptorService withs sync/async support to replace KeyInterceptor #9003

ScarletKuro commented May 18, 2024 •

edited

codecov bot commented May 18, 2024 •

edited

ScarletKuro commented May 18, 2024

danielchalmers commented May 18, 2024

ScarletKuro commented May 19, 2024 •

edited

danielchalmers commented May 19, 2024

	var keyInterceptorOptions = KeyInterceptorOptions.Create(
	targetClass: "mud-dialog",
	keys: KeyOptions.Of(key: "Escape", subscribeDown: true));
	await KeyInterceptorService.SubscribeAsync(_elementId, keyInterceptorOptions, keyDown: HandleKeyDown);

[Draft] Introduce KeyInterceptorService withs sync/async support to replace KeyInterceptor #9003

Are you sure you want to change the base?

[Draft] Introduce KeyInterceptorService withs sync/async support to replace KeyInterceptor #9003

Conversation

ScarletKuro commented May 18, 2024 • edited

Description

How Has This Been Tested?

Type of Changes

Checklist

codecov bot commented May 18, 2024 • edited

Codecov Report

ScarletKuro commented May 18, 2024

danielchalmers commented May 18, 2024

ScarletKuro commented May 19, 2024 • edited

danielchalmers commented May 19, 2024

ScarletKuro commented May 18, 2024 •

edited

codecov bot commented May 18, 2024 •

edited

ScarletKuro commented May 19, 2024 •

edited