Skip to content

Commit

Permalink
refactor(select): Fix all syncing issues with the Select component (#…
Browse files Browse the repository at this point in the history
…2983)

The `useComboboxInputConstrained` hook was created for Comboboxes that can only have a value from within a set of defined options. The value is constrained by the options passed to it. `useComboboxInputConstrained` assumes there will be multiple inputs where one is presented to the user and the other is for the form/server. The hook keeps the model in sync with external ways to manipulate the input: React `value` prop ([controlled inputs](https://react.dev/reference/react-dom/components/input#controlling-an-input-with-a-state-variable)), browser/extension autofill, and testing tools.

`Select` was updated to use `useComboboxInputConstrained` which fixes several bugs at once:
- Fixes #2888
- Fixes #2675
- Fixes #2616
- Fixes #2533

[category:Components]
  • Loading branch information
NicholasBoll authored Oct 14, 2024
1 parent b361815 commit c1e0252
Show file tree
Hide file tree
Showing 21 changed files with 495 additions and 236 deletions.
4 changes: 2 additions & 2 deletions cypress/component/Autocomplete.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ describe('Autocomplete', () => {
cy.mount(<Autocomplete />);
});

it('should have aria-haspopup set to true', () => {
cy.findByRole('combobox').should('have.attr', 'aria-haspopup', 'true');
it('should have aria-haspopup set to listbox', () => {
cy.findByRole('combobox').should('have.attr', 'aria-haspopup', 'listbox');
});

it('should have aria-expanded set to false', () => {
Expand Down
4 changes: 2 additions & 2 deletions cypress/component/Select.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,8 @@ describe('Select', () => {
cy.findByRole('combobox').should('have.attr', 'role', 'combobox');
});

it('should have an `aria-popup="menu"`', () => {
cy.findByRole('combobox').should('have.attr', 'aria-haspopup', 'menu');
it('should have an `aria-popup="listbox"`', () => {
cy.findByRole('combobox').should('have.attr', 'aria-haspopup', 'listbox');
});

it('should have an `aria-expanded="false"`', () => {
Expand Down
65 changes: 64 additions & 1 deletion modules/docs/mdx/12.0-UPGRADE-GUIDE.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ A note to the reader:
- [Component Updates](#component-updates)
- [Styling API and CSS Tokens](#styling-api-and-css-tokens)
- [Avatar](#avatar)
- [Combobox](#combmbox)
- [Form Field](#form-field)
- [Form Field Group](#form-field-group)
- [Form Field Field](#form-field-field)
Expand Down Expand Up @@ -252,6 +253,64 @@ The following changes have been made to the API:
> union types. You will see a console warn message while in development if you're still using this.
> Please update your types and usage before v13.
### Combobox

**PR** [#2983](https://github.com/Workday/canvas-kit/pull/2983)

The `useComboboxInput` hook, and the `Combobox.Input` component used to automatically update the
input when an option was selected. This functionality has been split between
`useComboboxInputArbitrary` and `useComboboxInputConstrained` depending on the combobox's value
type. The `useComboboxInput` had the arbitrary functionality built in. This has been separated out.
The `<Select>` component has been updated to use `useComboboxInputConstrained` hook and the
`Autocomplete` example uses the `useComboboxInputArbitrary` hook. This is a breaking change if you
use the `Combobox.Input` component directly. You'll have to either pass the
`useComboboxInputArbitrary` elemProps hook directly to the `<Combbox.Input>` or create a new
component composing them.

```tsx
// v11
<Combobox>
<Combobox.Input />
// ...
</Combobox>

// v12
<Combobox>
<Combobox.Input elemPropsHook={useComboboxInputArbitrary} />
// ...
</Combobox>

// Better - create a specialized component
const useMyComboboxInput = composeHooks(
// or your own model that extends `useComboboxModel`
createElemPropsHook(useComboboxModel)(model => {
return {
// anything you need your input to do
}
}),
useComboboxInputArbitrary,
useComboboxInput
)

const MyComboboxInput = createSubcomponent(TextInput)({
// or your own model that extends `useComboboxModel`
modelHook: useComboboxModel,
elemPropsHook: useMyComboboxInput,
})((elemProps, Element) => {
// return a TextInput
return <Element {...elemProps} />

// return an InputGroupgs
return (
<InputGroup>
<InputGroup.InnerStart>{/* Icon or something */}</InputGroup.InnerStart>
<InputGroup.Input as={Element} {...elemProps} />
<InputGroup.InnerEnd><InputGroup.ClearButton /></InputGroup.InnerEnd>
</InputGroup>
)
})
```

### Form Field

<div style={{display: 'inline-flex', gap: '.5rem'}}>
Expand Down Expand Up @@ -457,7 +516,8 @@ to set the appropiate aria attributes and `id` on the underlying input element.

### Select

**PR:** [#2865](https://github.com/Workday/canvas-kit/pull/2865)
**PRs:** [#2865](https://github.com/Workday/canvas-kit/pull/2865),
[#2983](https://github.com/Workday/canvas-kit/pull/2983)

As we transition to use our CSS tokens to provide better theming capabilities and our new styling
methods, we're moving away from components extending `Themeable`. `Themeable` has been removed from
Expand Down Expand Up @@ -504,6 +564,9 @@ const theme: PartialEmotionCanvasTheme = {
</CanvasProvider>;
```

`Select` has been refactored to use `useComboboxInputConstrained` to sync the model and the `input`
element regardless of the source. This makes the `Select` a controllable component.

### Text Area

**PR:** [#2825](https://github.com/Workday/canvas-kit/pull/2825)
Expand Down
6 changes: 6 additions & 0 deletions modules/react/collection/lib/useSelectionListModel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,12 @@ export const useSelectionListModel = createModelHook({
unselectAll() {
setSelectedIds([]);
},
/**
* Should be used with care and can be used to keep a model in sync with external controlled
* inputs.
*/ setSelectedIds(ids: 'all' | string[]) {
setSelectedIds(ids);
},
};

return {...cursor, state, events, selection};
Expand Down
22 changes: 11 additions & 11 deletions modules/react/combobox/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ export {Combobox} from './lib/Combobox';
export {useComboboxCard} from './lib/ComboboxCard';
export {useComboboxMenuItem} from './lib/ComboboxMenuItem';
export {useComboboxMenuList} from './lib/ComboboxMenuList';
export {
useComboboxLoader,
useComboboxModel,
useComboboxInputOpenWithArrowKeys,
useComboboxKeyboardTypeAhead,
useSetPopupWidth,
useComboboxMoveCursorToSelected,
useComboboxInput,
useComboboxListKeyboardHandler,
useComboboxResetCursorToSelected,
} from './lib/hooks';
export * from './lib/hooks/useComboboxInputOpenWithArrowKeys';
export * from './lib/hooks/useComboboxLoader';
export * from './lib/hooks/useComboboxModel';
export * from './lib/hooks/useComboboxMoveCursorToSelected';
export * from './lib/hooks/useSetPopupWidth';
export * from './lib/hooks/useComboboxKeyboardTypeAhead';
export * from './lib/hooks/useComboboxListKeyboardHandler';
export * from './lib/hooks/useComboboxInput';
export * from './lib/hooks/useComboboxInputArbitrary';
export * from './lib/hooks/useComboboxInputConstrained';
export * from './lib/hooks/useComboboxResetCursorToSelected';
9 changes: 0 additions & 9 deletions modules/react/combobox/lib/hooks/index.ts

This file was deleted.

4 changes: 2 additions & 2 deletions modules/react/combobox/lib/hooks/useComboboxInput.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,14 @@ export const useComboboxInput = composeHooks(
},
value: model.state.value,
role: 'combobox',
'aria-haspopup': 'true' as React.AriaAttributes['aria-haspopup'],
'aria-haspopup': 'listbox',
'aria-expanded': model.state.visibility === 'visible',
'aria-autocomplete': 'list',
'aria-controls': `${model.state.id}-list`,
'aria-activedescendant': model.state.visibility === 'hidden' ? null : undefined, // Removes activedescendant on menu close
id: model.state.id,
ref: elementRef,
} as const;
};
}),
useSetPopupWidth,
useComboboxInputOpenWithArrowKeys,
Expand Down
32 changes: 32 additions & 0 deletions modules/react/combobox/lib/hooks/useComboboxInputArbitrary.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import React from 'react';
import {
createElemPropsHook,
useLocalRef,
dispatchInputEvent,
} from '@workday/canvas-kit-react/common';
import {useComboboxModel} from './useComboboxModel';

/**
* An arbitrary combobox can have any value. The list of options are suggestions to aid the user in
* entering values. A Typeahead or Autocomplete are examples are arbitrary value comboboxes.
*/
export const useComboboxInputArbitrary = createElemPropsHook(useComboboxModel)((model, ref) => {
const {elementRef, localRef} = useLocalRef(ref as React.Ref<HTMLInputElement>);

// sync model selection state with inputs
React.useLayoutEffect(() => {
if (localRef.current) {
const formValue = (model.state.selectedIds === 'all' ? [] : model.state.selectedIds).join(
', '
);

if (formValue !== localRef.current.value) {
dispatchInputEvent(localRef.current, formValue);
}
}
}, [model.state.selectedIds, localRef]);

return {
ref: elementRef,
};
});
157 changes: 157 additions & 0 deletions modules/react/combobox/lib/hooks/useComboboxInputConstrained.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
import React from 'react';
import {
createElemPropsHook,
useLocalRef,
dispatchInputEvent,
} from '@workday/canvas-kit-react/common';
import {useComboboxModel} from './useComboboxModel';

function onlyDefined<T>(input: T | undefined): input is T {
return !!input;
}

/**
* A constrained combobox input can only offer values that are part of the provided list of `items`.
* The default is an unconstrained. A constrained input should have both a form input that is hidden
* from the user as well as a user input that will be visible to the user. This hook is in charge of
* keeping the inputs and the model in sync with each other and working with a browser's
* autocomplete, form libraries and the model.
*/
export const useComboboxInputConstrained = createElemPropsHook(useComboboxModel)(
(
model,
ref,
{
disabled,
value: reactValue,
onChange,
name,
}: Pick<
React.InputHTMLAttributes<HTMLInputElement>,
'disabled' | 'value' | 'onChange' | 'name'
> = {}
) => {
// The user element is what the user sees
const {elementRef: userElementRef, localRef: userLocalRef} = useLocalRef(
model.state.targetRef as React.Ref<HTMLInputElement>
);

// The form element is what is seen in `FormData` during for submission to the server
const {elementRef: formElementRef, localRef: formLocalRef} = useLocalRef(
ref as React.Ref<HTMLInputElement>
);

// Create React refs so we can get the current value inside an Effect without using those values
// as part of the dependency array.
const modelNavigationRef = React.useRef(model.navigation);
modelNavigationRef.current = model.navigation;
const modelStateRef = React.useRef(model.state);
modelStateRef.current = model.state;

// Watch the `value` prop passed from React props and update the model accordingly
React.useLayoutEffect(() => {
if (formLocalRef.current && typeof reactValue === 'string') {
// const value = formLocalRef.current.value;
if (reactValue !== formLocalRef.current.value) {
model.events.setSelectedIds(reactValue ? reactValue.split(', ') : []);
}
}
}, [reactValue, formLocalRef, model.events]);

// useImperativeHandle allows us to modify the `ref` before it is sent to the application,
// but after it is defined. We can add value watches, and redirect methods here.
React.useImperativeHandle(
formElementRef,
() => {
if (formLocalRef.current) {
// Hook into the DOM `value` property of the form input element and update the model
// accordingly
Object.defineProperty(formLocalRef.current, 'value', {
get() {
const value = Object.getOwnPropertyDescriptor(
Object.getPrototypeOf(formLocalRef.current),
'value'
)?.get?.call(formLocalRef.current);
return value;
},
set(value: string) {
if (
formLocalRef.current &&
value !==
(modelStateRef.current.selectedIds === 'all'
? []
: modelStateRef.current.selectedIds
).join(', ')
) {
model.events.setSelectedIds(value ? value.split(', ') : []);
}
},
});

// forward calls to `.focus()` and `.blur()` to the user input
formLocalRef.current.focus = (options?: FocusOptions) => {
userLocalRef.current!.focus(options);
};
formLocalRef.current.blur = () => {
userLocalRef.current!.blur();
};
}
return formLocalRef.current!;
},
[formLocalRef, userLocalRef, model.events]
);

// sync model selection state with inputs
React.useLayoutEffect(() => {
if (userLocalRef.current) {
const userValue =
model.state.items.length === 0
? ''
: (model.state.selectedIds === 'all'
? []
: model.state.selectedIds
.map(id =>
modelNavigationRef.current.getItem(id, {state: modelStateRef.current})
)
.filter(onlyDefined)
.map(item => item.textValue)
).join(', ');

if (userValue !== userLocalRef.current.value) {
dispatchInputEvent(userLocalRef.current, userValue);
}
}

if (formLocalRef.current) {
const formValue = (model.state.selectedIds === 'all' ? [] : model.state.selectedIds).join(
', '
);

if (formValue !== formLocalRef.current.value) {
dispatchInputEvent(formLocalRef.current, formValue);
}
}
}, [model.state.selectedIds, model.state.items, formLocalRef, userLocalRef]);

// The props here will go to the user input.
return {
ref: userElementRef,
form: '', // We don't want the user input to be part of the form [elements](https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/elements)
value: null,
onChange: null,
name: null,
disabled,
/**
* These props should be spread onto the form input.
*/
formInputProps: {
disabled,
tabIndex: -1,
'aria-hidden': true,
ref: formElementRef,
onChange,
name,
},
};
}
);
Loading

0 comments on commit c1e0252

Please sign in to comment.