From 2bd89ed7e76a6e6372f931f1ce4147f2e7716880 Mon Sep 17 00:00:00 2001 From: Gururajj77 Date: Mon, 7 Oct 2024 15:30:32 +0530 Subject: [PATCH 1/5] fix: filterable multiselect readonly state implemented --- .../MultiSelect/FilterableMultiSelect.tsx | 56 +++++++++++++++++-- .../__tests__/FilterableMultiSelect-test.js | 23 ++++++++ 2 files changed, 74 insertions(+), 5 deletions(-) diff --git a/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx b/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx index eef35bc96692..4dfcfb22a5f9 100644 --- a/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx +++ b/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx @@ -257,6 +257,11 @@ export interface FilterableMultiSelectProps */ placeholder?: string; + /** + * Whether or not the filterable multiselect is readonly + */ + readOnly?: boolean; + /** * Specify feedback (mode) of the selection. * `top`: selected item jumps to top @@ -334,6 +339,7 @@ const FilterableMultiSelect = React.forwardRef(function FilterableMultiSelect< onChange, onMenuChange, placeholder, + readOnly, titleText, type, selectionFeedback = 'top-after-reopen', @@ -504,9 +510,11 @@ const FilterableMultiSelect = React.forwardRef(function FilterableMultiSelect< }; function handleMenuChange(forceIsOpen: boolean): void { - const nextIsOpen = forceIsOpen ?? !isOpen; - setIsOpen(nextIsOpen); - validateHighlightFocus(); + if (!readOnly) { + const nextIsOpen = forceIsOpen ?? !isOpen; + setIsOpen(nextIsOpen); + validateHighlightFocus(); + } } useEffect(() => { @@ -688,6 +696,7 @@ const FilterableMultiSelect = React.forwardRef(function FilterableMultiSelect< [`${prefix}--multi-select--selected`]: controlledSelectedItems?.length > 0, [`${prefix}--multi-select--filterable--input-focused`]: inputFocused, + [`${prefix}--multi-select--readonly`]: readOnly, } ); @@ -699,6 +708,14 @@ const FilterableMultiSelect = React.forwardRef(function FilterableMultiSelect< handleMenuChange(!isOpen); textInput.current?.focus(); }, + // onClick: (event) => { + // if (!readOnly) { + // handleMenuChange(!isOpen); + // textInput.current?.focus(); + // } else { + // event.preventDefault(); + // } + // }, // When we moved the "root node" of Downshift to the for // ARIA 1.2 compliance, we unfortunately hit this branch for the // "mouseup" event that downshift listens to: @@ -727,7 +744,7 @@ const FilterableMultiSelect = React.forwardRef(function FilterableMultiSelect< placeholder, preventKeyAction: isOpen, - onClick: () => handleMenuChange(true), + onClick: () => !readOnly && handleMenuChange(true), onKeyDown(event: KeyboardEvent) { const $input = event.target as HTMLInputElement; const $value = $input.value; @@ -736,6 +753,11 @@ const FilterableMultiSelect = React.forwardRef(function FilterableMultiSelect< event.stopPropagation(); } + if (readOnly) { + event.preventDefault(); + return; + } + if (match(event, keys.Enter)) { handleMenuChange(true); } @@ -797,6 +819,28 @@ const FilterableMultiSelect = React.forwardRef(function FilterableMultiSelect< } }; + const mergedRef = mergeRefs(textInput, inputProps.ref); + + const readOnlyEventHandlers = readOnly + ? { + onClick: (evt: React.MouseEvent) => { + // NOTE: does not prevent click + evt.preventDefault(); + // focus on the element as per readonly input behavior + if (mergedRef.current !== undefined) { + mergedRef.current.focus(); + } + }, + onKeyDown: (evt: React.KeyboardEvent) => { + const selectAccessKeys = ['ArrowDown', 'ArrowUp', ' ', 'Enter']; + // This prevents the select from opening for the above keys + if (selectAccessKeys.includes(evt.key)) { + evt.preventDefault(); + } + }, + } + : {}; + const clearSelectionContent = controlledSelectedItems.length > 0 ? ( @@ -831,7 +875,7 @@ const FilterableMultiSelect = React.forwardRef(function FilterableMultiSelect< invalidText={invalidText} warn={warn} warnText={warnText} - isOpen={isOpen} + isOpen={!readOnly && isOpen} size={size}>
{invalid && ( diff --git a/packages/react/src/components/MultiSelect/__tests__/FilterableMultiSelect-test.js b/packages/react/src/components/MultiSelect/__tests__/FilterableMultiSelect-test.js index a6af8ad0e00d..0317c7940f95 100644 --- a/packages/react/src/components/MultiSelect/__tests__/FilterableMultiSelect-test.js +++ b/packages/react/src/components/MultiSelect/__tests__/FilterableMultiSelect-test.js @@ -7,6 +7,7 @@ import React from 'react'; import { act, render, screen } from '@testing-library/react'; +import { getByText } from '@carbon/test-utils/dom'; import userEvent from '@testing-library/user-event'; import FilterableMultiSelect from '../FilterableMultiSelect'; import { @@ -63,6 +64,28 @@ describe('FilterableMultiSelect', () => { expect(mockProps.onMenuChange).toHaveBeenCalledWith(false); }); + it('should not be interactive if readonly', async () => { + const items = generateItems(4, generateGenericItem); + const label = 'test-label'; + const { container } = render( + + ); + await waitForPosition(); + + // eslint-disable-next-line testing-library/prefer-screen-queries + const labelNode = getByText(container, label); + await userEvent.click(labelNode); + + expect( + // eslint-disable-next-line testing-library/no-container, testing-library/no-node-access + container.querySelector('[aria-expanded="true"][aria-haspopup="listbox"]') + ).toBeFalsy(); + }); it('should initially have the menu open when open prop is provided', async () => { render(); await waitForPosition(); From 98bf3c2f184026349d29bf4513c309c3bbfaa42c Mon Sep 17 00:00:00 2001 From: Gururajj77 Date: Mon, 7 Oct 2024 19:19:35 +0530 Subject: [PATCH 2/5] fix: reviewed changes added --- .../MultiSelect/FilterableMultiSelect.tsx | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx b/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx index 253cab17b49b..b67ddb3d0069 100644 --- a/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx +++ b/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx @@ -709,14 +709,6 @@ const FilterableMultiSelect = React.forwardRef(function FilterableMultiSelect< handleMenuChange(!isOpen); textInput.current?.focus(); }, - // onClick: (event) => { - // if (!readOnly) { - // handleMenuChange(!isOpen); - // textInput.current?.focus(); - // } else { - // event.preventDefault(); - // } - // }, // When we moved the "root node" of Downshift to the for // ARIA 1.2 compliance, we unfortunately hit this branch for the // "mouseup" event that downshift listens to: @@ -754,11 +746,6 @@ const FilterableMultiSelect = React.forwardRef(function FilterableMultiSelect< event.stopPropagation(); } - if (readOnly) { - event.preventDefault(); - return; - } - if (match(event, keys.Enter)) { handleMenuChange(true); } @@ -897,7 +884,7 @@ const FilterableMultiSelect = React.forwardRef(function FilterableMultiSelect< From ff3ccbf62dfb689fd20815544dda4cf2f5625aaa Mon Sep 17 00:00:00 2001 From: Gururajj77 Date: Fri, 11 Oct 2024 08:27:39 +0530 Subject: [PATCH 3/5] fix: resovled pr review comments --- .../react/src/components/MultiSelect/FilterableMultiSelect.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx b/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx index b67ddb3d0069..7601c5bb1f5b 100644 --- a/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx +++ b/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx @@ -737,7 +737,7 @@ const FilterableMultiSelect = React.forwardRef(function FilterableMultiSelect< placeholder, preventKeyAction: isOpen, - onClick: () => !readOnly && handleMenuChange(true), + onClick: () => handleMenuChange(true), onKeyDown(event: KeyboardEvent) { const $input = event.target as HTMLInputElement; const $value = $input.value; From b6552db6017eabc19faab4a074b8befc0fee218e Mon Sep 17 00:00:00 2001 From: Gururajj77 Date: Tue, 15 Oct 2024 20:20:38 +0530 Subject: [PATCH 4/5] feat: added readonly state for listboxselection --- .../ListBox/next/ListBoxSelection.tsx | 26 ++++++++++++++----- .../MultiSelect/FilterableMultiSelect.tsx | 2 ++ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/packages/react/src/components/ListBox/next/ListBoxSelection.tsx b/packages/react/src/components/ListBox/next/ListBoxSelection.tsx index 726c84810d8a..a74a3dcfc71d 100644 --- a/packages/react/src/components/ListBox/next/ListBoxSelection.tsx +++ b/packages/react/src/components/ListBox/next/ListBoxSelection.tsx @@ -32,6 +32,7 @@ const defaultTranslations: Record = { function defaultTranslateWithId(id: TranslationKey): string { return defaultTranslations[id]; } + export interface ListBoxSelectionProps { /** * Specify a function to be invoked when a user interacts with the clear @@ -57,6 +58,11 @@ export interface ListBoxSelectionProps { * Specify whether or not the clear selection element should be disabled */ disabled?: boolean; + /** + * Whether or not the listbox is readonly + */ + readOnly?: boolean; + /** * Specify an optional `onClearSelection` handler that is called when the underlying * element is cleared @@ -86,6 +92,7 @@ function ListBoxSelection({ selectionCount, translateWithId: t = defaultTranslateWithId, disabled, + readOnly, onClearSelection, ...rest }: ListBoxSelectionProps) { @@ -100,13 +107,13 @@ function ListBoxSelection({ `${prefix}--tag--filter`, `${prefix}--tag--high-contrast`, { - [`${prefix}--tag--disabled`]: disabled, + [`${prefix}--tag--disabled`]: disabled || readOnly, } ); function onClick(event: React.MouseEvent) { event.stopPropagation(); - if (disabled) { + if (disabled || readOnly) { return; } clearSelection(event); @@ -126,11 +133,12 @@ function ListBoxSelection({
@@ -142,11 +150,12 @@ function ListBoxSelection({ {...rest} aria-label={description} className={className} - disabled={disabled} + disabled={disabled || readOnly} onClick={onClick} tabIndex={-1} title={description} - type="button"> + type="button" + aria-disabled={readOnly ? true : undefined}> ); @@ -164,6 +173,11 @@ ListBoxSelection.propTypes = { */ disabled: PropTypes.bool, + /** + * Whether or not the listbox is readonly + */ + readOnly: PropTypes.bool, + /** * Specify an optional `onClearSelection` handler that is called when the underlying * element is cleared diff --git a/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx b/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx index 7601c5bb1f5b..c7df0543ab0d 100644 --- a/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx +++ b/packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx @@ -870,6 +870,7 @@ const FilterableMultiSelect = React.forwardRef(function FilterableMultiSelect< ref={autoAlign ? refs.setReference : null}> {controlledSelectedItems.length > 0 && ( { clearSelection(); if (textInput.current) { @@ -901,6 +902,7 @@ const FilterableMultiSelect = React.forwardRef(function FilterableMultiSelect< clearSelection={clearInputValue} disabled={disabled} translateWithId={translateWithId} + readOnly={readOnly} onMouseUp={(event: MouseEvent) => { // If we do not stop this event from propagating, // it seems like Downshift takes our event and From dea3128eb095651e93519c3ccb51e1598dcad5ff Mon Sep 17 00:00:00 2001 From: Gururajj77 Date: Fri, 18 Oct 2024 16:35:49 +0530 Subject: [PATCH 5/5] fix: removed conditional class on readonly matching disabled --- packages/react/src/components/ListBox/next/ListBoxSelection.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/components/ListBox/next/ListBoxSelection.tsx b/packages/react/src/components/ListBox/next/ListBoxSelection.tsx index a74a3dcfc71d..9e6059b5ef2d 100644 --- a/packages/react/src/components/ListBox/next/ListBoxSelection.tsx +++ b/packages/react/src/components/ListBox/next/ListBoxSelection.tsx @@ -107,7 +107,7 @@ function ListBoxSelection({ `${prefix}--tag--filter`, `${prefix}--tag--high-contrast`, { - [`${prefix}--tag--disabled`]: disabled || readOnly, + [`${prefix}--tag--disabled`]: disabled, } );