Skip to content

Commit

Permalink
[combobox] Revert #783
Browse files Browse the repository at this point in the history 
This PR introduced regression in some cases where the popover doesn't
open until the user presses the down or up key after initial input.
See #755 for the related issue.
  • Loading branch information
@chaance
chaance committed Oct 19, 2021
1 parent 03c58dd commit 55e1d8c
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 92 deletions.
148 changes: 74 additions & 74 deletions packages/combobox/__tests__/combobox.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,32 +207,32 @@ describe("<Combobox />", () => {
expect(getByTextWithMarkup(optionToSelect)).toBeInTheDocument();
});

it("should *not* open a list when input value changes without text entry", () => {
let optionToSelect = "Eagle Pass, Texas";

function EaglePassSelector() {
let [term, setTerm] = React.useState("");
return (
<div>
<button
type="button"
onClick={() => {
setTerm(optionToSelect);
}}
>
Select Eagle Pass
</button>
<ControlledCombobox term={term} setTerm={setTerm} />
</div>
);
}

let { getByRole, queryByRole } = render(<EaglePassSelector />);

let button = getByRole("button");
userEvent.click(button);
expect(queryByRole("listbox")).toBeFalsy();
});
// it("should *not* open a list when input value changes without text entry", () => {
// let optionToSelect = "Eagle Pass, Texas";

// function EaglePassSelector() {
// let [term, setTerm] = React.useState("");
// return (
// <div>
// <button
// type="button"
// onClick={() => {
// setTerm(optionToSelect);
// }}
// >
// Select Eagle Pass
// </button>
// <ControlledCombobox term={term} setTerm={setTerm} />
// </div>
// );
// }

// let { getByRole, queryByRole } = render(<EaglePassSelector />);

// let button = getByRole("button");
// userEvent.click(button);
// expect(queryByRole("listbox")).toBeFalsy();
// });
});
});

Expand Down Expand Up @@ -277,54 +277,54 @@ function BasicCombobox() {
);
}

function ControlledCombobox({
term,
setTerm,
}: {
term: string;
setTerm:
| ((term: string) => void)
| ((setter: (prevTerm: string) => string) => void);
}) {
let results = useCityMatch(term);

function handleChange(event: any) {
setTerm(event.target.value);
}

return (
<div>
<h2>Clientside Search</h2>
<Combobox id="holy-smokes">
<ComboboxInput
aria-label="cool search"
data-testid="input"
name="awesome"
onChange={handleChange}
value={term}
/>
{results ? (
<ComboboxPopover portal={false}>
{results.length === 0 ? (
<p>No results</p>
) : (
<ComboboxList data-testid="list">
{results.slice(0, 10).map((result, index) => (
<ComboboxOption
key={index}
value={`${result.city}, ${result.state}`}
/>
))}
</ComboboxList>
)}
</ComboboxPopover>
) : (
<span>No Results!</span>
)}
</Combobox>
</div>
);
}
// function ControlledCombobox({
// term,
// setTerm,
// }: {
// term: string;
// setTerm:
// | ((term: string) => void)
// | ((setter: (prevTerm: string) => string) => void);
// }) {
// let results = useCityMatch(term);

// function handleChange(event: any) {
// setTerm(event.target.value);
// }

// return (
// <div>
// <h2>Clientside Search</h2>
// <Combobox id="holy-smokes">
// <ComboboxInput
// aria-label="cool search"
// data-testid="input"
// name="awesome"
// onChange={handleChange}
// value={term}
// />
// {results ? (
// <ComboboxPopover portal={false}>
// {results.length === 0 ? (
// <p>No results</p>
// ) : (
// <ComboboxList data-testid="list">
// {results.slice(0, 10).map((result, index) => (
// <ComboboxOption
// key={index}
// value={`${result.city}, ${result.state}`}
// />
// ))}
// </ComboboxList>
// )}
// </ComboboxPopover>
// ) : (
// <span>No Results!</span>
// )}
// </Combobox>
// </div>
// );
// }

