Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Combobox with autoSelect + autoComplete="both" props #1219

Merged
merged 6 commits into from
Apr 13, 2022
Merged
Show file tree
Hide file tree
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
5 changes: 5 additions & 0 deletions .changeset/hot-plums-tie.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"ariakit-utils": minor
---

Add `usePreviousValue` function to `ariakit-utils/hooks`. ([#1219](https://github.com/ariakit/ariakit/pull/1219))
5 changes: 5 additions & 0 deletions .changeset/polite-points-act.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"ariakit": patch
---

Fix `Combobox` with `autoSelect` and `autoComplete="both"` props setting an incorrect value when there are no matches. ([#1219](https://github.com/ariakit/ariakit/pull/1219))
11 changes: 11 additions & 0 deletions packages/ariakit-utils/src/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,17 @@ export function useLiveRef<T>(value: T) {
return ref;
}

/**
* Keeps the reference of the previous value to be used in the render phase.
*/
export function usePreviousValue<T>(value: T) {
const [previousValue, setPreviousValue] = useState(value);
if (value !== previousValue) {
setPreviousValue(value);
}
return previousValue;
}

/**
* Creates a memoized callback function that is constantly updated with the
* incoming callback.
Expand Down
73 changes: 73 additions & 0 deletions packages/ariakit/src/combobox/__examples__/combobox-group/food.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
export default [
{ name: "Apple", type: "Fruits" },
{ name: "Avocado", type: "Fruits" },
{ name: "Banana", type: "Fruits" },
{ name: "Cherry", type: "Fruits" },
{ name: "Coconut", type: "Fruits" },
{ name: "Grape", type: "Fruits" },
{ name: "Kiwi", type: "Fruits" },
{ name: "Lemon", type: "Fruits" },
{ name: "Lime", type: "Fruits" },
{ name: "Mango", type: "Fruits" },
{ name: "Orange", type: "Fruits" },
{ name: "Papaya", type: "Fruits" },
{ name: "Pear", type: "Fruits" },
{ name: "Pineapple", type: "Fruits" },
{ name: "Pomegranate", type: "Fruits" },
{ name: "Strawberry", type: "Fruits" },
{ name: "Watermelon", type: "Fruits" },

{ name: "Bacon", type: "Meat" },
{ name: "Beef", type: "Meat" },
{ name: "Chicken", type: "Meat" },
{ name: "Fish", type: "Meat" },
{ name: "Ham", type: "Meat" },
{ name: "Lamb", type: "Meat" },
{ name: "Pork", type: "Meat" },
{ name: "Sausage", type: "Meat" },
{ name: "Turkey", type: "Meat" },

{ name: "Bread", type: "Bakery" },
{ name: "Cake", type: "Bakery" },
{ name: "Cookie", type: "Bakery" },
{ name: "Croissant", type: "Bakery" },
{ name: "Donut", type: "Bakery" },
{ name: "Eclair", type: "Bakery" },
{ name: "Fondant", type: "Bakery" },
{ name: "Gelato", type: "Bakery" },
{ name: "Muffin", type: "Bakery" },
{ name: "Pie", type: "Bakery" },
{ name: "Pudding", type: "Bakery" },
{ name: "Scone", type: "Bakery" },
{ name: "Souffle", type: "Bakery" },
{ name: "Tart", type: "Bakery" },
{ name: "Waffle", type: "Bakery" },

{ name: "Butter", type: "Dairy" },
{ name: "Cheese", type: "Dairy" },
{ name: "Cream", type: "Dairy" },
{ name: "Egg", type: "Dairy" },
{ name: "Milk", type: "Dairy" },
{ name: "Sour cream", type: "Dairy" },
{ name: "Yogurt", type: "Dairy" },

{ name: "Beer", type: "Beverages" },
{ name: "Brandy", type: "Beverages" },
{ name: "Cider", type: "Beverages" },
{ name: "Coffee", type: "Beverages" },
{ name: "Cognac", type: "Beverages" },
{ name: "Gin", type: "Beverages" },
{ name: "Juice", type: "Beverages" },
{ name: "Liqueur", type: "Beverages" },
{ name: "Milkshake", type: "Beverages" },
{ name: "Rum", type: "Beverages" },
{ name: "Sake", type: "Beverages" },
{ name: "Sherry", type: "Beverages" },
{ name: "Soda", type: "Beverages" },
{ name: "Tea", type: "Beverages" },
{ name: "Tequila", type: "Beverages" },
{ name: "Vodka", type: "Beverages" },
{ name: "Water", type: "Beverages" },
{ name: "Whiskey", type: "Beverages" },
{ name: "Wine", type: "Beverages" },
];
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { Fragment, useMemo } from "react";
import {
Combobox,
ComboboxGroup,
ComboboxGroupLabel,
ComboboxItem,
ComboboxPopover,
ComboboxSeparator,
useComboboxState,
} from "ariakit/combobox";
import groupBy from "lodash/groupBy";
import food from "./food";
import "./style.css";

const list = food.map((item) => item.name);

export default function Example() {
const combobox = useComboboxState({ gutter: 8, sameWidth: true, list });

// Transform combobox.matches into groups of objects.
const matches = useMemo(() => {
const items = combobox.matches
.map((value) => food.find((item) => item.name === value)!)
.filter(Boolean);
return Object.entries(groupBy(items, "type"));
}, [combobox.matches]);

return (
<div>
<label className="label">
Your favorite food
<Combobox
state={combobox}
placeholder="e.g., Apple"
className="combobox"
autoComplete="both"
autoSelect
/>
</label>
<ComboboxPopover state={combobox} className="popover">
{!!matches.length ? (
matches.map(([type, items], index, array) => (
<Fragment key={type}>
<ComboboxGroup className="group">
<ComboboxGroupLabel className="group-label">
{type}
</ComboboxGroupLabel>
{items.map((item) => (
<ComboboxItem
focusOnHover
key={item.name}
value={item.name}
className="combobox-item"
/>
))}
</ComboboxGroup>
{index < array.length - 1 && (
<ComboboxSeparator className="separator" />
)}
</Fragment>
))
) : (
<div className="no-results">No results found</div>
)}
</ComboboxPopover>
</div>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
@import url("../combobox/style.css");

.group-label {
@apply
text-sm
p-2
font-medium
opacity-60
cursor-default;
}

.separator {
border-width: 1px 0 0;
border-color: currentcolor;
@apply my-2 h-0 opacity-25;
}

.no-results {
@apply gap-2 p-2;
}
80 changes: 80 additions & 0 deletions packages/ariakit/src/combobox/__examples__/combobox-group/test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import {
click,
getByRole,
hover,
press,
render,
type,
} from "ariakit-test-utils";
import { axe } from "jest-axe";
import Example from ".";

const getCombobox = () => getByRole("combobox");
const getOption = (name: string) => getByRole("option", { name });

function getSelectionValue(element: Element) {
const input = element as HTMLInputElement;
const { selectionStart, selectionEnd } = input;
const selectionValue = input.value.slice(selectionStart!, selectionEnd!);
return selectionValue;
}

test("a11y", async () => {
const { container } = render(<Example />);
expect(await axe(container)).toHaveNoViolations();
});

test("auto select with inline autocomplete on typing", async () => {
render(<Example />);
await press.Tab();
await type("a");
expect(getCombobox()).toHaveValue("apple");
expect(getSelectionValue(getCombobox())).toBe("pple");
await press.ArrowDown();
expect(getCombobox()).toHaveValue("Avocado");
expect(getSelectionValue(getCombobox())).toBe("");
await press.ArrowUp();
expect(getCombobox()).toHaveValue("apple");
expect(getSelectionValue(getCombobox())).toBe("pple");
await type("e");
expect(getCombobox()).toHaveValue("ae");
expect(getSelectionValue(getCombobox())).toBe("");
await type("\b\bv");
expect(getCombobox()).toHaveValue("vodka");
expect(getSelectionValue(getCombobox())).toBe("odka");
});

test("auto select with inline autocomplete on arrow down", async () => {
render(<Example />);
await press.Tab();
await press.ArrowDown();
await press.ArrowDown();
expect(getCombobox()).toHaveValue("Apple");
expect(getSelectionValue(getCombobox())).toBe("Apple");
await press.ArrowDown();
expect(getCombobox()).toHaveValue("Avocado");
expect(getSelectionValue(getCombobox())).toBe("");
await press.ArrowUp();
expect(getCombobox()).toHaveValue("Apple");
});

test("blur input after autocomplete", async () => {
render(<Example />);
await press.Tab();
await type("a");
expect(getCombobox()).toHaveValue("apple");
await press.ArrowDown();
expect(getCombobox()).toHaveValue("Avocado");
await click(document.body);
await click(document.body);
expect(getCombobox()).toHaveValue("Avocado");
});

test("autocomplete on focus on hover", async () => {
render(<Example />);
await click(getCombobox());
await type("g");
expect(getCombobox()).toHaveValue("grape");
await hover(getOption("Gelato"));
expect(getCombobox()).toHaveValue("g");
});
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export default function Example() {
<ComboboxItem key={value} value={value} className="combobox-item" />
))
) : (
<div>No results found</div>
<div className="no-results">No results found</div>
)}
</ComboboxPopover>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1,5 @@
@import url("../combobox/style.css");

.no-results {
@apply gap-2 p-2;
}
28 changes: 13 additions & 15 deletions packages/ariakit/src/combobox/combobox-state.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { useEffect, useMemo, useState } from "react";
import { useMemo, useState } from "react";
import {
useControlledState,
useDeferredValue,
useLiveRef,
usePreviousValue,
useUpdateLayoutEffect,
} from "ariakit-utils/hooks";
import { normalizeString } from "ariakit-utils/misc";
Expand Down Expand Up @@ -98,24 +98,22 @@ export function useComboboxState({
includesBaseElement,
});
const popover = usePopoverState({ ...props, placement });
const [activeValue, setActiveValue] = useState<string | undefined>();
const compositeRef = useLiveRef(composite);
const prevActiveId = usePreviousValue(composite.activeId);
const prevMoves = usePreviousValue(composite.moves);
const [moved, setMoved] = useState(false);

// Always reset the active value when the active item changes.
useEffect(() => {
setActiveValue(undefined);
}, [composite.activeId]);
if (prevActiveId !== composite.activeId) {
setMoved(prevMoves !== composite.moves);
}

// Update the active value when the active item changes by moving (which
// usually happens when using the keyboard).
useEffect(() => {
const { items, activeId } = compositeRef.current;
if (!activeId) return;
const nextActiveValue = items.find(
(item) => item.id === activeId && item.value
const activeValue = useMemo(() => {
if (!moved) return undefined;
return composite.items.find(
(item) => item.id === composite.activeId && item.value
)?.value;
setActiveValue(nextActiveValue);
}, [composite.moves]);
}, [moved, composite.items, composite.activeId]);

const deferredValue = useDeferredValue(value);

Expand Down
16 changes: 15 additions & 1 deletion packages/ariakit/src/combobox/combobox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,21 @@ import {
createHook,
} from "ariakit-utils/system";
import { As, BooleanOrCallback, Props } from "ariakit-utils/types";
import { unstable_batchedUpdates } from "react-dom";
import { CompositeOptions, useComposite } from "../composite/composite";
import {
PopoverAnchorOptions,
usePopoverAnchor,
} from "../popover/popover-anchor";
import { ComboboxState } from "./combobox-state";

function batchUpdates(fn: () => void) {
if (unstable_batchedUpdates) {
return unstable_batchedUpdates(fn);
}
return fn();
}

function isFirstItemAutoSelected(
items: ComboboxState["items"],
activeValue: ComboboxState["activeValue"],
Expand Down Expand Up @@ -164,7 +172,13 @@ export const useCombobox = createHook<ComboboxOptions>(
// we want to automatically focus on the first suggestion. This effect
// will run both when value changes and when items change so we can also
// catch async items. We need to defer the focus to avoid scroll jumps.
const timeout = setTimeout(() => state.move(state.first()), 16);
const timeout = setTimeout(() => {
// Since we're updating the state inside the timeout, we need to
// manually batch the updates so they happen at the same time.
// Otherwise, state.moves and state.activeId may be out of sync. This
// only applies to React 17 and below.
batchUpdates(() => state.move(state.first()));
}, 16);
return () => clearTimeout(timeout);
}, [
valueUpdated,
Expand Down