-
Notifications
You must be signed in to change notification settings - Fork 665
Scheduler - Appointment Form - Fix KBN issues #32225
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
Conversation
| private createRecurrenceRuleGroup(): GroupItem { | ||
| // Change of frequency editor's value causes rerender of the recurrencePatternGroup. | ||
| // To prevent focus loss in this editor, we use this flag. | ||
| let needRestoreFrequencyEditorFocus = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason of focus loss is because recurrencePatternGroup group gets rerendered when updateDayEditorsVisibility() is called.
Another solution is to wrap weekGroup, monthGroup and yearGroup into different group here.
I have decided to use this flag, because it's used in very narrow scope and because form items already have very deep nesting (docs).
But if you have any objections, let's discuss it :)
e2e/testcafe-devextreme/tests/scheduler/common/appointmentForm/recurrence-form.visual.ts
Outdated
Show resolved
Hide resolved
e2e/testcafe-devextreme/tests/scheduler/common/appointmentForm/recurrence-form.visual.ts
Outdated
Show resolved
Hide resolved
e2e/testcafe-devextreme/tests/scheduler/common/appointmentForm/recurrence-form.visual.ts
Outdated
Show resolved
Hide resolved
e2e/testcafe-devextreme/tests/scheduler/common/appointmentForm/recurrence-form.ts
Outdated
Show resolved
Hide resolved
e2e/testcafe-devextreme/tests/scheduler/common/appointmentForm/form.functional.ts
Show resolved
Hide resolved
packages/devextreme-scss/scss/widgets/fluent/scheduler/_index.scss
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes keyboard navigation (KBN) issues in the Scheduler's appointment form, specifically focusing on proper focus management when switching between the main form and recurrence settings form.
Changes:
- Replaced
tabindex="-1"with theinertHTML attribute for better accessibility when hiding form groups - Added automatic focus management when switching between form groups
- Fixed focus loss issue in the frequency editor when its value changes
- Added visual focus state styling for the recurrence settings button in Fluent theme
Reviewed changes
Copilot reviewed 7 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/testcafe-models/scheduler/appointment/popup.ts | Added selectors for back button, recurrence start date input, and frequency editor to support new functional tests |
| packages/devextreme/js/__internal/scheduler/appointment_popup/m_recurrence_form.ts | Added CSS class to recurrence start date editor and implemented focus restoration logic for frequency editor to prevent focus loss during value changes |
| packages/devextreme/js/__internal/scheduler/appointment_popup/m_form.ts | Replaced tabindex attribute with inert attribute for form group visibility, added focusFirstFocusableInGroup method, and reordered toolbar updates to occur before focus management |
| packages/devextreme/js/__internal/scheduler/appointment_popup/appointment_popup.test.ts | Updated existing tests to check for inert attribute instead of tabindex, and added new tests to verify frequency editor focus behavior |
| packages/devextreme-scss/scss/widgets/fluent/scheduler/_index.scss | Added focus state styling with outline for recurrence settings button in Fluent theme |
| e2e/testcafe-devextreme/tests/scheduler/common/appointmentForm/form.visual.ts | Added visual regression test for recurrence settings button focus state |
| e2e/testcafe-devextreme/tests/scheduler/common/appointmentForm/form.functional.ts | Added functional tests to verify focus behavior when navigating between main and recurrence forms |
| frequencyEditorInputElement.click(); | ||
| jest.useFakeTimers(); | ||
| keydown(frequencyEditorInputElement, 'ArrowDown'); | ||
| jest.runAllTimers(); |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test uses jest.useFakeTimers() but doesn't restore real timers before completing. While the afterEach hook does restore real timers globally, it's better practice to restore timers explicitly at the end of the test to avoid potential test isolation issues. Consider adding jest.useRealTimers() after line 1720 or wrapping the fake timer usage in a try-finally block.
| jest.runAllTimers(); | |
| jest.runAllTimers(); | |
| jest.useRealTimers(); |
No description provided.