refactor(form): make errrormessageid required #1365
Conversation
BREAKING CHANGE: The property `SiFormItemControl.errormessageId` is now required. All implementations of `SiFormItemControl` must provide this property to ensure proper accessibility support for error messages.
Summary of ChangesHello @akashsonune, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a breaking change by making the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
akashsonune
added
the
breaking-changes
| * This property will be required with v48. | ||
| */ | ||
| readonly errormessageId?: string | Signal<string | undefined>; | ||
| readonly errormessageId: string | Signal<string | undefined>; |
There was a problem hiding this comment.
| readonly errormessageId: string | Signal<string | undefined>; | |
| readonly errormessageId: string | Signal<string>; |
I hope this works as well
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes the errormessageId property on the SiFormItemControl interface required, which is a breaking change. This is a good step towards improving accessibility for form error messages. The change is correctly implemented in SiFormlyFormFieldProviderDirective to satisfy the new interface requirement. However, I've noted an issue with the implementation in SiFormlyFormFieldProviderDirective where it doesn't fully adhere to the contract of SiFormItemControl regarding the assignment of aria-describedby, which is crucial for accessibility. Please see my detailed comment.
| readonly field = input.required<FormlyFieldConfig>(); | ||
|
|
||
| readonly id = computed(() => this.field().id); | ||
| readonly errormessageId = computed(() => `${this.field().id}-errormessage`); |
There was a problem hiding this comment.
While adding errormessageId is correct to satisfy the updated SiFormItemControl interface, this implementation doesn't fully adhere to the contract defined in the interface's documentation.
The documentation for SiFormItemControl states:
Implementations of {@link SiFormItemControl} must generate this ID and assign it to the
aria-describedbyattribute of their focusable elements.
This directive only generates the ID. It does not assign it to the aria-describedby attribute of the focusable formly field. This means the accessibility link between the field and its error message is not created for formly fields, which is a significant accessibility issue.
To resolve this, the errormessageId should be propagated to the underlying formly field component, which can then apply it to the aria-describedby attribute. A robust way to do this with signal-based inputs is to use an effect in this directive's constructor to update the field's properties:
import { effect } from '@angular/core';
// ...
constructor() {
effect(() => {
const field = this.field();
field.props ??= {};
field.props['aria-describedby'] = this.errormessageId();
});
}This would require adding a constructor to the directive. The underlying formly field components would also need to be updated to read this property and apply it.
|
|
|
Documentation. Coverage Reports: |
BREAKING CHANGE: The property
SiFormItemControl.errormessageIdis now required. All implementations ofSiFormItemControlmust provide this property to ensure proper accessibility support for error messages.