Skip to content

Commit

Permalink
refactor: No longer preventDefault in usePress and allow browser to m…
Browse files Browse the repository at this point in the history
…anage focus (#7542)

* working first pass

* Update mouse and touch handlers as well

* Always set a tabIndex on native buttons and inputs so Safari focuses them

* lint

* Update tests

* Make grid selection announcement more resilient to focus order

* Remove focus during virtual clicks

#5940

* Add test story for #7480

* Add story for #6512

* Make sure to cancel press if onClick's propagation is stopped

For #5833

* fix ts

* Use focusWithoutScrolling when restoring focus
  • Loading branch information
devongovett authored Jan 17, 2025
1 parent df3f1ea commit cdba748
Show file tree
Hide file tree
Showing 57 changed files with 886 additions and 516 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ describe('useBreadcrumbItem', function () {

it('handles descendant link with href', function () {
let {itemProps} = renderLinkHook({children: <a href="https://example.com">Breadcrumb Item</a>});
expect(itemProps.tabIndex).toBeUndefined();
expect(itemProps.tabIndex).toBe(0);
expect(itemProps.role).toBeUndefined();
expect(itemProps['aria-disabled']).toBeUndefined();
expect(typeof itemProps.onKeyDown).toBe('function');
Expand Down
1 change: 0 additions & 1 deletion packages/@react-aria/button/src/useButton.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ export function useButton(props: AriaButtonOptions<ElementType>, ref: RefObject<
} else {
additionalProps = {
role: 'button',
tabIndex: isDisabled ? undefined : 0,
href: elementType === 'a' && !isDisabled ? href : undefined,
target: elementType === 'a' ? target : undefined,
type: elementType === 'input' ? type : undefined,
Expand Down
24 changes: 12 additions & 12 deletions packages/@react-aria/dnd/test/dnd.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2466,7 +2466,7 @@ describe('useDrag and useDrop', function () {

let draggable = tree.getByText('Drag me');

fireEvent.focus(draggable);
act(() => draggable.focus());
fireEvent(draggable, pointerEvent('pointerdown', {pointerId: 1, width: 1, height: 1, pressure: 0, detail: 0}));
fireEvent(draggable, pointerEvent('pointerup', {pointerId: 1, width: 1, height: 1, pressure: 0, detail: 0}));
await user.click(draggable);
Expand Down Expand Up @@ -2496,7 +2496,7 @@ describe('useDrag and useDrop', function () {
let draggable = tree.getByText('Drag me');
let droppable = tree.getByText('Drop here');

fireEvent.focus(draggable);
act(() => draggable.focus());
fireEvent(draggable, pointerEvent('pointerdown', {pointerId: 1, width: 1, height: 1, pressure: 0, detail: 0}));
fireEvent(draggable, pointerEvent('pointerup', {pointerId: 1, width: 1, height: 1, pressure: 0, detail: 0}));
await user.click(draggable);
Expand All @@ -2505,7 +2505,7 @@ describe('useDrag and useDrop', function () {


// Android Talkback fires with click event of detail = 1, test that our onPointerDown listener detects that it is a virtual click
fireEvent.focus(droppable);
act(() => droppable.focus());
fireEvent(droppable, pointerEvent('pointerdown', {pointerId: 1, width: 1, height: 1, pressure: 0, detail: 0, pointerType: 'mouse'}));
fireEvent(droppable, pointerEvent('pointerup', {pointerId: 1, width: 1, height: 1, pressure: 0, detail: 0, pointerType: 'mouse'}));
fireEvent.click(droppable, {detail: 1});
Expand Down Expand Up @@ -2535,7 +2535,7 @@ describe('useDrag and useDrop', function () {

let draggable = tree.getByText('Drag me');

fireEvent.focus(draggable);
act(() => draggable.focus());
await user.click(draggable);
act(() => jest.runAllTimers());

Expand Down Expand Up @@ -2563,7 +2563,7 @@ describe('useDrag and useDrop', function () {

expect(tree.getAllByRole('textbox')).toHaveLength(1);

fireEvent.focus(draggable);
act(() => draggable.focus());
fireEvent.click(draggable);
act(() => jest.runAllTimers());

Expand Down Expand Up @@ -2596,7 +2596,7 @@ describe('useDrag and useDrop', function () {

let draggable = tree.getByText('Drag me');

fireEvent.focus(draggable);
act(() => draggable.focus());
await user.click(draggable);
act(() => jest.runAllTimers());

Expand All @@ -2621,7 +2621,7 @@ describe('useDrag and useDrop', function () {

let draggable = tree.getByText('Drag me');

fireEvent.focus(draggable);
act(() => draggable.focus());
await user.click(draggable);
act(() => jest.runAllTimers());

Expand All @@ -2644,7 +2644,7 @@ describe('useDrag and useDrop', function () {
let droppable = tree.getByText('Drop here');
let input = tree.getByRole('textbox');

fireEvent.focus(draggable);
act(() => draggable.focus());
await user.click(draggable);
act(() => jest.runAllTimers());

Expand All @@ -2664,7 +2664,7 @@ describe('useDrag and useDrop', function () {
let draggable = tree.getByText('Drag me');
let input = tree.getByRole('textbox');

fireEvent.focus(draggable);
act(() => draggable.focus());
await user.click(draggable);
act(() => jest.runAllTimers());

Expand All @@ -2681,7 +2681,7 @@ describe('useDrag and useDrop', function () {
let draggable = tree.getByText('Drag me');
let droppable = tree.getByText('Drop here');

fireEvent.focus(draggable);
act(() => draggable.focus());
await user.click(draggable);
act(() => jest.runAllTimers());

Expand Down Expand Up @@ -2715,7 +2715,7 @@ describe('useDrag and useDrop', function () {

let draggable = tree.getByText('Drag me');

fireEvent.focus(draggable);
act(() => draggable.focus());
fireEvent.click(draggable, {detail: 1});
act(() => jest.runAllTimers());

Expand All @@ -2731,7 +2731,7 @@ describe('useDrag and useDrop', function () {
let draggable = tree.getByText('Drag me');
let droppable = tree.getByText('Drop here');

fireEvent.focus(draggable);
act(() => draggable.focus());
await user.click(draggable);
act(() => jest.runAllTimers());
expect(draggable).toHaveAttribute('data-dragging', 'true');
Expand Down
11 changes: 6 additions & 5 deletions packages/@react-aria/dnd/test/useDraggableCollection.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -781,7 +781,8 @@ describe('useDraggableCollection', () => {

let dragButton = within(cells[1]).getByRole('button');
expect(dragButton).toHaveAttribute('aria-label', 'Drag Bar');
fireEvent.focus(dragButton);
act(() => cells[1].focus());
act(() => dragButton.focus());
fireEvent.click(dragButton);
act(() => jest.runAllTimers());
expect(cells[0]).not.toHaveClass('is-dragging');
Expand Down Expand Up @@ -852,15 +853,15 @@ describe('useDraggableCollection', () => {
let cells = within(grid).getAllByRole('gridcell');
expect(cells).toHaveLength(3);

fireEvent.focus(cells[0]);
act(() => cells[0].focus());
fireEvent.click(cells[0]);
expect(rows[0]).toHaveAttribute('aria-selected', 'true');
fireEvent.click(cells[1]);
expect(rows[1]).toHaveAttribute('aria-selected', 'true');

let dragButton = within(cells[1]).getByRole('button');
expect(dragButton).toHaveAttribute('aria-label', 'Drag 2 selected items');
fireEvent.focus(dragButton);
act(() => dragButton.focus());
fireEvent.click(dragButton);
act(() => jest.runAllTimers());
expect(cells[0]).toHaveClass('is-dragging');
Expand Down Expand Up @@ -939,13 +940,13 @@ describe('useDraggableCollection', () => {
let cells = within(grid).getAllByRole('gridcell');
expect(cells).toHaveLength(3);

fireEvent.focus(cells[0]);
act(() => cells[0].focus());
fireEvent.click(cells[0]);
expect(rows[0]).toHaveAttribute('aria-selected', 'true');

let dragButton = within(cells[1]).getByRole('button');
expect(dragButton).toHaveAttribute('aria-label', 'Drag Bar');
fireEvent.focus(dragButton);
act(() => dragButton.focus());
fireEvent.click(dragButton);
act(() => jest.runAllTimers());
expect(cells[0]).not.toHaveClass('is-dragging');
Expand Down
31 changes: 3 additions & 28 deletions packages/@react-aria/focus/src/FocusScope.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import {FocusableElement, RefObject} from '@react-types/shared';
import {focusSafely} from './focusSafely';
import {getInteractionModality} from '@react-aria/interactions';
import {getOwnerDocument, isAndroid, isChrome, useLayoutEffect} from '@react-aria/utils';
import {getOwnerDocument, isAndroid, isChrome, isFocusable, isTabbable, useLayoutEffect} from '@react-aria/utils';
import {isElementVisible} from './isElementVisible';
import React, {ReactNode, useContext, useEffect, useMemo, useRef} from 'react';

Expand Down Expand Up @@ -268,31 +268,6 @@ function createFocusManagerForScope(scopeRef: React.RefObject<Element[] | null>)
};
}

const focusableElements = [
'input:not([disabled]):not([type=hidden])',
'select:not([disabled])',
'textarea:not([disabled])',
'button:not([disabled])',
'a[href]',
'area[href]',
'summary',
'iframe',
'object',
'embed',
'audio[controls]',
'video[controls]',
'[contenteditable]:not([contenteditable^="false"])'
];

const FOCUSABLE_ELEMENT_SELECTOR = focusableElements.join(':not([hidden]),') + ',[tabindex]:not([disabled]):not([hidden])';

focusableElements.push('[tabindex]:not([tabindex="-1"]):not([disabled])');
const TABBABLE_ELEMENT_SELECTOR = focusableElements.join(':not([hidden]):not([tabindex="-1"]),');

export function isFocusable(element: HTMLElement) {
return element.matches(FOCUSABLE_ELEMENT_SELECTOR);
}

function getScopeRoot(scope: Element[]) {
return scope[0].parentElement!;
}
Expand Down Expand Up @@ -749,7 +724,7 @@ function restoreFocusToElement(node: FocusableElement) {
* that matches all focusable/tabbable elements.
*/
export function getFocusableTreeWalker(root: Element, opts?: FocusManagerOptions, scope?: Element[]) {
let selector = opts?.tabbable ? TABBABLE_ELEMENT_SELECTOR : FOCUSABLE_ELEMENT_SELECTOR;
let filter = opts?.tabbable ? isTabbable : isFocusable;
let walker = getOwnerDocument(root).createTreeWalker(
root,
NodeFilter.SHOW_ELEMENT,
Expand All @@ -760,7 +735,7 @@ export function getFocusableTreeWalker(root: Element, opts?: FocusManagerOptions
return NodeFilter.FILTER_REJECT;
}

if ((node as Element).matches(selector)
if (filter(node as Element)
&& isElementVisible(node as Element)
&& (!scope || isElementInScope(node as Element, scope))
&& (!opts?.accept || opts.accept(node as Element))
Expand Down
4 changes: 3 additions & 1 deletion packages/@react-aria/focus/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@
* governing permissions and limitations under the License.
*/

export {FocusScope, useFocusManager, getFocusableTreeWalker, createFocusManager, isElementInChildOfActiveScope, isFocusable} from './FocusScope';
export {FocusScope, useFocusManager, getFocusableTreeWalker, createFocusManager, isElementInChildOfActiveScope} from './FocusScope';
export {FocusRing} from './FocusRing';
export {FocusableProvider, useFocusable} from './useFocusable';
export {useFocusRing} from './useFocusRing';
export {focusSafely} from './focusSafely';
export {useHasTabbableChild} from './useHasTabbableChild';
// For backward compatibility.
export {isFocusable} from '@react-aria/utils';

export type {FocusScopeProps, FocusManager, FocusManagerOptions} from './FocusScope';
export type {FocusRingProps} from './FocusRing';
Expand Down
8 changes: 7 additions & 1 deletion packages/@react-aria/focus/src/useFocusable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,17 @@ export function useFocusable<T extends FocusableElement = FocusableElement>(prop
autoFocusRef.current = false;
}, [domRef]);

// Always set a tabIndex so that Safari allows focusing native buttons and inputs.
let tabIndex: number | undefined = props.excludeFromTabOrder ? -1 : 0;
if (props.isDisabled) {
tabIndex = undefined;
}

return {
focusableProps: mergeProps(
{
...interactions,
tabIndex: props.excludeFromTabOrder && !props.isDisabled ? -1 : undefined
tabIndex
},
interactionProps
)
Expand Down
18 changes: 14 additions & 4 deletions packages/@react-aria/grid/src/useGridSelectionAnnouncement.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ import {Collection, Key, Node, Selection} from '@react-types/shared';
// @ts-ignore
import intlMessages from '../intl/*.json';
import {SelectionManager} from '@react-stately/selection';
import {useEffectEvent, useUpdateEffect} from '@react-aria/utils';
import {useLocalizedStringFormatter} from '@react-aria/i18n';
import {useRef} from 'react';
import {useUpdateEffect} from '@react-aria/utils';

export interface GridSelectionAnnouncementProps {
/**
Expand Down Expand Up @@ -46,8 +46,8 @@ export function useGridSelectionAnnouncement<T>(props: GridSelectionAnnouncement
// We do this using an ARIA live region.
let selection = state.selectionManager.rawSelection;
let lastSelection = useRef(selection);
useUpdateEffect(() => {
if (!state.selectionManager.isFocused) {
let announceSelectionChange = useEffectEvent(() => {
if (!state.selectionManager.isFocused || selection === lastSelection.current) {
lastSelection.current = selection;

return;
Expand Down Expand Up @@ -96,7 +96,17 @@ export function useGridSelectionAnnouncement<T>(props: GridSelectionAnnouncement
}

lastSelection.current = selection;
}, [selection]);
});

useUpdateEffect(() => {
if (state.selectionManager.isFocused) {
announceSelectionChange();
} else {
// Wait a frame in case the collection is about to become focused (e.g. on mouse down).
let raf = requestAnimationFrame(announceSelectionChange);
return () => cancelAnimationFrame(raf);
}
}, [selection, state.selectionManager.isFocused]);
}

function diffSelection(a: Selection, b: Selection): Set<Key> {
Expand Down
7 changes: 6 additions & 1 deletion packages/@react-aria/interactions/src/useFocusVisible.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// See https://github.com/facebook/react/tree/cc7c1aece46a6b69b41958d731e0fd27c94bfc6c/packages/react-interactions

import {getOwnerDocument, getOwnerWindow, isMac, isVirtualClick} from '@react-aria/utils';
import {ignoreFocusEvent} from './utils';
import {useEffect, useState} from 'react';
import {useIsSSR} from '@react-aria/ssr';

Expand Down Expand Up @@ -92,7 +93,7 @@ function handleFocusEvent(e: FocusEvent) {
// Firefox fires two extra focus events when the user first clicks into an iframe:
// first on the window, then on the document. We ignore these events so they don't
// cause keyboard focus rings to appear.
if (e.target === window || e.target === document) {
if (e.target === window || e.target === document || ignoreFocusEvent) {
return;
}

Expand All @@ -108,6 +109,10 @@ function handleFocusEvent(e: FocusEvent) {
}

function handleWindowBlur() {
if (ignoreFocusEvent) {
return;
}

// When the window is blurred, reset state. This is necessary when tabbing out of the window,
// for example, since a subsequent focus event won't be fired.
hasEventBeforeFocus = false;
Expand Down
10 changes: 8 additions & 2 deletions packages/@react-aria/interactions/src/useLongPress.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
* governing permissions and limitations under the License.
*/

import {DOMAttributes, LongPressEvent} from '@react-types/shared';
import {mergeProps, useDescription, useGlobalListeners} from '@react-aria/utils';
import {DOMAttributes, FocusableElement, LongPressEvent} from '@react-types/shared';
import {focusWithoutScrolling, getOwnerDocument, mergeProps, useDescription, useGlobalListeners} from '@react-aria/utils';
import {usePress} from './usePress';
import {useRef} from 'react';

Expand Down Expand Up @@ -81,6 +81,12 @@ export function useLongPress(props: LongPressProps): LongPressResult {
timeRef.current = setTimeout(() => {
// Prevent other usePress handlers from also handling this event.
e.target.dispatchEvent(new PointerEvent('pointercancel', {bubbles: true}));

// Ensure target is focused. On touch devices, browsers typically focus on pointer up.
if (getOwnerDocument(e.target).activeElement !== e.target) {
focusWithoutScrolling(e.target as FocusableElement);
}

if (onLongPress) {
onLongPress({
...e,
Expand Down
Loading

1 comment on commit cdba748

@rspbot
Copy link

@rspbot rspbot commented on cdba748 Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.