From a2e7df7ae63b1045ea07ca8184001f4eb0c4e80e Mon Sep 17 00:00:00 2001 From: Joan Perals Date: Tue, 1 Oct 2024 15:37:37 +0200 Subject: [PATCH] fix: Hide disabled reason tooltip when the option is scrolled away (#2791) --- pages/multiselect/disabled-reason.page.tsx | 10 ++ pages/select/disabled-reason.page.tsx | 23 ++- src/internal/components/tooltip/index.tsx | 3 + src/popover/__tests__/positions.test.ts | 60 +++++++ src/popover/container.tsx | 33 ++-- src/popover/interfaces.ts | 5 + src/popover/use-popover-position.ts | 33 +++- src/popover/utils/positions.ts | 9 +- src/select/__integ__/disabled-reason.test.ts | 76 +++++++++ src/select/__tests__/common.ts | 30 ++++ src/select/__tests__/disabled.test.tsx | 160 +++++++++++++++++++ src/select/__tests__/select.test.tsx | 125 +-------------- src/select/parts/item.tsx | 1 + src/select/parts/multiselect-item.tsx | 1 + 14 files changed, 413 insertions(+), 156 deletions(-) create mode 100644 src/select/__integ__/disabled-reason.test.ts create mode 100644 src/select/__tests__/common.ts create mode 100644 src/select/__tests__/disabled.test.tsx diff --git a/pages/multiselect/disabled-reason.page.tsx b/pages/multiselect/disabled-reason.page.tsx index ee70a31aac..f9792f841f 100644 --- a/pages/multiselect/disabled-reason.page.tsx +++ b/pages/multiselect/disabled-reason.page.tsx @@ -7,6 +7,14 @@ import Multiselect, { MultiselectProps } from '~components/multiselect'; import ScreenshotArea from '../utils/screenshot-area'; +const extraOptions = [...Array(30).keys()].map(n => { + const numberToDisplay = (n + 5).toString(); + return { + value: numberToDisplay, + label: `Option ${n + 5}`, + }; +}); + const options: MultiselectProps.Options = [ { value: 'first', label: 'Simple' }, { value: 'second', label: 'With small icon', iconName: 'folder' }, @@ -24,6 +32,8 @@ const options: MultiselectProps.Options = [ disabled: true, disabledReason: 'disabled reason', }, + ...extraOptions, + { label: 'Last option', disabled: true, disabledReason: 'disabled reason' }, ]; export default function MultiselectPage() { diff --git a/pages/select/disabled-reason.page.tsx b/pages/select/disabled-reason.page.tsx index 386632780e..4e10b5e46b 100644 --- a/pages/select/disabled-reason.page.tsx +++ b/pages/select/disabled-reason.page.tsx @@ -7,18 +7,17 @@ import Select from '~components/select'; import ScreenshotArea from '../utils/screenshot-area'; -const options = [ - { - value: '1', - label: 'Option 1', - disabled: true, - disabledReason: 'disabled reason', - }, - { - value: '2', - label: 'Option 2', - }, -]; +const options = [...Array(50).keys()].map(n => { + const numberToDisplay = (n + 1).toString(); + const baseOption = { + value: numberToDisplay, + label: `Option ${numberToDisplay}`, + }; + if (n === 0 || n === 24 || n === 49) { + return { ...baseOption, disabled: true, disabledReason: 'disabled reason' }; + } + return baseOption; +}); export default function SelectPage() { return ( diff --git a/src/internal/components/tooltip/index.tsx b/src/internal/components/tooltip/index.tsx index 95d790b44c..c2fd98a9ac 100644 --- a/src/internal/components/tooltip/index.tsx +++ b/src/internal/components/tooltip/index.tsx @@ -20,6 +20,7 @@ export interface TooltipProps { className?: string; contentAttributes?: React.HTMLAttributes; size?: PopoverProps['size']; + hideOnOverscroll?: boolean; } export default function Tooltip({ @@ -30,6 +31,7 @@ export default function Tooltip({ contentAttributes = {}, position = 'top', size = 'small', + hideOnOverscroll, }: TooltipProps) { if (!trackKey && (typeof value === 'string' || typeof value === 'number')) { trackKey = value; @@ -48,6 +50,7 @@ export default function Tooltip({ position={position} zIndex={7000} arrow={position => } + hideOnOverscroll={hideOnOverscroll} > {value} diff --git a/src/popover/__tests__/positions.test.ts b/src/popover/__tests__/positions.test.ts index 7969db93a1..1b3793699e 100644 --- a/src/popover/__tests__/positions.test.ts +++ b/src/popover/__tests__/positions.test.ts @@ -3,6 +3,7 @@ import { calculatePosition, intersectRectangles, + isCenterOutside, PRIORITY_MAPPING, } from '../../../lib/components/popover/utils/positions'; @@ -199,3 +200,62 @@ describe('intersectRectangles', () => { expect(intersectRectangles([])).toEqual(null); }); }); + +describe('isCenterOutside', () => { + const parentRect = { + blockSize: 50, + inlineSize: 10, + insetBlockStart: 15, + insetBlockEnd: 65, + insetInlineStart: 0, + insetInlineEnd: 10, + }; + + test('returns true if the block-level center of the first rect is smaller than the block-level start of the second rect', () => { + expect( + isCenterOutside( + { + blockSize: 20, + inlineSize: 10, + insetBlockStart: 0, + insetBlockEnd: 20, + insetInlineStart: 0, + insetInlineEnd: 10, + }, + parentRect + ) + ).toBe(true); + }); + + test('returns true if the block-level center of the first rect is bigger than the block-level start of the second rect', () => { + expect( + isCenterOutside( + { + blockSize: 20, + inlineSize: 10, + insetBlockStart: 60, + insetBlockEnd: 80, + insetInlineStart: 0, + insetInlineEnd: 10, + }, + parentRect + ) + ).toBe(true); + }); + + test('returns false if the block-level center of the first rect is between the block-level start and the block-level end of the second rect', () => { + expect( + isCenterOutside( + { + blockSize: 20, + inlineSize: 10, + insetBlockStart: 10, + insetBlockEnd: 30, + insetInlineStart: 0, + insetInlineEnd: 10, + }, + parentRect + ) + ).toBe(false); + }); +}); diff --git a/src/popover/container.tsx b/src/popover/container.tsx index fd86aebf54..5988f08da5 100644 --- a/src/popover/container.tsx +++ b/src/popover/container.tsx @@ -39,6 +39,8 @@ export interface PopoverContainerProps { // Do not use this if the popover is open on hover, in order to avoid unexpected movement. allowScrollToFit?: boolean; allowVerticalOverflow?: boolean; + // Whether the popover should be hidden when the trigger is scrolled away. + hideOnOverscroll?: boolean; } export default function PopoverContainer({ @@ -55,6 +57,7 @@ export default function PopoverContainer({ keepPosition, allowScrollToFit, allowVerticalOverflow, + hideOnOverscroll, }: PopoverContainerProps) { const bodyRef = useRef(null); const contentRef = useRef(null); @@ -64,18 +67,20 @@ export default function PopoverContainer({ const isRefresh = useVisualRefresh(); // Updates the position handler. - const { updatePositionHandler, popoverStyle, internalPosition, positionHandlerRef } = usePopoverPosition({ - popoverRef, - bodyRef, - arrowRef, - trackRef, - contentRef, - allowScrollToFit, - allowVerticalOverflow, - preferredPosition: position, - renderWithPortal, - keepPosition, - }); + const { updatePositionHandler, popoverStyle, internalPosition, positionHandlerRef, isOverscrolling } = + usePopoverPosition({ + popoverRef, + bodyRef, + arrowRef, + trackRef, + contentRef, + allowScrollToFit, + allowVerticalOverflow, + preferredPosition: position, + renderWithPortal, + keepPosition, + hideOnOverscroll, + }); // Recalculate position when properties change. useLayoutEffect(() => { @@ -124,9 +129,9 @@ export default function PopoverContainer({ window.removeEventListener('resize', updatePositionOnResize); window.removeEventListener('scroll', refreshPosition, true); }; - }, [keepPosition, positionHandlerRef, trackRef, updatePositionHandler]); + }, [hideOnOverscroll, keepPosition, positionHandlerRef, trackRef, updatePositionHandler]); - return ( + return isOverscrolling ? null : (
; bodyRef: React.RefObject; @@ -37,14 +38,18 @@ export default function usePopoverPosition({ preferredPosition: PopoverProps.Position; renderWithPortal?: boolean; keepPosition?: boolean; + hideOnOverscroll?: boolean; }) { const previousInternalPositionRef = useRef(null); const [popoverStyle, setPopoverStyle] = useState>({}); const [internalPosition, setInternalPosition] = useState(null); + const [isOverscrolling, setIsOverscrolling] = useState(false); // Store the handler in a ref so that it can still be replaced from outside of the listener closure. const positionHandlerRef = useRef<() => void>(() => {}); + const scrollableContainerRectRef = useRef(null); + const updatePositionHandler = useCallback( (onContentResize = false) => { if (!trackRef.current || !popoverRef.current || !bodyRef.current || !contentRef.current || !arrowRef.current) { @@ -152,15 +157,32 @@ export default function usePopoverPosition({ scrollRectangleIntoView(rect, scrollableParent); } + if (hideOnOverscroll && trackRef.current instanceof HTMLElement) { + const scrollableContainer = getFirstScrollableParent(trackRef.current); + if (scrollableContainer) { + scrollableContainerRectRef.current = getLogicalBoundingClientRect(scrollableContainer); + } + } + positionHandlerRef.current = () => { + const trackRect = getLogicalBoundingClientRect(track); + const newTrackOffset = toRelativePosition( - getLogicalBoundingClientRect(track), + trackRect, containingBlock ? getLogicalBoundingClientRect(containingBlock) : viewportRect ); + setPopoverStyle({ insetBlockStart: newTrackOffset.insetBlockStart + trackRelativeOffset.insetBlockStart, insetInlineStart: newTrackOffset.insetInlineStart + trackRelativeOffset.insetInlineStart, }); + + if (hideOnOverscroll && scrollableContainerRectRef.current) { + // Assuming the arrow tip is at the vertical center of the popover trigger. + // This is good enough for disabled reason tooltip in select and multiselect. + // Can be further refined to take the exact arrow position into account if hideOnOverscroll is to be used in other cases. + setIsOverscrolling(isCenterOutside(trackRect, scrollableContainerRectRef.current)); + } }; }, [ @@ -170,13 +192,14 @@ export default function usePopoverPosition({ contentRef, arrowRef, keepPosition, - allowScrollToFit, preferredPosition, renderWithPortal, allowVerticalOverflow, + allowScrollToFit, + hideOnOverscroll, ] ); - return { updatePositionHandler, popoverStyle, internalPosition, positionHandlerRef }; + return { updatePositionHandler, popoverStyle, internalPosition, positionHandlerRef, isOverscrolling }; } function getBorderWidth(element: HTMLElement) { diff --git a/src/popover/utils/positions.ts b/src/popover/utils/positions.ts index d239173bd7..82dbd4d3d3 100644 --- a/src/popover/utils/positions.ts +++ b/src/popover/utils/positions.ts @@ -1,6 +1,6 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -import { BoundingBox, Dimensions, InternalPosition, PopoverProps } from '../interfaces'; +import { BoundingBox, Dimensions, InternalPosition, PopoverProps, Rect } from '../interfaces'; // A structure describing how the popover should be positioned interface CalculatedPosition { @@ -336,3 +336,10 @@ export function getDimensions(element: HTMLElement) { function isTopOrBottom(internalPosition: InternalPosition) { return ['top', 'bottom'].includes(internalPosition.split('-')[0]); } + +export function isCenterOutside(child: Rect, parent: Rect) { + const childCenter = child.insetBlockStart + child.blockSize / 2; + const overflowsBlockStart = childCenter < parent.insetBlockStart; + const overflowsBlockEnd = childCenter > parent.insetBlockEnd; + return overflowsBlockStart || overflowsBlockEnd; +} diff --git a/src/select/__integ__/disabled-reason.test.ts b/src/select/__integ__/disabled-reason.test.ts new file mode 100644 index 0000000000..b7ed4c0eff --- /dev/null +++ b/src/select/__integ__/disabled-reason.test.ts @@ -0,0 +1,76 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 +import useBrowser from '@cloudscape-design/browser-test-tools/use-browser'; + +import createWrapper from '../../../lib/components/test-utils/selectors'; +import SelectPageObject from './page-objects/select-page'; + +function setupTest(testFn: (page: SelectPageObject) => Promise) { + return useBrowser(async browser => { + await browser.url('/#/light/select/disabled-reason'); + const select = createWrapper().findSelect(); + const page = new SelectPageObject(browser, select); + await testFn(page); + }); +} + +describe('Disabled reasons', () => { + test( + 'shows disabled reason on hover', + setupTest(async page => { + const select = createWrapper().findSelect(); + await page.clickSelect(); + await page.assertDropdownOpen(true); + const firstDisabledOption = select.findDropdown().findOption(1); + await page.hoverElement(firstDisabledOption.toSelector()); + const disabledTooltip = firstDisabledOption.findDisabledReason(); + await page.waitForJsTimers(); + expect(await page.isDisplayed(disabledTooltip.toSelector())).toBe(true); + }) + ); + + describe('hides disabled reason when the disabled option is scrolled out of view', () => { + test( + 'by scrolling down from the top', + setupTest(async page => { + const select = createWrapper().findSelect(); + await page.clickSelect(); + await page.assertDropdownOpen(true); + const dropdown = select.findDropdown(); + const disabledOption = select.findDropdown().findOption(1); + const disabledOptionToolipSelector = disabledOption.findDisabledReason().toSelector(); + + await page.hoverElement(disabledOption.toSelector()); + expect(await page.isDisplayed(disabledOptionToolipSelector)).toBe(true); + await page.elementScrollTo(dropdown.findOptionsContainer().toSelector(), { top: 500 }); + await page.waitForJsTimers(); + expect(await page.isDisplayed(disabledOptionToolipSelector)).toBe(false); + await page.elementScrollTo(dropdown.findOptionsContainer().toSelector(), { top: 0 }); + await page.waitForJsTimers(); + expect(await page.isDisplayed(disabledOptionToolipSelector)).toBe(true); + }) + ); + + test( + 'by scrolling up from the bottom', + setupTest(async page => { + const select = createWrapper().findSelect(); + await page.clickSelect(); + await page.assertDropdownOpen(true); + const dropdown = select.findDropdown(); + const disabledOption = select.findDropdown().findOption(50); + const disabledOptionToolipSelector = disabledOption.findDisabledReason().toSelector(); + + await page.elementScrollTo(dropdown.findOptionsContainer().toSelector(), { top: 1000 }); + await page.hoverElement(disabledOption.toSelector()); + expect(await page.isDisplayed(disabledOptionToolipSelector)).toBe(true); + await page.elementScrollTo(dropdown.findOptionsContainer().toSelector(), { top: 500 }); + await page.waitForJsTimers(); + expect(await page.isDisplayed(disabledOptionToolipSelector)).toBe(false); + await page.elementScrollTo(dropdown.findOptionsContainer().toSelector(), { top: 1000 }); + await page.waitForJsTimers(); + expect(await page.isDisplayed(disabledOptionToolipSelector)).toBe(true); + }) + ); + }); +}); diff --git a/src/select/__tests__/common.ts b/src/select/__tests__/common.ts new file mode 100644 index 0000000000..c8e75058b7 --- /dev/null +++ b/src/select/__tests__/common.ts @@ -0,0 +1,30 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 +import { SelectProps } from '../../../lib/components/select'; + +export const VALUE_WITH_SPECIAL_CHARS = 'Option 4, test"2'; + +export const defaultOptions: SelectProps.Options = [ + { label: 'First', value: '1' }, + { label: 'Second', value: '2' }, + { + label: 'Group', + options: [ + { + label: 'Third', + value: '3', + lang: 'de', + }, + { + label: 'Forth', + value: VALUE_WITH_SPECIAL_CHARS, + }, + ], + }, +]; + +export const defaultProps = { + options: defaultOptions, + selectedOption: null, + onChange: () => {}, +}; diff --git a/src/select/__tests__/disabled.test.tsx b/src/select/__tests__/disabled.test.tsx new file mode 100644 index 0000000000..607609a19a --- /dev/null +++ b/src/select/__tests__/disabled.test.tsx @@ -0,0 +1,160 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 +import * as React from 'react'; +import { render, waitFor } from '@testing-library/react'; + +import { KeyCode } from '@cloudscape-design/test-utils-core/utils'; + +import Select, { SelectProps } from '../../../lib/components/select'; +import createWrapper from '../../../lib/components/test-utils/dom'; +import { defaultOptions, defaultProps } from './common'; + +// Mocks for the disabled reason tooltip popover +jest.mock('../../../lib/components/popover/utils/positions', () => { + const originalModule = jest.requireActual('../../../lib/components/popover/utils/positions'); + return { + ...originalModule, + getOffsetDimensions: () => ({ offsetWidth: 136, offsetHeight: 44 }), // Approximate mock dimensions for the disabled reason popover dimensions + isCenterOutside: () => true, // Simulate the trigger being scrolled away from the dropdown + }; +}); +jest.mock('../../../lib/components/internal/utils/scrollable-containers', () => { + const originalModule = jest.requireActual('../../../lib/components/internal/utils/scrollable-containers'); + return { + __esModule: true, + ...originalModule, + getFirstScrollableParent: () => createWrapper().findSelect()!.findDropdown()!.getElement(), + }; +}); + +function renderSelect(props?: Partial) { + const { container } = render(); const wrapper = createWrapper(container).findSelect()!; @@ -439,103 +413,6 @@ describe.each([false, true])('expandToViewport=%s', expandToViewport => { }); }); - describe('Disabled state', () => { - test('enabled by default', () => { - const { wrapper } = renderSelect(); - expect(wrapper.isDisabled()).toEqual(false); - wrapper.openDropdown(); - expect(wrapper.findDropdown({ expandToViewport })!.findOpenDropdown()).toBeTruthy(); - }); - - test('can be disabled', () => { - const { wrapper } = renderSelect({ disabled: true }); - expect(wrapper.isDisabled()).toEqual(true); - wrapper.openDropdown(); - expect(wrapper.findDropdown({ expandToViewport })?.findOpenDropdown()).toBeFalsy(); - }); - - describe('Disabled item with reason', () => { - test('has no tooltip open by default', () => { - const { wrapper } = renderSelect({ - options: [{ label: 'First', value: '1', disabled: true, disabledReason: 'disabled reason' }], - }); - wrapper.openDropdown(); - - expect(wrapper.findDropdown({ expandToViewport }).findOption(1)!.findDisabledReason()).toBe(null); - }); - - test('has no tooltip without disabledReason', () => { - const { wrapper } = renderSelect({ - options: [{ label: 'First', value: '1', disabled: true }], - }); - wrapper.openDropdown(); - wrapper.findTrigger()!.keydown(KeyCode.down); - - expect(wrapper.findDropdown({ expandToViewport }).findOption(1)!.findDisabledReason()).toBe(null); - }); - - test('open tooltip when the item is highlighted', () => { - const { wrapper } = renderSelect({ - options: [{ label: 'First', value: '1', disabled: true, disabledReason: 'disabled reason' }], - }); - wrapper.openDropdown(); - wrapper.findTrigger().keydown(KeyCode.down); - - expect( - wrapper.findDropdown({ expandToViewport }).findOption(1)!.findDisabledReason()!.getElement() - ).toHaveTextContent('disabled reason'); - }); - - test('has no disabledReason a11y attributes by default', () => { - const { wrapper } = renderSelect({ - options: defaultOptions, - }); - wrapper.openDropdown(); - - expect( - wrapper.findDropdown({ expandToViewport })!.find('[data-test-index="1"]')!.getElement() - ).not.toHaveAttribute('aria-describedby'); - expect(wrapper.findDropdown({ expandToViewport })!.find('[data-test-index="1"]')!.find('span[hidden]')).toBe( - null - ); - }); - - test('has disabledReason a11y attributes', () => { - const { wrapper } = renderSelect({ - options: [{ label: 'First', value: '1', disabled: true, disabledReason: 'disabled reason' }], - }); - wrapper.openDropdown(); - - expect(wrapper.findDropdown({ expandToViewport })!.find('[data-test-index="1"]')!.getElement()).toHaveAttribute( - 'aria-describedby' - ); - expect( - wrapper.findDropdown({ expandToViewport })!.find('[data-test-index="1"]')!.find('span[hidden]')!.getElement() - ).toHaveTextContent('disabled reason'); - }); - - test('can not select disabled with reason option', () => { - const onChange = jest.fn(); - const { wrapper } = renderSelect({ - options: [{ label: 'First', value: '1', disabled: true, disabledReason: 'disabled reason' }], - onChange, - }); - wrapper.openDropdown(); - wrapper.selectOptionByValue('1', { expandToViewport }); - expect(onChange).not.toHaveBeenCalled(); - }); - - test('click on disabled with reason option does not close dropdown', () => { - const { wrapper } = renderSelect({ - options: [{ label: 'First', value: '1', disabled: true, disabledReason: 'disabled reason' }], - }); - wrapper.openDropdown(); - wrapper.selectOptionByValue('1', { expandToViewport }); - expect(wrapper.findDropdown({ expandToViewport })?.findOpenDropdown()).toBeTruthy(); - }); - }); - }); - describe('Inline Label', () => { test('should render', () => { const testLabel = 'Test label'; diff --git a/src/select/parts/item.tsx b/src/select/parts/item.tsx index 926dbb09bc..267214abb9 100644 --- a/src/select/parts/item.tsx +++ b/src/select/parts/item.tsx @@ -108,6 +108,7 @@ const Item = ( trackRef={internalRef} value={disabledReason!} position="right" + hideOnOverscroll={true} /> )} diff --git a/src/select/parts/multiselect-item.tsx b/src/select/parts/multiselect-item.tsx index af0a31044c..dfa3681477 100644 --- a/src/select/parts/multiselect-item.tsx +++ b/src/select/parts/multiselect-item.tsx @@ -96,6 +96,7 @@ const MultiSelectItem = ( trackRef={internalRef} value={disabledReason!} position="right" + hideOnOverscroll={true} /> )}