Skip to content

Commit

Permalink
Addressed Jesse's feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
m-akinc committed Sep 19, 2024
1 parent f3941eb commit 7040c53
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 4 deletions.
48 changes: 44 additions & 4 deletions specs/required-inputs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ Initially, we need this for:
- `nimble-text-area`
- `nimble-text-field`

Any other controls that could possibly be considered inputs are out of scope.

## Links To Relevant Work Items and Reference Material

- [Nimble issue](https://github.com/ni/nimble/issues/2100)
Expand All @@ -32,22 +34,52 @@ For radio buttons/groups, only the button elements expose the `required` attribu

### Visuals

We will update our templates so that setting `required` causes a red asterisk to be displayed after the label, as per the visual design. For radio buttons/groups, the asterisk is on the group label, not individual button labels.
We will update our templates so that setting `required` causes a red asterisk to be displayed after the label, as per the visual design. For radio buttons/groups, the asterisk is on the group label, not individual button labels. We have already forked some of the templates from FAST, but we will now need to fork the ones for `nimble-number-field`, `nimble-radio-group`, and `nimble-text-field`.

This will be implemented using a styled span containing the text "*". The span must be outside of the `label` element so that it is not included in the accessible name. We will wrap the two in a `div` so that we can force a horizontal layout (`flex-direction: row`). This structure will also ensure that whatever the wrapping/ellipsizing behavior of the label is, the asterisk will always appear at the far right:

```html
<div class="annotated-label">
<label part="label" class="label">
<slot></slot>
</label>
${when(x => x.required, html`
<span class="required-indicator">*</span>
`)}
</div>
```

Note that for text-field, text-area, and number-field, we will also remove the visual if the control has the `readonly` attribute set, since [validation is not enforced on a readonly input](https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/readonly#attribute_interactions). This doesn't apply to select, combobox, or radio buttons, because they do not support `readonly`.

Styles will be shared.

### Form validation

#### Vanilla HTML

We would expect this to already work, since the FAST components we derive from provide forms support. Unfortunately, this support is broken for `nimble-combobox` and `nimble-radio`.
When the user attempts to submit a form and a `required` input is missing a value, a browser-specific indicator reports the validation error:

![Missing value indicator](missing-value.png)

- `nimble-combobox`: All that is needed is to forward the `required` attribute to the `input` in the template (which we have already forked from FAST). We will make this change.
We would expect this to already work, since the FAST components we derive from provide forms support. Unfortunately, this support is broken or incomplete for multiple components:

- `nimble-combobox`:
- In Chrome/Edge, a missing required value properly blocks form submission and updates the control's `validity` flags, but the visual indicator of the validation error is not shown, and instead an error is printed to the console: "An invalid form control with name='' is not focusable." Presumably, something is trying to focus the host element, though it delegates focus to the shadow DOM. The error comes from a call to [`form.requestSubmit()` in the FAST button code](https://github.com/microsoft/fast/blob/913c27e7e8503de1f7cd50bdbc9388134f52ef5d/packages/web-components/fast-foundation/src/button/button.ts#L221), which we cannot debug into. Things work properly in Firefox.
- Accessibility: the control is not marked/announced as required. Forwarding `required` to the `input` in the template (which we have already forked) solves this.
- `nimble-text-area`:
- Has same Chrome/Edge issue as `nimble-combobox`.
- `nimble-select`:
- Validation incorrectly reports a missing value until a value change has occurred. I.e. the initial value is not seen. This happens because we try to mirror the initial value to the proxy native `select` element (within [a call to `super.slottedOptionsChanged()`](https://github.com/ni/nimble/blob/ddad57c4c97da9504f8146ad48668f290dae5301/packages/nimble-components/src/select/index.ts#L331)) before we have [mirrored the child option elements](https://github.com/ni/nimble/blob/ddad57c4c97da9504f8146ad48668f290dae5301/packages/nimble-components/src/select/index.ts#L346). The native `select` will reject/ignore setting `value` if it doesn't correspond to the value of one of its child options (of which it has none, at that point). Right after mirroring the child options, there is a call to `updateValue()`, but because the value has already been updated earlier, it [short-circuits](https://github.com/ni/nimble/blob/ddad57c4c97da9504f8146ad48668f290dae5301/packages/nimble-components/src/select/index.ts#L268), skipping the code path that would have updated the proxy. I suspect we can find a reasonable fix for this.
- Accessibility: the control is not marked/announced as required. The accessible control (i.e. the one defining the `role`) is the `nimble-select` itself. Since this is not a native input, the accessibility tree doesn't see/understand the `required` attribute on it. We need to additionally set `aria-required` on it (bound to `required`).
- `nimble-radio`:
- `required` on a radio button [does not take other buttons in the group into account](https://github.com/microsoft/fast/issues/6866). It will result in a validation error if _that specific_ radio is unchecked.
- Validity is only updated when the value or checked state changes, so if the radio button is initially unchecked and remains unchecked, it will remain marked valid, even though it violates the `required` constraint.

Forms support is complete and functional for `nimble-number-field` and `nimble-text-field`.

Because the `nimble-radio` support has never worked, would take significant work to fix, and FAST is no longer accepting submissions to the archive branch we use, we **will not fix radio button forms support** as part of this feature.

Forms support already works for `nimble-number-field`, `nimble-select`, `nimble-text-area`, and `nimble-text-field`.
We will file a Chromium bug for the issue affecting the `nimble-combobox` and `nimble-text-area` but **will not try to work around it.**

#### Angular

Expand All @@ -57,6 +89,14 @@ The `required` attribute only plays a role in validation for template-driven for

Typically, form validation is handled via a `DataAnnotationsValidator`, and an input is treated as required if it is bound to a model property annotated with `RequiredAttribute` (i.e. `[Required]`). The Nimble component's `required` attribute is still needed to turn on the visual affordance, though, so we will add it to the Razor API.

## Documentation

We will add documentation for the `required` attribute. No additional guidance is necessary.

## Testing

Visual matrix test cases will be added for the affected components.

## Alternative Implementations / Designs

None
Expand Down
Binary file added specs/required-inputs/missing-value.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

0 comments on commit 7040c53

Please sign in to comment.