LaunchPadLab / lp-components

Our Components
http://lp-components.herokuapp.com
MIT License
5 stars 1 forks source link

Rendering a form multiple times in the same view results in ID clashes #447

Open chawes13 opened 3 years ago

chawes13 commented 3 years ago

Issue

For example, the "Name" input is read 6 times on this view: https://share.getcloudapp.com/KouwQ51p & https://share.getcloudapp.com/YEuyvYJA

Potential Enhancement

Workarounds

dpikt commented 3 years ago

@chawes13 this is an interesting problem. The solution you proposed seems like the best way to go. I had assumed that non-dynamic IDs were preferred over dynamic ones but it looks like that just isn't the case (https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id):

This attribute's value is an opaque string: this means that web authors should not rely on it to convey human-readable information (although having your IDs somewhat human-readable can be useful for code comprehension, e.g. consider ticket-18659 versus r45tgfe-freds&$@).

Given that info, my only concern would be making sure default styles worked with the new IDs. And this would probably be a breaking change, if an app has any styles relying on the current ID naming scheme.

For a short-term workaround, the route of adding unique IDs to a form shouldn't be too time-consuming if you use a consistent prefix:

function MyForm ({ form }) {
    const uniqueId = (inputName) => form + '-' + inputName
    ... 
    <Field name="firstName" id={ uniqueId('firstName')  } ... />
}
chawes13 commented 3 years ago

@dpikt What process should we use for including the fix for this issue in an upcoming major release? E.g., create an Asana task?

Unfortunately it is pretty time consuming. Every step in the survey renders multiple forms (sometimes the same form, sometimes different forms). Both scenarios run the risk of having an ID conflict.

function ContactForm () { 
  // ...
  <Field name="name" ... />
}

function ProgramForm () {
  // ...
  <Field name="name" ... />
}

function SurveyStep () {
  return (
    <>
      <ContactForm />
      {programs.map((p) => <ProgramForm key={p.id} .. />}
    </>
}

The former scenario makes it so that the form+'-'+inputName is not sufficiently unique. That being said, react-uid is already included as dependency for other features in the app so that could be used as well. Unfortunately this makes me realize that there's another issue, where the id that is generated for InputError is not guaranteed to be unique, which could also impact users who use a screenreader (e.g., inaccurately reporting that a field has an error) https://github.com/LaunchPadLab/lp-components/blob/f1b26955d5858b6efa3cae1ee8ad90abcea2a055/src/forms/labels/input-error.js#L76

dpikt commented 3 years ago

Wouldn't form names still be unique in the first scenario since redux-form requires a unique key?

As far as resolving goes, I think a PR just for this issue would be sufficient. You could probably resolve the input error issue by allowing the auto-generated ID to be overridden and passing it in via LabeledField if you wanted to avoid a breaking change.

chawes13 commented 3 years ago

🤦 right, right of course. Thanks for the clarification!

chawes13 commented 3 years ago

@dpikt Unfortunately this approach does not work on CheckboxGroup or RadioGroup inputs, as the explicit id gets passed to each individual input.

This behavior may be able to be extracted into a separate, isolated issue as I do not think we want this behavior in any circumstance. Thoughts?

dpikt commented 3 years ago

@chawes13 agreed, that issue is separate.