MudDatePicker: Fix keyboard navigation with fixed year/month#12180
MudDatePicker: Fix keyboard navigation with fixed year/month#12180danielchalmers merged 18 commits intoMudBlazor:devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes keyboard navigation issues in MudDatePicker when FixYear or FixMonth parameters are set, ensuring these constraints are respected during keyboard interactions. It also adds comprehensive accessibility documentation for keyboard shortcuts.
Key changes:
- Added guard checks in navigation methods (MoveToNextYear, MoveToPreviousYear, MoveToNextMonth, MoveToPreviousMonth) to prevent changes when year/month are fixed
- Refactored duplicate month navigation logic by adding parameters to existing methods
- Added test coverage for FixYear and FixMonth keyboard navigation scenarios
- Documented all keyboard shortcuts in an accessibility section
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| MudDatePicker.cs | Added FixYear/FixMonth guards to navigation methods and refactored month navigation to accept step parameter |
| DatePickerTests.cs | Added tests verifying keyboard navigation respects FixYear and FixMonth constraints |
| DatePickerPage.razor | Added accessibility section documenting keyboard shortcuts (with some documentation errors) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <td>Increment month/date in respective view</td> | ||
| </tr> | ||
| <tr> | ||
| <td> | ||
| <CodeInline>ArrowRight</CodeInline> | ||
| </td> | ||
| <td>Decrement month/date in respective view</td> |
There was a problem hiding this comment.
The descriptions for ArrowLeft and ArrowRight are swapped. Based on the code:
- ArrowLeft decrements (moves to previous day/month)
- ArrowRight increments (moves to next day/month)
The descriptions should be:
- ArrowLeft: "Decrement month/date in respective view"
- ArrowRight: "Increment month/date in respective view"
| <td>Increment month/date in respective view</td> | |
| </tr> | |
| <tr> | |
| <td> | |
| <CodeInline>ArrowRight</CodeInline> | |
| </td> | |
| <td>Decrement month/date in respective view</td> | |
| <td>Decrement month/date in respective view</td> | |
| </tr> | |
| <tr> | |
| <td> | |
| <CodeInline>ArrowRight</CodeInline> | |
| </td> | |
| <td>Increment month/date in respective view</td> |
| <td>Increment month in date view</td> | ||
| </tr> | ||
| <tr> | ||
| <td> | ||
| <CodeInline>Shift+ArrowRight</CodeInline> | ||
| </td> | ||
| <td>Decrement month in date view</td> |
There was a problem hiding this comment.
The descriptions for Shift+ArrowLeft and Shift+ArrowRight are swapped. Based on the code:
- Shift+ArrowLeft moves to previous month
- Shift+ArrowRight moves to next month
The descriptions should be:
- Shift+ArrowLeft: "Decrement month in date view"
- Shift+ArrowRight: "Increment month in date view"
| <td>Increment month in date view</td> | |
| </tr> | |
| <tr> | |
| <td> | |
| <CodeInline>Shift+ArrowRight</CodeInline> | |
| </td> | |
| <td>Decrement month in date view</td> | |
| <td>Decrement month in date view</td> | |
| </tr> | |
| <tr> | |
| <td> | |
| <CodeInline>Shift+ArrowRight</CodeInline> | |
| </td> | |
| <td>Increment month in date view</td> |
|
|
||
| private void MoveToPreviousMonth() | ||
| private void MoveToPreviousMonth(int numberOfMonths = 1) | ||
|
|
There was a problem hiding this comment.
Remove the extra blank line between the method signature and the opening brace for consistency with the rest of the codebase.
| var datePicker = datePickerComponent.Instance; | ||
| await comp.InvokeAsync(() => datePicker.OnHandleKeyDownAsync(new KeyboardEventArgs() { Key = "Enter", Type = "keydown", })); | ||
| comp.WaitForAssertion(() => comp.FindAll("div.mud-picker-open").Count.Should().Be(1)); | ||
| //try to select month outside fixed year in date view |
There was a problem hiding this comment.
The comment mentions "fixed year" but the test is for FixMonth. The comment should say "try to select month outside fixed month in date view" to accurately describe what the test is checking.
| //try to select month outside fixed year in date view | |
| //try to select month outside fixed month in date view |
|
P.S. not to merge before #12177 |
|
Hi @spingee, could you take another look with Copilot's feedback and merge conflicts? |
52d021c to
05876a6
Compare
|
@danielchalmers Hello, done |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/MudBlazor.Docs/Pages/Components/DatePicker/DatePickerPage.razor
Outdated
Show resolved
Hide resolved
…razor Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
src/MudBlazor.Docs/Pages/Components/DatePicker/DatePickerPage.razor
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
So looking at these tests they execute (Act) repeatedly but we only check at the end. Also we should align the tests to act on the DOM instead of just executing the method. like the following. You will see I used the Id for easy targetting and since the PopoverProvider is included inside the test razor I don't have to render it as a component as well. You should be able to inputElement.KeyDownAsync just like you was before and verify what it's supposed to do. And what happens when enter is pressed etc. You need to be able to verify via the dom so we protect the logic, not just that one method if fired probably works. Lastly if you want to read a parameter on a comp.Instance it is best to read it from the Dom. If it's parameter state and you must read it then use comp.Instance.GetState(x => x.parameter)
var startDate = new DateTime(2022, 12, 31, new CultureInfo("en-US").Calendar);
var comp = Context.Render<SimpleMudDatePickerTest>(parameters => parameters
.Add(x => x.Date, startDate)
.Add(x => x.OpenTo, OpenTo.Month)
.Add(x => x.InputId, "datePickerTest_FixYear_Id")
.Add(x => x.FixYear, 2022));
var inputElement = comp.WaitForElement("#datePickerTest_FixYear_Id");
// activate picker popover
await inputElement.KeyDownAsync(new KeyboardEventArgs() { Key = "Enter", Type = "keydown" });
var pop = comp.WaitForElement("div.mud-picker-popover");
comp.WaitForAssertion(() => pop.ClassList.Should().Contain("mud-popover-open"));
// starts with nothing selected
comp.FindAll("button.mud-picker-calendar-day.mud-selected").Count.Should().Be(0);
//try to select year outside fixed year in month view
There was a problem hiding this comment.
@versile2 Hello this approach doesnt work, sending keydown directly on input, OnKeydownHandler does not get called.
It maybe because event bubbling is perhaps not supported in bunit or involment of keyinterceptor
There was a problem hiding this comment.
Push your changes and I'll check it
There was a problem hiding this comment.
Sorry for the delay. Yes this binding is pushed by KeyInterceptor and cannot be tested that way, revert your test to the way you had it. Best we can get.
…razor Co-authored-by: Versile Johnson II <148913404+versile2@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@spingee The two comments from copilot about the math, I looked at it very briefly, Thoughts? |
|
Need to address the failing test (versile said to revert the testing) and fix or comment the copilot concerns. Then this can be merged, since overall it looks good. |
I will look at it tomorrow |
|
@versile2 resolved copilot reviews, also reverted commit with temporary dom level keydown failing test, as there was no response from you |
|
Thank you! |
Hello,
This is followup for this PR #12028 , it was merged too fast :).
I fixed issue when it was able to move weird state with fixed year/month with keyboard nav.
Also updated docs with info about keyboard shortcuts