Skip to content
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

useInput defaults to non-unique IDs #9534

Closed
david-bezero opened this issue Dec 19, 2023 · 12 comments
Closed

useInput defaults to non-unique IDs #9534

david-bezero opened this issue Dec 19, 2023 · 12 comments

Comments

@david-bezero
Copy link
Contributor

This is mostly relevant when using the enterprise package @react-admin/ra-form-layout

use-case: using a modal "create" form while on an "edit" page to create a sub-resource of the same type (but could also apply to different types which share property names)

What you were expecting:
All form elements (i.e. useInput) should default to having a unique ID which does not conflict with other forms (i.e. via React's useId hook)

What happened instead:
Form elements default to using the source as an ID, which is not unique if multiple forms are on the page.

Steps to reproduce:

const MyEdit = () => (
  <Edit resource="person">
    <SimpleForm>
      <TextInput name="name" source="name" label="Name" /> {/* id is implicitly 'name' */}
      <CreateInDialogButton label="Add child" resource="person">
        <SimpleForm>
          <TextInput name="name" source="name" label="Name" /> {/* id is also implicitly 'name' */}
        </SimpleForm>
      </CreateInDialogButton>
    </SimpleForm>
  </Edit>
);

The browser will complain about multiple fields sharing the same ID, and in some situations this will cause the fields to be impossible to select (e.g. playwright browser testing fails to find the fields)

It is possible to work around this by explicitly providing an id to all fields, but this is tedious, easy to forget, and difficult to diagnose failures. Changing the default to a unique ID would make it guaranteed to be correct without extra work for the developer.

Environment

  • React-admin version: 4.16.2
  • React version: 18.2.0
@david-bezero
Copy link
Contributor Author

the same issue applies when using filter inputs and BulkUpdateFormButton in the list page

@slax57 slax57 added the bug label Dec 20, 2023
@slax57
Copy link
Contributor

slax57 commented Dec 20, 2023

Thanks for the report, reproduced!

@slax57
Copy link
Contributor

slax57 commented Dec 20, 2023

Note that this can be worked around by manually specifying an id

                <TextInput source="title" />
                <TextInput source="title" id="title2" />

@arimet arimet added enhancement and removed bug labels Dec 20, 2023
@arimet
Copy link
Contributor

arimet commented Dec 20, 2023

We will support this ticket in react-admin v5.
As React-Admin 4 supports React 16 and useID only appears in React 18, we advise you to use the workaround in the meantime.

@arimet arimet closed this as completed Dec 20, 2023
@david-bezero
Copy link
Contributor Author

a polyfill if you want to support React 16 could be (for example):

let globalCounter = 0;

function generateUniqueId() {
  const id = globalCounter;
  globalCounter++;
  return `:${id}:`;
}

const useId = () => useState(generateUniqueId)[0];

@fzaninotto
Copy link
Member

The problem with this change is that it makes the id of each form input impossible to guess. This isn't ideal for rehydrating server-side rendered content, unit tests, and CSS rules based on ids. I think that using useId isn't a good solution.

Besides, it's a big breaking change: any app relying on input ids for styles or imperative JS will break.

@david-bezero
Copy link
Contributor Author

@fzaninotto React's built-in useId is built to handle hydration correctly, though that would be an issue with the polyfil I posted (I'm not sure how often react-admin gets used with SSR though?)

For the other concerns: using IDs in those ways is not recommended anyway (and has been considered an anti-pattern for over a decade). Prefer using data-testid for tests, classes for styles, and React's ref for code references. This isn't unique to react-admin, but applies to all frontend/react development.

The bigger issue is that IDs must be unique within a page, which currently isn't the case in a number of situations when they're being set based on other data (as noted in my original post). Having two elements with the same ID can cause all sorts of issues, some more noticeable than others, and just because it works for you in one browser does not mean it will work in another, or when using accessibility tools, or when using various browser plugins, or even in a different version of the same browser. It's a very risky position to be in!

@fzaninotto
Copy link
Member

I don't object fixing the issue, I object using useId for that.

@slax57
Copy link
Contributor

slax57 commented Feb 6, 2024

@fzaninotto FTR, I'm rather with @david-bezero on this one.
Unless this causes an issue with SSR, I think useId would be a good solution. I too consider that tests and CSS rules should not be based on the id, and IMHO the uniqueness of the id is more important than the ability to guess it (if we can't have both).
Lastly, since we can only use useId with React 18, this would need to be kept for the v5 in any case, so it's a good time to introduce a minor breaking change.

@djhi
Copy link
Collaborator

djhi commented Apr 29, 2024

Besides, useId has been introduced to actually fix SSR issues. It's meant for that

@fzaninotto
Copy link
Member

all right, I'm convinced, go for useId

@fzaninotto
Copy link
Member

Fixed by #9788

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants