Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
191 changes: 191 additions & 0 deletions rfcs/convergence/input-native-element-props.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
# RFC: How to handle native element props for input components

---

@sopranopillow @ecraig12345

## Summary

This RFC proposes options on how to handle the native element props for input components on `vNext`. Currently the native props for all vNext components are applied to the root element, but in the case of an input component there are native input props that should be handled as well.

<!-- Explain the proposed change -->

## Problem statement

For most components, it makes sense to apply native props (and the ref) to the root element.

The input components are a little unique because the actual `<input>` _cannot_ be the root element of the component, since we need various wrappers for styling. For example, rendering a `<Checkbox/>` actually does this:

```tsx
<div>
<div ... /> {/* checkmark */}
<input type="checkbox" />
<label {...slotProps.label} />
</div>
```

Or an `<Input/>` is roughly like this:

```tsx
<div>
{/* optional bookend before slot */}
<div> {/* wrapper visually styled as an input (maybe, TBD) */}
{/* optional start slot */}
<input type="text" .../>
{/* optional end slot */}
</div>
{/* optional bookend after slot */}
</div>
```

Comment thread
sopranopillow marked this conversation as resolved.
### 3rd party Form validation libraries

Another unique thing about inputs is that to the degree possible, we'd like them to work nicely with 3rd-party form libraries, which may have APIs that expect to be able to pass native props to the component and have them applied to the actual `<input>`. (needs more research)

#### [Formik](https://github.com/formium/formik)

#### [react-hook-form](https://github.com/react-hook-form/react-hook-form)

Note, for this one they'd need to make a wrapper for our component because the library expects to be able to _set_ `.value` on the input, which we probably don't intend to support. (Someone brought this up for v8 [here](https://github.com/microsoft/fluentui/issues/18126).)

#### [React Final Form](https://github.com/final-form/react-final-form)

## Detailed Design or Proposal

We don't have one suggested/preferred solution yet, because all of them have some significant problems.

The examples below assume you're rendering a Checkbox and this is roughly your desired HTML output (it would also include a generated ID to associate the label and input):

```html
<div>
<input type="checkbox" name="foo" checked />
<label>sample</label>
</div>
```

### Option 1: All native props always applied to root (current behavior)
Copy link
Copy Markdown
Contributor

@ling1726 ling1726 Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you do a survey of other react design systems and what solutions they use ? AFAIK most design systems will follow something along the lines of Option 1 or Option 2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have to dig into it a bit more for Input (and Esteban would have a better idea for Checkbox) but I don't believe that's true. IIRC from the research I did, they appear to most commonly use either option 2 (combination) or possibly tending to apply the props just to the input not the root. But to confirm that I'd need to make a bunch of codesandboxes and look at their props and DOM.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far IMO I'd go for Option 1. seems more consistent with all other converged components.
But yeah, the arguments of counter intuitivity is valid.

Perhaps you could faltten some obvious properties in the root element and manually transfer it to the slot.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could faltten some obvious properties in the root element and manually transfer it to the slot.

unless this is consistent and explicitly stated for all vNext controls, I'd avoid any "special case" magic

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you could faltten some obvious properties in the root element and manually transfer it to the slot.

@bsunderhus Define "obvious"? I haven't been able to come up with a definition that relies on anything besides intuition, which is subjective and varies between people.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right. This is too much subjective.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked a few other systems that I could readily get a working sandbox for their text Input component, and the consensus seemed to be something like Option 2, only slightly modified in that all native props go to the input element, with a small number of exceptions that go to the wrapper (e.g. className).

I tested by adding a few props to the checkbox component and seeing what DOM element they ended up on (using data-test as a proxy for "any arbitrary prop"). The results were similar for most, but I've marked the differences with [*]

name id className aria-label data-test
Ant input input wrapper input input
Carbon input input input [*] input input
Fabric v8 input input wrapper - input
Northstar v0 input input wrapper input wrapper [*]
  • Carbon put className on the input element for text Input component in that table, but for the Checkbox component, it put className on the label element (not the wrapper, nor the input elements).


Given this:

```tsx
<Checkbox name="foo" checked>
sample
</Checkbox>
```

You get roughly this HTML, which is not useful:

```html
<div name="foo" checked>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure about checked attribute being added ? setAttribute throws error if you don't provide value. is this some buggy react-dom behaviour ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm looks like it's not applied. So we should update the resulting HTML, but it doesn't change the point, which is that spreading the attribute there is not useful. https://codepen.io/ecraig12345/pen/mdmOjqL

<input type="checkbox" />
<label>sample</label>
</div>
```

So to specify a value, you'd have to use slot props:

```tsx
<Checkbox input={{ name: 'foo', checked: true }}>sample</Checkbox>
```

#### Pros

- Complete consistency between all components

#### Cons

- Very counterintuitive API
- We could use typings to ensure that doing e.g. `<Checkbox checked/>` caused a TS error, but it still seems very unnatural
- Might not work as easily with 3rd-party form libraries? (needs more research)

### Option 2: Most native props applied to root, selected props applied to "actual" element
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To a certain extent this would happen anyway right ? since the component itself would most likely be exposing some kind of value prop for controlling


Given this, and an implementation which uses a list of special props (including `checked` and `name`) that are passed to the "actual" element:

```tsx
<Checkbox name="foo" checked>
sample
</Checkbox>
```

You'd get the desired HTML:

```html
<div>
<input type="checkbox" checked name="foo" />
<label>sample</label>
</div>
```

The problem is, it's unclear where some of the other props ought to be applied, such as `id`, `className`, and event handlers.

#### Pros

- Better API for input components (and maybe other special-case components)

#### Cons

- Not clear how to determine which top-level props should be passed down to the `input`
- If `checked` and `name` (and a few others) go to the `input`, the user might expect this behavior for all props and then be surprised
- Not clear what happens if a prop may be needed in multiple places
- Example: if top-level `className` or `id` is passed to the `input`, what happens if they also want to give the root a `className` or `id`?
- Inconsistent between components (reduces API clarity)

### Option 3

Let the native input element be the "root" slot, but use a `wrapper` slot as the actual root DOM element. Here's an example:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a slight modification to Option 3:

  • Modify the internal slots API to allow a component to specify the name of a slot that gets the native props that are specified directly on the component's props. It would be the root slot by default, but in this case, we'd override it to be the input slot instead.
  • The className prop would be special-cased to always go to the root slot.
  • Make root a real slot for every component, so you could write root={{ id: 'foo' }} to set the id of the root slot.

The result would be that components could choose the most logical slot to receive the native props for that component, but there would always be an escape hatch for a user to specify props on any slot.

E.g.:

<Checkbox id="foo" /> {/* The input element gets id="foo" */}
<Checkbox input={{ id: 'foo' }} /> {/* Same as above, just being more explicit */}
<Checkbox root={{ id: 'bar' }} /> {/* The root element gets id="bar" */}


```tsx
<slots.wrapper>
<div {...checkmark} />
<slots.root {...slotProps.root} /> <!-- input element -->
<slots.label {...slotProps.label} />
</slots.wrapper>
```

So doing this would give the desired HTML:
Comment thread
sopranopillow marked this conversation as resolved.

```html
<div>
<input type="checkbox" />
<label>Foo</label>
</div>
```

And if you wanted to apply props to the wrapper element, you could use slot props:

```tsx
<Checkbox name="foo" checked className="foo" wrapper={{ className: 'bar' }}>
sample
</Checkbox>
```

Which would give you roughly this:

```html
<div class="bar">
<input type="checkbox" name="foo" checked className="foo" />
<label>sample</label>
</div>
```

#### Pros

- For the individual component, more obvious which props go where, and a more intuitive API
- Allows any valid prop to be applied to the root and/or the input (not just one or the other)

#### Cons

- Inconsistent between components, potentially leading to confusion
- Might be unclear which components implement this special handling

## Discarded Solutions

(to be filled out once a solution is chosen)

## Open Issues

Issues related to this RFC:

- [Checkbox Convergence](https://github.com/microsoft/fluentui/issues/18454)
- [Input Convergence](https://github.com/microsoft/fluentui/issues/18131)