marmelab / react-admin

A frontend Framework for single-page applications on top of REST/GraphQL APIs, using TypeScript, React and Material Design
http://marmelab.com/react-admin
MIT License
24.89k stars 5.23k forks source link

useInput defaults to non-unique IDs #9534

Closed david-bezero closed 9 months ago

david-bezero commented 9 months ago

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

david-bezero commented 9 months ago

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

slax57 commented 9 months ago

Thanks for the report, reproduced!

slax57 commented 9 months ago

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

                <TextInput source="title" />
                <TextInput source="title" id="title2" />
arimet commented 9 months ago

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.

david-bezero commented 9 months ago

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 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

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

slax57 commented 8 months ago

@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 commented 5 months ago

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

fzaninotto commented 5 months ago

all right, I'm convinced, go for useId

fzaninotto commented 5 months ago

Fixed by #9788