freeCodeCamp icon indicating copy to clipboard operation
freeCodeCamp copied to clipboard

[UI Components] Usage of FormGroup's `controlId` resulting duplicate ID

Open huyenltnguyen opened this issue 1 year ago • 5 comments

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

  1. Go to https://www.freecodecamp.org/update-email/
  2. Check the DOM
Screenshot 2023-12-31 at 16 00 53

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/

huyenltnguyen avatar Dec 31 '23 09:12 huyenltnguyen

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 ?

dvip1 avatar Dec 31 '23 18:12 dvip1

i can see the test which checks for existence of id on the outer tag image which means - the id located for some purpose there

shootermv avatar Dec 31 '23 19:12 shootermv

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.

altsun avatar Jan 01 '24 02:01 altsun

@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.

huyenltnguyen avatar Jan 01 '24 06:01 huyenltnguyen

do we need to remove the id or change to different one?

Sahillather002 avatar Jan 01 '24 06:01 Sahillather002