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

Dropdown: VR changes and overall fixes #3967

Merged
merged 6 commits into from
Jan 31, 2025
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
8 changes: 8 additions & 0 deletions packages/gestalt/src/Dropdown.css
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,11 @@
.VRDropdownSection:not(:first-child) {
margin-top: var(--sema-space-200);
}

.hovered:hover {
background-color: var(--sema-color-hover-background-elevation);
}

.hovered:active {
background-color: var(--sema-color-pressed-background-elevation);
}
5 changes: 5 additions & 0 deletions packages/gestalt/src/Dropdown.jsdom.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,11 @@ describe('Dropdown', () => {
fireEvent.keyDown(window.document, {
keyCode: DOWN_ARROW,
});

fireEvent.keyDown(window.document, {
keyCode: ENTER,
});

// eslint-disable-next-line testing-library/no-node-access -- Please fix the next time this file is touched!
expect(document.activeElement).toHaveAttribute('href', 'https://pinterest.com/today');

Expand Down
26 changes: 21 additions & 5 deletions packages/gestalt/src/Dropdown.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -176,14 +176,20 @@ export default function Dropdown({
const [isPopoverPositioned, setIsPopoverPositioned] = useState(false);
const deviceType = useDeviceType();
const isMobile = deviceType === 'mobile';

const isInVRExperiment = useInExperiment({
webExperimentName: 'web_gestalt_visualrefresh',
mwebExperimentName: 'web_gestalt_visualrefresh',
});

const [hoveredItemIndex, setHoveredItemIndex] = useState<number | null | undefined>(
isMobile ? undefined : 0,
);

const [focusedItemIndex, setFocusedItemIndex] = useState<number | null | undefined>(
isMobile ? undefined : 0,
);

// @ts-expect-error - TS2558 - Expected 0 type arguments, but got 1.
const dropdownChildrenArray = Children.toArray<ReactNode>(children);
const allowedChildrenOptions = getChildrenOptions(dropdownChildrenArray);
Expand Down Expand Up @@ -227,7 +233,8 @@ export default function Dropdown({
if (cursorOption) {
const item = cursorOption.option;

setHoveredItemIndex(cursorIndex);
setFocusedItemIndex(cursorIndex);
setHoveredItemIndex(null);

if (direction === KEYS.ENTER && !cursorOption.disabled) {
cursorOption.onSelect?.({
Expand All @@ -241,14 +248,14 @@ export default function Dropdown({
const onKeyDown = ({ event }: { event: React.KeyboardEvent<HTMLElement> }) => {
const { keyCode } = event;
if (keyCode === UP_ARROW) {
handleKeyNavigation(event, KEYS.UP, hoveredItemIndex);
handleKeyNavigation(event, KEYS.UP, focusedItemIndex);
event.preventDefault();
} else if (keyCode === DOWN_ARROW) {
handleKeyNavigation(event, KEYS.DOWN, hoveredItemIndex);
handleKeyNavigation(event, KEYS.DOWN, focusedItemIndex);
event.preventDefault();
} else if (keyCode === ENTER) {
event.stopPropagation();
handleKeyNavigation(event, KEYS.ENTER, hoveredItemIndex);
handleKeyNavigation(event, KEYS.ENTER, focusedItemIndex);
} else if ([ESCAPE, TAB].includes(keyCode)) {
anchor?.focus();
onDismiss?.();
Expand Down Expand Up @@ -276,7 +283,9 @@ export default function Dropdown({
<DropdownContextProvider
value={{
id,
focusedItemIndex,
hoveredItemIndex,
setFocusedItemIndex,
setHoveredItemIndex,
setOptionRef,
}}
Expand Down Expand Up @@ -318,7 +327,14 @@ export default function Dropdown({
{Boolean(headerContent) && <Box padding={2}>{headerContent}</Box>}

<DropdownContextProvider
value={{ id, hoveredItemIndex, setHoveredItemIndex, setOptionRef }}
value={{
id,
hoveredItemIndex,
setHoveredItemIndex,
setFocusedItemIndex,
setOptionRef,
focusedItemIndex,
}}
>
{renderChildrenWithIndex(dropdownChildrenArray)}
</DropdownContextProvider>
Expand Down
6 changes: 5 additions & 1 deletion packages/gestalt/src/Dropdown/Context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,19 @@ import { Context, createContext } from 'react';
type DropdownContextType = {
id: string;
hoveredItemIndex: number | null | undefined;
setHoveredItemIndex: (n: number) => void;
setHoveredItemIndex: (n: number | null | undefined) => void;
setOptionRef: (arg1?: HTMLElement | null | undefined) => void;
focusedItemIndex: number | null | undefined;
setFocusedItemIndex: (n: number | null | undefined) => void;
};

const initialContextState = {
id: '',
hoveredItemIndex: -1,
setHoveredItemIndex: () => {},
setFocusedItemIndex: () => {},
setOptionRef: () => {},
focusedItemIndex: -1,
} as const;

const context: Context<DropdownContextType> =
Expand Down
3 changes: 3 additions & 0 deletions packages/gestalt/src/Dropdown/OptionItem.jsdom.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import OptionItem from './OptionItem';
describe('OptionItem', () => {
const props = {
hoveredItemIndex: null,
focusedItemIndex: null,
id: 'optionItemId',
index: 0,
option: {
Expand All @@ -12,6 +13,8 @@ describe('OptionItem', () => {
},
// @ts-expect-error - TS2344 - Type 'undefined' does not satisfy the constraint 'any[]'.
setHoveredItemIndex: jest.fn<[number], undefined>(),
// @ts-expect-error - TS2344 - Type 'undefined' does not satisfy the constraint 'any[]'.
setFocusedItemIndex: jest.fn<[number], undefined>(),
} as const;

it('Should render correctly', () => {
Expand Down
73 changes: 38 additions & 35 deletions packages/gestalt/src/Dropdown/OptionItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@ import { useRequestAnimationFrame } from '../animation/RequestAnimationFrameCont
import Badge from '../Badge';
import Box from '../Box';
import { useDeviceType } from '../contexts/DeviceTypeProvider';
import styles from '../Dropdown.css';
import Flex from '../Flex';
import focusStyles from '../Focus.css';
import getRoundingClassName from '../getRoundingClassName';
import Icon from '../Icon';
import Link from '../Link';
import styles from '../TapArea.css';
import tapAreaStyles from '../TapArea.css';
import TapAreaLink from '../TapAreaLink';
import Text from '../Text';
import { FontWeight } from '../textTypes';
import TextUI from '../TextUI';
Expand Down Expand Up @@ -43,6 +44,7 @@ type Props = {
children?: ReactNode;
dataTestId?: string;
disabled?: boolean;
focusedItemIndex: number | null | undefined;
hoveredItemIndex: number | null | undefined;
href?: string;
id: string;
Expand All @@ -56,7 +58,8 @@ type Props = {
onSelect?: (arg1: { item: OptionItemType; event: React.ChangeEvent<HTMLInputElement> }) => void;
option: OptionItemType;
selected?: OptionItemType | ReadonlyArray<OptionItemType> | null;
setHoveredItemIndex: (arg1: number) => void;
setHoveredItemIndex: (arg1: number | null | undefined) => void;
setFocusedItemIndex: (arg1: number | null | undefined) => void;
textWeight?: FontWeight;
};

Expand All @@ -68,7 +71,9 @@ const OptionItemWithForwardRef = forwardRef<HTMLElement | null | undefined, Prop
dataTestId,
disabled,
onSelect,
focusedItemIndex,
hoveredItemIndex,
setFocusedItemIndex,
href,
id,
index,
Expand All @@ -93,31 +98,25 @@ const OptionItemWithForwardRef = forwardRef<HTMLElement | null | undefined, Prop
const isSelectedItem =
matches.length > 0 || JSON.stringify(option) === JSON.stringify(selected);

const handleOnTap = (event: React.ChangeEvent<HTMLInputElement>) => {
if (!href && !children) {
event.preventDefault();
}

if (disabled) return;
onSelect?.({ event, item: option });
};

const { isFocusVisible } = useFocusVisible();

const className = classnames(getRoundingClassName(2), {
[focusStyles.hideOutline]: !isFocusVisible,
[focusStyles.accessibilityOutline]: isFocusVisible,
[focusStyles.accessibilityOutlineFocusWithin]: isFocusVisible,
[styles.fullWidth]: true,
[styles.pointer]: !disabled,
[styles.noDrop]: disabled,
});

const isInVRExperiment = useInExperiment({
webExperimentName: 'web_gestalt_visualrefresh',
mwebExperimentName: 'web_gestalt_visualrefresh',
});

const className = classnames(getRoundingClassName(2), focusStyles.hideOutlineWithin, {
[styles.hovered]: isInVRExperiment && index === hoveredItemIndex,
[focusStyles.hideOutline]: index !== focusedItemIndex || index === hoveredItemIndex,
[focusStyles.accessibilityOutlineFocus]:
!isInVRExperiment && index === focusedItemIndex && isFocusVisible,
[focusStyles.accessibilityVROutlineFocus]:
isInVRExperiment && index === focusedItemIndex && isFocusVisible,
[tapAreaStyles.fullWidth]: true,
[tapAreaStyles.pointer]: !disabled,
[tapAreaStyles.noDrop]: disabled,
});

const textColor = disabled ? 'disabled' : 'default';

const optionItemContent = (
Expand Down Expand Up @@ -148,14 +147,10 @@ const OptionItemWithForwardRef = forwardRef<HTMLElement | null | undefined, Prop
</Text>
)}
{badge && !disabled && (
<Box marginStart={2} marginTop={1}>
<Box marginStart={2} marginTop={isInVRExperiment ? undefined : 1}>
{/* Adds a pause for screen reader users between the text content */}
<Box display="visuallyHidden">{`, `}</Box>
<Badge
position={isInVRExperiment ? 'top' : undefined}
text={badge.text}
type={badge.type || 'info'}
/>
<Badge text={badge.text} type={badge.type || 'info'} />
</Box>
)}
</Fragment>
Expand Down Expand Up @@ -188,7 +183,7 @@ const OptionItemWithForwardRef = forwardRef<HTMLElement | null | undefined, Prop
size={isInVRExperiment ? 16 : 12}
/>
) : (
<Box minWidth={12} />
<Box minWidth={isInVRExperiment ? 16 : 12} />
)}
</Box>
{iconEnd && (
Expand All @@ -213,15 +208,19 @@ const OptionItemWithForwardRef = forwardRef<HTMLElement | null | undefined, Prop
return (
<div
// @ts-expect-error - TS2322 - Type 'ForwardedRef<HTMLElement | null | undefined>' is not assignable to type 'LegacyRef<HTMLDivElement> | undefined'.
ref={index === hoveredItemIndex ? ref : null}
ref={index === hoveredItemIndex || index === focusedItemIndex ? ref : null}
aria-disabled={disabled}
className={className}
data-test-id={dataTestId}
// These event.stopPropagation are important so interactive anchors don't receive the onFocus/onBlur event
id={`${id}-item-${index}`}
onBlur={(event) => event.stopPropagation()}
// @ts-expect-error - TS2322 - Type '(event: React.ChangeEvent<HTMLInputElement>) => void' is not assignable to type 'MouseEventHandler<HTMLDivElement>'.
onClick={handleOnTap}
onClick={(event: React.ChangeEvent<HTMLInputElement>) => {
if (!href && !children) event.preventDefault();
if (disabled) return;
onSelect?.({ event, item: option });
}}
// This event.stopPropagation is important so interactive anchors don't compress with the onMouseDown event
onFocus={(event) => event.stopPropagation()}
onKeyPress={(event) => {
Expand All @@ -233,6 +232,7 @@ const OptionItemWithForwardRef = forwardRef<HTMLElement | null | undefined, Prop
}}
onMouseEnter={() => {
if (!disabled) {
setFocusedItemIndex(null);
setHoveredItemIndex(index);
}
}}
Expand All @@ -241,28 +241,31 @@ const OptionItemWithForwardRef = forwardRef<HTMLElement | null | undefined, Prop
tabIndex={isMobile && !disabled ? 0 : -1}
>
<Box
color={index === hoveredItemIndex && !disabled ? 'secondary' : 'transparent'}
color={
index === hoveredItemIndex && !disabled && !isInVRExperiment
? 'secondary'
: 'transparent'
}
direction="column"
display="flex"
paddingX={isInVRExperiment ? 3 : 2}
paddingY={isInVRExperiment ? 2 : 2}
rounding={2}
>
{href && !disabled ? (
<Link
<TapAreaLink
href={href}
onClick={({ event, dangerouslyDisableOnNavigation }) =>
onTap={({ event, dangerouslyDisableOnNavigation }) =>
onClick?.({
event,
dangerouslyDisableOnNavigation,
mobileOnDismissStart: isMobile ? onExternalDismiss : () => {},
})
}
target={iconEnd === 'visit' ? 'blank' : 'self'}
underline="none"
>
{optionItemContent}
</Link>
</TapAreaLink>
) : (
optionItemContent
)}
Expand Down
11 changes: 10 additions & 1 deletion packages/gestalt/src/DropdownItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -88,19 +88,28 @@ export default function DropdownItem({
}: Props) {
return (
<DropdownContextConsumer>
{({ id, hoveredItemIndex, setHoveredItemIndex, setOptionRef }) => (
{({
id,
hoveredItemIndex,
setHoveredItemIndex,
setOptionRef,
focusedItemIndex,
setFocusedItemIndex,
}) => (
<OptionItem
key={`${option.value + _index}`}
ref={setOptionRef}
badge={badge}
dataTestId={dataTestId}
disabled={disabled}
focusedItemIndex={focusedItemIndex}
hoveredItemIndex={hoveredItemIndex}
id={id}
index={_index}
onSelect={onSelect}
option={option}
selected={selected}
setFocusedItemIndex={setFocusedItemIndex}
setHoveredItemIndex={setHoveredItemIndex}
textWeight="bold"
>
Expand Down
11 changes: 10 additions & 1 deletion packages/gestalt/src/DropdownLink.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -80,20 +80,29 @@ export default function DropdownLink({
}: Props) {
return (
<DropdownContextConsumer>
{({ id, hoveredItemIndex, setHoveredItemIndex, setOptionRef }) => (
{({
id,
hoveredItemIndex,
setHoveredItemIndex,
setOptionRef,
focusedItemIndex,
setFocusedItemIndex,
}) => (
<OptionItem
key={`${option.value + _index}`}
ref={setOptionRef}
badge={badge}
dataTestId={dataTestId}
disabled={disabled}
focusedItemIndex={focusedItemIndex}
hoveredItemIndex={hoveredItemIndex}
href={href}
iconEnd={iconEnd}
id={id}
index={_index}
onClick={onClick}
option={option}
setFocusedItemIndex={setFocusedItemIndex}
setHoveredItemIndex={setHoveredItemIndex}
textWeight="bold"
>
Expand Down
Loading
Loading