freeCodeCamp
freeCodeCamp copied to clipboard
[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
input
element. - 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?