From 3ede826f0281edeb1314688e003231c9d289f504 Mon Sep 17 00:00:00 2001 From: Esteban Munoz Date: Tue, 31 Jan 2023 18:39:05 -0700 Subject: [PATCH 1/9] adding possible fix --- .../ComboBox/ComboBox.FreeInput.Example.tsx | 79 +++++++++++-------- .../src/components/ComboBox/ComboBox.tsx | 27 ++++++- 2 files changed, 73 insertions(+), 33 deletions(-) diff --git a/packages/react-examples/src/react/ComboBox/ComboBox.FreeInput.Example.tsx b/packages/react-examples/src/react/ComboBox/ComboBox.FreeInput.Example.tsx index 39a9c919af2f9e..86d6e02b4db95a 100644 --- a/packages/react-examples/src/react/ComboBox/ComboBox.FreeInput.Example.tsx +++ b/packages/react-examples/src/react/ComboBox/ComboBox.FreeInput.Example.tsx @@ -1,44 +1,59 @@ import * as React from 'react'; -import { ComboBox } from '@fluentui/react'; -import type { IComboBoxOption, IComboBoxStyles } from '@fluentui/react'; +import { ComboBox, IComboBoxProps } from '@fluentui/react'; +import type { IComboBoxOption } from '@fluentui/react'; const options: IComboBoxOption[] = [ - { key: 'black', text: 'Black' }, - { key: 'blue', text: 'Blue' }, - { key: 'brown', text: 'Brown' }, - { key: 'cyan', text: 'Cyan' }, - { key: 'green', text: 'Green' }, - { key: 'magenta', text: 'Magenta', disabled: true }, - { key: 'mauve', text: 'Mauve' }, - { key: 'orange', text: 'Orange' }, - { key: 'pink', text: 'Pink' }, - { key: 'purple', text: 'Purple' }, - { key: 'red', text: 'Red' }, - { key: 'rose', text: 'Rose' }, - { key: 'violet', text: 'Violet' }, - { key: 'white', text: 'White' }, - { key: 'yellow', text: 'Yellow' }, + { + key: 'apple', + text: 'apple', + }, + { + key: 'aardvark', + text: 'aardvark', + }, ]; -// Optional styling to make the example look nicer -const comboBoxStyles: Partial = { root: { maxWidth: 300 } }; export const ComboBoxFreeInputExample: React.FunctionComponent = () => { + const [searchTerm, setSearchTerm] = React.useState(''); + const [selected, setSelected] = React.useState(''); + const [customPendingValueIndex, setCustomPendingValueIndex] = React.useState(-1); + + const filteredOptions = searchTerm ? options.filter(opt => opt.text.toLowerCase().startsWith(searchTerm)) : options; + + const handleChange: IComboBoxProps['onChange'] = (e, o, i, v) => { + if (o) { + setSelected(o.key.toString()); + setCustomPendingValueIndex(options.indexOf(o) ?? -1); + console.log(customPendingValueIndex); + } + if (!o && v) { + setCustomPendingValueIndex(-1); + } + }; + + function handleInputChange(o: string) { + setSearchTerm(o); + } + + function handlePendingChange(option, index, value) { + console.log(option, index, value); + } + return (
-
); diff --git a/packages/react/src/components/ComboBox/ComboBox.tsx b/packages/react/src/components/ComboBox/ComboBox.tsx index 03eb5041dd6f8e..de68bb53f93ec6 100644 --- a/packages/react/src/components/ComboBox/ComboBox.tsx +++ b/packages/react/src/components/ComboBox/ComboBox.tsx @@ -2367,8 +2367,33 @@ class ComboBoxInternal extends React.Component ({ ...item, index })); + + let currentPendingValueValidIndex = this.state.currentPendingValueValidIndex; + + // Update the pending value if the current index doesn't exist or it doesn't match the text of the value. + if ( + currentPendingValue && + (currentPendingValueValidIndex >= options.length || + (currentPendingValueValidIndex >= 0 && + options[currentPendingValueValidIndex].text.indexOf(currentPendingValue) < 0)) + ) { + let items; + if (this.props.autoComplete === 'on') { + items = options.filter( + option => !option.disabled && option.text.toLocaleLowerCase().indexOf(currentPendingValue) === 0, + ); + } else { + items = options.filter(option => !option.disabled && option.text.toLocaleLowerCase() === currentPendingValue); + } + + if (items.length > 0) { + this._setPendingInfo(currentPendingValue, items[0].index, items[0].text); + currentPendingValueValidIndex = items[0].index; + } + } + let descendantText = isOpen && selectedIndices?.length ? options[selectedIndices[0]].id ?? this._id + '-list' + selectedIndices[0] From fd45e0bc2acce093170041b0c9006b9a79ce1d5c Mon Sep 17 00:00:00 2001 From: Esteban Munoz Date: Tue, 7 Feb 2023 15:26:29 -0700 Subject: [PATCH 2/9] making _getAriaActiveDescendantValue update pending value and index --- .../ComboBox/ComboBox.FreeInput.Example.tsx | 79 ++++++++----------- .../src/components/ComboBox/ComboBox.tsx | 38 ++++----- 2 files changed, 49 insertions(+), 68 deletions(-) diff --git a/packages/react-examples/src/react/ComboBox/ComboBox.FreeInput.Example.tsx b/packages/react-examples/src/react/ComboBox/ComboBox.FreeInput.Example.tsx index 86d6e02b4db95a..39a9c919af2f9e 100644 --- a/packages/react-examples/src/react/ComboBox/ComboBox.FreeInput.Example.tsx +++ b/packages/react-examples/src/react/ComboBox/ComboBox.FreeInput.Example.tsx @@ -1,59 +1,44 @@ import * as React from 'react'; -import { ComboBox, IComboBoxProps } from '@fluentui/react'; -import type { IComboBoxOption } from '@fluentui/react'; +import { ComboBox } from '@fluentui/react'; +import type { IComboBoxOption, IComboBoxStyles } from '@fluentui/react'; const options: IComboBoxOption[] = [ - { - key: 'apple', - text: 'apple', - }, - { - key: 'aardvark', - text: 'aardvark', - }, + { key: 'black', text: 'Black' }, + { key: 'blue', text: 'Blue' }, + { key: 'brown', text: 'Brown' }, + { key: 'cyan', text: 'Cyan' }, + { key: 'green', text: 'Green' }, + { key: 'magenta', text: 'Magenta', disabled: true }, + { key: 'mauve', text: 'Mauve' }, + { key: 'orange', text: 'Orange' }, + { key: 'pink', text: 'Pink' }, + { key: 'purple', text: 'Purple' }, + { key: 'red', text: 'Red' }, + { key: 'rose', text: 'Rose' }, + { key: 'violet', text: 'Violet' }, + { key: 'white', text: 'White' }, + { key: 'yellow', text: 'Yellow' }, ]; +// Optional styling to make the example look nicer +const comboBoxStyles: Partial = { root: { maxWidth: 300 } }; export const ComboBoxFreeInputExample: React.FunctionComponent = () => { - const [searchTerm, setSearchTerm] = React.useState(''); - const [selected, setSelected] = React.useState(''); - const [customPendingValueIndex, setCustomPendingValueIndex] = React.useState(-1); - - const filteredOptions = searchTerm ? options.filter(opt => opt.text.toLowerCase().startsWith(searchTerm)) : options; - - const handleChange: IComboBoxProps['onChange'] = (e, o, i, v) => { - if (o) { - setSelected(o.key.toString()); - setCustomPendingValueIndex(options.indexOf(o) ?? -1); - console.log(customPendingValueIndex); - } - if (!o && v) { - setCustomPendingValueIndex(-1); - } - }; - - function handleInputChange(o: string) { - setSearchTerm(o); - } - - function handlePendingChange(option, index, value) { - console.log(option, index, value); - } - return (
+
); diff --git a/packages/react/src/components/ComboBox/ComboBox.tsx b/packages/react/src/components/ComboBox/ComboBox.tsx index de68bb53f93ec6..af252905095d47 100644 --- a/packages/react/src/components/ComboBox/ComboBox.tsx +++ b/packages/react/src/components/ComboBox/ComboBox.tsx @@ -801,8 +801,9 @@ class ComboBoxInternal extends React.Component { this._autoCompleteTimeout = undefined; }, ReadOnlyPendingAutoCompleteTimeout); - return; + return items[0].index; } } @@ -927,6 +930,7 @@ class ComboBoxInternal extends React.Component ({ ...item, index })); - let currentPendingValueValidIndex = this.state.currentPendingValueValidIndex; - // Update the pending value if the current index doesn't exist or it doesn't match the text of the value. + // We should check if the pending value index is withing the bounds of the options and that it matches the text of + // the pending value. If it does not match, we should update it the same way we do in _onInputChange. This is + // needed since the user may be filtering the options and the pending value and index are only updated on input + // change. if ( currentPendingValue && (currentPendingValueValidIndex >= options.length || - (currentPendingValueValidIndex >= 0 && - options[currentPendingValueValidIndex].text.indexOf(currentPendingValue) < 0)) + options[currentPendingValueValidIndex].text.indexOf(currentPendingValue) < 0) ) { - let items; - if (this.props.autoComplete === 'on') { - items = options.filter( - option => !option.disabled && option.text.toLocaleLowerCase().indexOf(currentPendingValue) === 0, - ); - } else { - items = options.filter(option => !option.disabled && option.text.toLocaleLowerCase() === currentPendingValue); - } - - if (items.length > 0) { - this._setPendingInfo(currentPendingValue, items[0].index, items[0].text); - currentPendingValueValidIndex = items[0].index; - } + currentPendingValueValidIndex = + this.props.allowFreeform || this.props.allowFreeInput + ? this._processInputChangeWithFreeform(currentPendingValue) + : this._processInputChangeWithoutFreeform(currentPendingValue); } let descendantText = From c064dafa78a1dd5dfe0a558f2bddb22ef248a8c1 Mon Sep 17 00:00:00 2001 From: Esteban Munoz Date: Tue, 7 Feb 2023 15:28:17 -0700 Subject: [PATCH 3/9] change file --- ...luentui-react-12283c65-5330-4f04-9ce2-532900d9be76.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@fluentui-react-12283c65-5330-4f04-9ce2-532900d9be76.json diff --git a/change/@fluentui-react-12283c65-5330-4f04-9ce2-532900d9be76.json b/change/@fluentui-react-12283c65-5330-4f04-9ce2-532900d9be76.json new file mode 100644 index 00000000000000..57c71772d53a01 --- /dev/null +++ b/change/@fluentui-react-12283c65-5330-4f04-9ce2-532900d9be76.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "fix(ComboBox): Make _getAriaActiveDescendantValue update current pending value and index to avoid crash when filtering options.", + "packageName": "@fluentui/react", + "email": "esteban.230@hotmail.com", + "dependentChangeType": "patch" +} From 2eea9700b8d98868e9f247cd81896cded032f949 Mon Sep 17 00:00:00 2001 From: Esteban Munoz Date: Tue, 7 Feb 2023 15:29:56 -0700 Subject: [PATCH 4/9] updating comment --- packages/react/src/components/ComboBox/ComboBox.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react/src/components/ComboBox/ComboBox.tsx b/packages/react/src/components/ComboBox/ComboBox.tsx index af252905095d47..53c9b071acb890 100644 --- a/packages/react/src/components/ComboBox/ComboBox.tsx +++ b/packages/react/src/components/ComboBox/ComboBox.tsx @@ -801,7 +801,7 @@ class ComboBoxInternal extends React.Component Date: Wed, 8 Feb 2023 15:06:24 -0700 Subject: [PATCH 5/9] changing to state based solution --- .../src/components/ComboBox/ComboBox.tsx | 79 +++++++++---------- 1 file changed, 39 insertions(+), 40 deletions(-) diff --git a/packages/react/src/components/ComboBox/ComboBox.tsx b/packages/react/src/components/ComboBox/ComboBox.tsx index 53c9b071acb890..fb2ccad0d30bcf 100644 --- a/packages/react/src/components/ComboBox/ComboBox.tsx +++ b/packages/react/src/components/ComboBox/ComboBox.tsx @@ -63,6 +63,12 @@ export interface IComboBoxState { /** When taking input, this will store the actual text that is being entered */ currentPendingValue?: string; + + /** + * The id of the current focused combo item, otherwise the id of the currently selected element, + * null otherwise + */ + ariaActiveDescendantValue?: string; } enum SearchDirection { @@ -347,9 +353,9 @@ class ComboBoxInternal extends React.Component ({ ...item, index })); + + // If currentOptions differs from the previous currentOptions we need to update the currentPendingValueValidIndex + // otherwise, it will be out of sync with the currentOptions. This can happen when the options are filtered. + if (!shallowCompare(prevProps.hoisted.currentOptions, currentOptions) && currentPendingValue) { + newCurrentPendingValueValidIndex = + this.props.allowFreeform || this.props.allowFreeInput + ? this._processInputChangeWithFreeform(currentPendingValue) + : this._processInputChangeWithoutFreeform(currentPendingValue); + } + + let descendantText = undefined; + + if (isOpen && selectedIndices.length) { + descendantText = options[selectedIndices[0]].id ?? this._id + '-list' + selectedIndices[0]; + } else if (isOpen && this._hasFocus() && newCurrentPendingValueValidIndex !== -1) { + descendantText = + options[newCurrentPendingValueValidIndex].id ?? this._id + '-list' + newCurrentPendingValueValidIndex; + } else { + descendantText = undefined; + } + + if (descendantText !== this.state.ariaActiveDescendantValue) { + this.setState({ + ariaActiveDescendantValue: descendantText, + }); + } } public componentWillUnmount(): void { @@ -579,7 +614,7 @@ class ComboBoxInternal extends React.Component ({ ...item, index })); - let currentPendingValueValidIndex = this.state.currentPendingValueValidIndex; - - // We should check if the pending value index is withing the bounds of the options and that it matches the text of - // the pending value. If it does not match, we should update it the same way we do in _onInputChange. This is - // needed since the user may be filtering the options and the pending value and index are only updated on input - // change. - if ( - currentPendingValue && - (currentPendingValueValidIndex >= options.length || - options[currentPendingValueValidIndex].text.indexOf(currentPendingValue) < 0) - ) { - currentPendingValueValidIndex = - this.props.allowFreeform || this.props.allowFreeInput - ? this._processInputChangeWithFreeform(currentPendingValue) - : this._processInputChangeWithoutFreeform(currentPendingValue); - } - - let descendantText = - isOpen && selectedIndices?.length - ? options[selectedIndices[0]].id ?? this._id + '-list' + selectedIndices[0] - : undefined; - if (isOpen && this._hasFocus() && currentPendingValueValidIndex !== -1) { - descendantText = options[currentPendingValueValidIndex].id ?? this._id + '-list' + currentPendingValueValidIndex; - } - return descendantText; - } - /** * Get the aria autocomplete value for the combo box * @returns 'inline' if auto-complete automatically dynamic, 'both' if we have a list of possible values to pick from From 78cc213a0bf4d580da26436632fe564cc31a2127 Mon Sep 17 00:00:00 2001 From: Esteban Munoz Date: Wed, 8 Feb 2023 15:19:48 -0700 Subject: [PATCH 6/9] update api --- .../@fluentui-react-12283c65-5330-4f04-9ce2-532900d9be76.json | 2 +- packages/react/etc/react.api.md | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/change/@fluentui-react-12283c65-5330-4f04-9ce2-532900d9be76.json b/change/@fluentui-react-12283c65-5330-4f04-9ce2-532900d9be76.json index 57c71772d53a01..71a91954cbca2e 100644 --- a/change/@fluentui-react-12283c65-5330-4f04-9ce2-532900d9be76.json +++ b/change/@fluentui-react-12283c65-5330-4f04-9ce2-532900d9be76.json @@ -1,6 +1,6 @@ { "type": "patch", - "comment": "fix(ComboBox): Make _getAriaActiveDescendantValue update current pending value and index to avoid crash when filtering options.", + "comment": "fix(ComboBox): fix(react-combobox): Remove _getAriaActiveDescendantValue, compute aria-activedescendantvalue in state, and update currentPendingValue when the options change.", "packageName": "@fluentui/react", "email": "esteban.230@hotmail.com", "dependentChangeType": "patch" diff --git a/packages/react/etc/react.api.md b/packages/react/etc/react.api.md index d1131b3e0f7319..9877773a5c1a82 100644 --- a/packages/react/etc/react.api.md +++ b/packages/react/etc/react.api.md @@ -3943,6 +3943,7 @@ export interface IComboBoxProps extends ISelectableDroppableTextProps Date: Fri, 10 Feb 2023 11:58:43 -0800 Subject: [PATCH 7/9] fix errors --- .../src/components/ComboBox/ComboBox.tsx | 52 +++++++++++-------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/packages/react/src/components/ComboBox/ComboBox.tsx b/packages/react/src/components/ComboBox/ComboBox.tsx index f7cc3b7e4157d5..2521d2b703394b 100644 --- a/packages/react/src/components/ComboBox/ComboBox.tsx +++ b/packages/react/src/components/ComboBox/ComboBox.tsx @@ -420,13 +420,13 @@ class ComboBoxInternal extends React.Component ({ ...item, index: i })) - - .filter( - option => - isNormalOption(option) && !option.disabled && option.text.toLocaleLowerCase().indexOf(updatedValue) === 0, - ); - - // If we found a match, update the state - if (items.length > 0) { - this._setPendingInfo(originalUpdatedValue, items[0].index, getPreviewText(items[0])); - } + const matchingIndex = this._updateAutocompleteIndexWithoutFreeform(updatedValue); // Schedule a timeout to clear the pending value after the timeout span this._autoCompleteTimeout = this._async.setTimeout(() => { this._autoCompleteTimeout = undefined; }, ReadOnlyPendingAutoCompleteTimeout); - return items[0].index; + + return matchingIndex; } } - // If we get here, either autoComplete is off or we did not find a match with autoComplete on. + // If we get here, autoComplete is off. // Remember we are not allowing freeform, so at this point, if we have a pending valid value index // use that; otherwise use the selectedIndex const index = currentPendingValueValidIndex >= 0 ? currentPendingValueValidIndex : this._getFirstSelectedIndex(); @@ -968,6 +953,31 @@ class ComboBoxInternal extends React.Component ({ ...item, index: i })) + + .filter( + option => + isNormalOption(option) && !option.disabled && option.text.toLocaleLowerCase().indexOf(updatedValue) === 0, + ); + + // If we found a match, update the state + if (items.length > 0) { + this._setPendingInfo(originalUpdatedValue, items[0].index, getPreviewText(items[0])); + return items[0].index; + } + + const index = this._getFirstSelectedIndex(); + this._setPendingInfoFromIndex(index); + return index; + } + private _getFirstSelectedIndex(): number { const { selectedIndices } = this.props.hoisted; return selectedIndices?.length ? selectedIndices[0] : -1; From 687080b10c3965696e15503bf61bc66a4edca623 Mon Sep 17 00:00:00 2001 From: Esteban Munoz Date: Fri, 10 Feb 2023 13:44:48 -0700 Subject: [PATCH 8/9] restoring previous behavior --- packages/react/src/components/ComboBox/ComboBox.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/react/src/components/ComboBox/ComboBox.tsx b/packages/react/src/components/ComboBox/ComboBox.tsx index 2521d2b703394b..afadc877b591a3 100644 --- a/packages/react/src/components/ComboBox/ComboBox.tsx +++ b/packages/react/src/components/ComboBox/ComboBox.tsx @@ -973,9 +973,7 @@ class ComboBoxInternal extends React.Component Date: Fri, 17 Feb 2023 10:49:01 -0700 Subject: [PATCH 9/9] requested changes --- packages/react/src/components/ComboBox/ComboBox.tsx | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/react/src/components/ComboBox/ComboBox.tsx b/packages/react/src/components/ComboBox/ComboBox.tsx index afadc877b591a3..8f1426a7fe05ef 100644 --- a/packages/react/src/components/ComboBox/ComboBox.tsx +++ b/packages/react/src/components/ComboBox/ComboBox.tsx @@ -353,9 +353,9 @@ class ComboBoxInternal extends React.Component