chakra-ui / ark

Ark UI is a headless UI library with over 45+ components designed to build scalable Design Systems that works for a wide range of JS frameworks.
https://ark-ui.com
MIT License
3.82k stars 112 forks source link

`Fieldset`'s invalid state is not respected by inputs #2928

Closed wingsofovnia closed 1 month ago

wingsofovnia commented 1 month ago

Description

When I use FieldSet with invalid attribute set to true, I expect underlying fields, such as <Input/>, to change its state to invalid too, regardless if they are wrapped or not into individual <Field/>, as stated in the docs, but nothing happens instead.

Related code: https://github.com/chakra-ui/ark/blob/a863f77cfef7e18735c29c0bf1f01e67c3d4ae83/packages/react/src/components/field/use-field.ts#L45-L53

Suggested change:

- invalid = false,
+ invalid = Boolean(fieldset?.invalid),

Steps to Reproduce

<FieldSet
  label="This is FieldSet"
  invalid={true}
  errorText="This is FieldSet error"
>
  <Input/>
</FieldSet>

<Field
  label="This is Field"
  invalid={true}
  errorText="This is Field error"
>
  <Input/>
</Field>

image

Ark UI Version

One used by @chakra-ui/react@3.0.0-next.25

Framework

Additional Information

One use case where I found this issue very unfortunate.

Assume there is custom input component DomainInput consisting of two fields (<Input/> and <NativeSelect/>) based on Charka UI docs example that follows Ark/Chakra style of composition:

<DomainInputRoot>
  <DomainInputSelect  {...register("domain")}/>
  <DomainInputField  {...register("host")}/>
</DomainInputRoot>

This is a natural candidate for FieldSet:

<FieldSet
  legend="Enter your address"
  invalid={!!errors.domain || !!errors.host}
  errorText={errors.host?.message ?? errors.domain?.message}
>
  <DomainInputRoot>
    <DomainInputSelect  {...register("domain")}/>
    <DomainInputField  {...register("host")}/>
  </DomainInputRoot>
</FieldSet>

The error state will not be propagated to the fields.

Wrapping <DomainInputSelect/> and <DomainInputField/> into <Field/> individually is a bad option because it adds extra DOM nesting that <DomainInputRoot/> does not expect.

cschroeter commented 1 month ago

Thanks for reporting this issue, @wingsofovnia!

This behavior was actually a design decision. Imagine you have a fieldset for an address. If the entire fieldset were marked as invalid because the user missed a single input (e.g., the zip code), passing the invalid prop through to all fields would render every input red—regardless of whether they're correct.

The invalid prop for theFieldset is more focused on displaying the <Fieldset.ErrorText> rather than styling every individual field.

I will close this issue and leave it to @segunadebayo if he wants to re-visit this design decision.

wingsofovnia commented 1 month ago

I see, that makes sense. However, it is unclear how to implement a fieldset for a set of inputs that should be treated as a whole.

Could you please suggest a Chakra-valid course of action for the use case I put into the issue, @cschroeter?

cschroeter commented 1 month ago

@wingsofovnia

I can see a scenario where you could enforce the invalid state on the fields within the fieldset. If you have a solid API in mind, feel free to raise a PR.

// sth along those lines
const fieldset = useFieldsetContext()
  const {
    ids,
    disabled = Boolean(fieldset?.disabled),
    invalid = Boolean(fieldset?.enforceInvalid ? fieldset?.invalid : false),
    readOnly = false,
    required = false,
  } = props
wingsofovnia commented 1 month ago

That's great! I would however want to raise the question of Fieldset default behaviour. I do get your use case but at the same time it is unclear why disabled is propagated then? Feels inconsistent to me. The docs also explicitly mention that both disabled and invalid should be respected by other components.

I suggest changing the default behaviour of Fieldset to closer what html5 fieldset is, that is, just visual/accessibility grouping with no propagated context.

I suggest composite attribute (named after Composite pattern) to signal this fieldset should be used as a composite of inputs and propagate state.

const fieldset = useFieldsetContext()
  const {
    ids,
    disabled = Boolean(fieldset?.composite ? fieldset?.disabled : false),
    invalid = Boolean(fieldset?.composite ? fieldset?.invalid : false),
    readOnly = false,
    required = false,
  } = props

Alternative names: bound, aggregate, grouped.

What do you think, @cschroeter?

cschroeter commented 1 month ago

@wingsofovnia

It's funny you mention that—our design decision was inspired by the default behavior of HTML5 fieldset. If you set disabled on the fieldset, it disables both the radio buttons and the input fields, but if you set invalid, it has no effect.

<form>
  <fieldset disabled>
    <legend>Choose your favorite monster</legend>

    <input type="radio" id="kraken" name="monster" value="K" />
    <label for="kraken">Kraken</label><br />

    <input type="radio" id="sasquatch" name="monster" value="S" />
    <label for="sasquatch">Sasquatch</label><br />

    <input type="radio" id="mothman" name="monster" value="M" />
    <label for="mothman">Mothman</label>

    <label for="email">Email</label>
    <input type="email" id="email" />
  </fieldset>
</form>

Feel free to play around with the fieldset: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/fieldset

wingsofovnia commented 1 month ago

@cschroeter

It's funny you mention that—our design decision was inspired by the default behavior of HTML5 fieldset. If you set disabled on the fieldset, it disables both the radio buttons and the input fields, but if you set invalid, it has no effect.

Indeed, my mistake. I looked on the same page but missed this detail.

That said, some may say that similarly to fieldset's native disabled, invalid, an extra attribute added in Chakra, should works similarly and:

If this [invalid] Boolean attribute is set, all form controls that are descendants of the <fieldset>, are considered invalid ...

I might be stretching it a bit but sounds logical to me :)

If that is still not convincing, I will go ahead and implement and extra attribute to enable invalid propagation only. I'd suggest invalidAll attribute tho: <Fieldset.Root invalidAll={true}/>

const fieldset = useFieldsetContext()
  const {
    ids,
    disabled = Boolean(fieldset?.disabled),
    invalid = Boolean(fieldset?.invalidAll),
    readOnly = false,
    required = false,
  } = props
cschroeter commented 1 month ago

@wingsofovnia

I will chat with @segunadebayo on Monday so let me circle back to you at that point. Thanks for your contributions so far

cschroeter commented 1 month ago

@wingsofovnia,

We're having a bit of trouble understanding how inheriting the invalid prop from a fieldset to its fields would function in a real-world application. Could you share some Dribbble or other UI mockups demonstrating how this would be used?