[UI Components] Usage of FormGroup's `controlId` resulting duplicate ID
Describe the bug
The FormGroup component in the ui-components library has a controlId prop, which helps associate a form control with a label.
We expect that the following code
<FormGroup controlId='emailInput'>
<ControlLabel>Email</ControlLabel>
<FormControl type='email' />
</FormGroup>
would result
<div>
<label for="emailInput">Email</label>
<input id="emailInput" type='email' />
</div>
However, the result is actually:
<div id="emailInput">
<label for="emailInput">Email</label>
<input id="emailInput" type='email' />
</div>
This is invalid HTML, and messes up the label-input association, which I think is the cause of #52723.
Steps to reproduce
- Go to https://www.freecodecamp.org/update-email/
- Check the DOM
Notes:
- The issue for this particular page will be resolved once #52507 is merged, but it can be reproduced locally with this version of
main: https://github.com/freeCodeCamp/freeCodeCamp/blob/f8e7ff01dd66b78e202c8488cd79205de20e3cd6/client/src/pages/update-email.tsx. - I'm pretty sure we can see the issue in Storybook as well, but I discovered the issue when I was testing the client app.
Expected behavior
- The id should only be applied to the
inputelement. - Tests need to be added to prevent this issue from happening again.
Additional context
This issue was discovered in #52507 as we were trying to replace getByTestId() with getByLabel(). With the getByLabel method, the tests failed to find the input as they were confused by the duplicate ID.
Reference
Bootstrap FormGroup: https://react-bootstrap-v3.netlify.app/components/forms/
I am new to open source, I want to work on this. I'll send update to this thread if I managed to solve it. Also is that how I'm to ask for a PR ?
i can see the test which checks for existence of id on the outer tag
which means - the id located for some purpose there
I think the root cause of this defect is:
update-email.tsx imports FormGroup from @freecodecamp/ui.
Recently, this PR #51204 updated tools/ui-components/src/index.ts to use customized FormGroup component.
Customized FormGroup component was created in this PR #46758, I think it assigns controlId to id of component.
@shootermv Honestly, I wasn't following the development of this component, so I'm not really familiar with it. However, its current behavior is causing the HTML to be invalid and resulting an accessibility issue, so even if this was intended, it's an incorrect behavior and needs to be changed.
Thanks everyone for the interest in contributing. I think I should handle this one to have things moved a bit faster.
do we need to remove the id or change to different one?