function useCityMatch(term: string) {
return term.trim() === ""
Expand Down
2 changes: 1 addition & 1 deletion packages/combobox/examples/index.story.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export { Example as Controlled } from "./controlled.example.js";
export { Example as LotsOfElements } from "./lots-of-elements.example.js";
export { Example as NoPopover } from "./no-popover.example.js";
export { Example as OpenOnFocus } from "./open-on-focus.example.js";
export { Example as SimulatedChange } from "./simulated-change.example.js";
// export { Example as SimulatedChange } from "./simulated-change.example.js";
export { Example as TokenInput } from "./token-input.example.js";
export { Example as WithButton } from "./with-button.example.js";
export { Example as WithCustomSelectDataTs } from "./with-custom-select-data.example.tsx";
Expand Down
10 changes: 10 additions & 0 deletions packages/combobox/examples/simulated-change.example.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,16 @@ import {
import { useCityMatch } from "./utils";
import "@reach/combobox/styles.css";

/**
* TODO: This example is buggy at the moment. The example itself and the bug it
* fixed was introduced in #783, but I merged it before testing thoroughly
* enough and it introduced what I believe are actually a more harmful
* regression in some cases where the popover doesn't open until the user
* presses the down or up key after initial input. Leaving the example and
* related test in place as a TODO.
* See https://github.com/reach/reach-ui/issues/755
*/

let name = "Simulated Change";

function Example() {
Expand Down
35 changes: 18 additions & 17 deletions packages/combobox/src/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,10 @@ const CLEAR = "CLEAR";
// User is typing
const CHANGE = "CHANGE";

// Any input change that is not triggered by an actual onChange event.
// For example an initial value or a controlled value that was changed.
// Prevents sending the user to the NAVIGATING state
// Initial input value change handler for syncing user state with state machine
// Prevents initial change from sending the user to the NAVIGATING state
// https://github.com/reach/reach-ui/issues/464
const SIMULATED_CHANGE = "SIMULATED_CHANGE";
const INITIAL_CHANGE = "INITIAL_CHANGE";

// User is navigating w/ the keyboard
const NAVIGATE = "NAVIGATE";
Expand Down Expand Up @@ -108,7 +107,7 @@ const stateChart: StateChart = {
[BLUR]: IDLE,
[CLEAR]: IDLE,
[CHANGE]: SUGGESTING,
[SIMULATED_CHANGE]: IDLE,
[INITIAL_CHANGE]: IDLE,
[FOCUS]: SUGGESTING,
[NAVIGATE]: NAVIGATING,
[OPEN_WITH_BUTTON]: SUGGESTING,
Expand Down Expand Up @@ -161,7 +160,7 @@ const reducer: Reducer = (data: StateData, event: MachineEvent) => {
let nextState = { ...data, lastEventType: event.type };
switch (event.type) {
case CHANGE:
case SIMULATED_CHANGE:
case INITIAL_CHANGE:
return {
...nextState,
navigationValue: null,
Expand Down Expand Up @@ -429,8 +428,11 @@ export const ComboboxInput = React.forwardRef(function ComboboxInput(
forwardedRef
) {
// https://github.com/reach/reach-ui/issues/464
// https://github.com/reach/reach-ui/issues/755
let inputValueChangedRef = React.useRef(false);
let { current: initialControlledValue } = React.useRef(controlledValue);
let controlledValueChangedRef = React.useRef(false);
useUpdateEffect(() => {
controlledValueChangedRef.current = true;
}, [controlledValue]);

let {
data: { navigationValue, value, lastEventType },
Expand Down Expand Up @@ -469,13 +471,16 @@ export const ComboboxInput = React.forwardRef(function ComboboxInput(
(value: ComboboxValue) => {
if (value.trim() === "") {
transition(CLEAR);
} else if (!inputValueChangedRef.current) {
transition(SIMULATED_CHANGE, { value });
} else if (
value === initialControlledValue &&
!controlledValueChangedRef.current
) {
transition(INITIAL_CHANGE, { value });
} else {
transition(CHANGE, { value });
}
},
[transition]
[initialControlledValue, transition]
);

React.useEffect(() => {
Expand All @@ -490,17 +495,13 @@ export const ComboboxInput = React.forwardRef(function ComboboxInput(
) {
handleValueChange(controlledValue!);
}
// After we handled the changed value, we need to make sure the next
// controlled change won't trigger a CHANGE event. (instead of a SIMULATED_CHANGE)
inputValueChangedRef.current = false;
}, [controlledValue, handleValueChange, isControlled, value]);

// [*]... and when controlled, we don't trigger handleValueChange as the
// user types, instead the developer controls it with the normal input
// onChange prop
function handleChange(event: React.ChangeEvent<HTMLInputElement>) {
let { value } = event.target;
inputValueChangedRef.current = true;
if (!isControlled) {
handleValueChange(value);
}
Expand Down Expand Up @@ -1330,7 +1331,7 @@ type State = "IDLE" | "SUGGESTING" | "NAVIGATING" | "INTERACTING";
type MachineEventType =
| "CLEAR"
| "CHANGE"
| "SIMULATED_CHANGE"
| "INITIAL_CHANGE"
| "NAVIGATE"
| "SELECT_WITH_KEYBOARD"
| "SELECT_WITH_CLICK"
Expand Down Expand Up @@ -1362,7 +1363,7 @@ interface StateData {
type MachineEvent =
| { type: "BLUR" }
| { type: "CHANGE"; value: ComboboxValue }
| { type: "SIMULATED_CHANGE"; value: ComboboxValue }
| { type: "INITIAL_CHANGE"; value: ComboboxValue }
| { type: "CLEAR" }
| { type: "CLOSE_WITH_BUTTON" }
| { type: "ESCAPE" }
Expand Down

0 comments on commit 55e1d8c

Please sign in to comment.