From ba14e6549ac75d1e09d0f939378cb1a6245db442 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Fri, 15 Oct 2021 15:11:37 -0700 Subject: [PATCH 01/13] preventing text selection on all pressable elements --- .../@react-aria/interactions/src/usePress.ts | 22 +++++++++-- .../interactions/test/usePress.test.js | 37 +++++++++++++++++-- 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/packages/@react-aria/interactions/src/usePress.ts b/packages/@react-aria/interactions/src/usePress.ts index 7fe4fbf17b7..8df8253e9f4 100644 --- a/packages/@react-aria/interactions/src/usePress.ts +++ b/packages/@react-aria/interactions/src/usePress.ts @@ -35,7 +35,12 @@ export interface PressProps extends PressEvents { * still pressed, onPressStart will be fired again. If set to `true`, the press is canceled * when the pointer leaves the target and onPressStart will not be fired if the pointer returns. */ - shouldCancelOnPointerExit?: boolean + shouldCancelOnPointerExit?: boolean, + /** + * Whether text selection should be disabled on the pressable element. + * @default true + */ + preventTextSelectionOnPress?: boolean } export interface PressHookProps extends PressProps { @@ -99,6 +104,7 @@ export function usePress(props: PressHookProps): PressResult { isPressed: isPressedProp, preventFocusOnPress, shouldCancelOnPointerExit, + preventTextSelectionOnPress = true, // eslint-disable-next-line @typescript-eslint/no-unused-vars ref: _, // Removing `ref` from `domProps` because TypeScript is dumb, ...domProps @@ -321,7 +327,10 @@ export function usePress(props: PressHookProps): PressResult { focusWithoutScrolling(e.currentTarget); } - disableTextSelection(); + if (preventTextSelectionOnPress) { + disableTextSelection(); + } + triggerPressStart(e, state.pointerType); addGlobalListener(document, 'pointermove', onPointerMove, false); @@ -526,7 +535,10 @@ export function usePress(props: PressHookProps): PressResult { focusWithoutScrolling(e.currentTarget); } - disableTextSelection(); + if (preventTextSelectionOnPress) { + disableTextSelection(); + } + triggerPressStart(e, state.pointerType); addGlobalListener(window, 'scroll', onScroll, true); @@ -624,9 +636,11 @@ export function usePress(props: PressHookProps): PressResult { return () => restoreTextSelection(); }, []); + let styles = preventTextSelectionOnPress ? {userSelect: 'none'} : null; + return { isPressed: isPressedProp || isPressed, - pressProps: mergeProps(domProps, pressProps) + pressProps: mergeProps(domProps, pressProps, {style: styles}) }; } diff --git a/packages/@react-aria/interactions/test/usePress.test.js b/packages/@react-aria/interactions/test/usePress.test.js index 02b04175fc9..9a2dfbbaeec 100644 --- a/packages/@react-aria/interactions/test/usePress.test.js +++ b/packages/@react-aria/interactions/test/usePress.test.js @@ -2213,7 +2213,36 @@ describe('usePress', function () { document.documentElement.style.webkitUserSelect = oldUserSelect; }); - it('should add user-select: none to html element when press start (iOS)', function () { + it('should add user-select: none to the pressable element by default', function () { + let {getByText} = render( + + ); + + let el = getByText('test'); + expect(el).toHaveStyle('user-select: none'); + }); + + it('should not add user-select: none to the pressable element if preventTextSelectionOnPress is false', function () { + let {getByText} = render( + + ); + + let el = getByText('test'); + expect(el).not.toHaveStyle('user-select: none'); + }); + + it('should add user-select: none to the page on press start (iOS)', function () { let {getByText} = render( Date: Fri, 15 Oct 2021 17:58:44 -0700 Subject: [PATCH 02/13] allowing users to select link text if click+drag starts from off the link element previous commit added user-select:none to all pressable elements but that prevented users from copying Link text completely. This restores that functionality but prevents long press from selecting text --- .../@react-aria/interactions/src/usePress.ts | 40 ++++++++++-- .../interactions/test/usePress.test.js | 63 ++++++++++--------- 2 files changed, 67 insertions(+), 36 deletions(-) diff --git a/packages/@react-aria/interactions/src/usePress.ts b/packages/@react-aria/interactions/src/usePress.ts index 8df8253e9f4..0fee660cecf 100644 --- a/packages/@react-aria/interactions/src/usePress.ts +++ b/packages/@react-aria/interactions/src/usePress.ts @@ -16,7 +16,7 @@ // See https://github.com/facebook/react/tree/cc7c1aece46a6b69b41958d731e0fd27c94bfc6c/packages/react-interactions import {disableTextSelection, restoreTextSelection} from './textSelection'; -import {focusWithoutScrolling, mergeProps, useGlobalListeners, useSyncRef} from '@react-aria/utils'; +import {focusWithoutScrolling, isIOS, mergeProps, useGlobalListeners, useSyncRef} from '@react-aria/utils'; import {HTMLAttributes, RefObject, useContext, useEffect, useMemo, useRef, useState} from 'react'; import {isVirtualClick} from './utils'; import {PointerType, PressEvents} from '@react-types/shared'; @@ -121,7 +121,8 @@ export function usePress(props: PressHookProps): PressResult { activePointerId: null, target: null, isOverTarget: false, - pointerType: null + pointerType: null, + userSelect: '' }); let {addGlobalListener, removeAllGlobalListeners} = useGlobalListeners(); @@ -224,6 +225,13 @@ export function usePress(props: PressHookProps): PressResult { state.pointerType = null; removeAllGlobalListeners(); restoreTextSelection(); + // If non-iOS and we have modified the user-select of the target, revert to the original user-select value + if (!isIOS() && state.target.style.userSelect === 'none' && state.userSelect !== 'none' ) { + state.target.style.userSelect = state.userSelect; + if (state.target.getAttribute('style') == '') { + state.target.removeAttribute('style'); + } + } } }; @@ -329,6 +337,10 @@ export function usePress(props: PressHookProps): PressResult { if (preventTextSelectionOnPress) { disableTextSelection(); + if (!isIOS()) { + state.userSelect = state.target.style.userSelect; + state.target.style.userSelect = 'none'; + } } triggerPressStart(e, state.pointerType); @@ -405,6 +417,13 @@ export function usePress(props: PressHookProps): PressResult { state.pointerType = null; removeAllGlobalListeners(); restoreTextSelection(); + // If non-iOS and we have modified the user-select of the target, revert to the original user-select value + if (!isIOS() && state.target.style.userSelect === 'none' && state.userSelect !== 'none' ) { + state.target.style.userSelect = state.userSelect; + if (state.target.getAttribute('style') == '') { + state.target.removeAttribute('style'); + } + } } }; @@ -537,6 +556,10 @@ export function usePress(props: PressHookProps): PressResult { if (preventTextSelectionOnPress) { disableTextSelection(); + if (!isIOS()) { + state.userSelect = state.target.style.userSelect; + state.target.style.userSelect = 'none'; + } } triggerPressStart(e, state.pointerType); @@ -593,6 +616,13 @@ export function usePress(props: PressHookProps): PressResult { state.ignoreEmulatedMouseEvents = true; restoreTextSelection(); removeAllGlobalListeners(); + // If non-iOS and we have modified the user-select of the target, revert to the original user-select value + if (!isIOS() && state.target.style.userSelect === 'none' && state.userSelect !== 'none' ) { + state.target.style.userSelect = state.userSelect; + if (state.target.getAttribute('style') == '') { + state.target.removeAttribute('style'); + } + } }; pressProps.onTouchCancel = (e) => { @@ -628,7 +658,7 @@ export function usePress(props: PressHookProps): PressResult { } return pressProps; - }, [addGlobalListener, isDisabled, preventFocusOnPress, removeAllGlobalListeners]); + }, [addGlobalListener, isDisabled, preventFocusOnPress, removeAllGlobalListeners, preventTextSelectionOnPress]); // Remove user-select: none in case component unmounts immediately after pressStart // eslint-disable-next-line arrow-body-style @@ -636,11 +666,9 @@ export function usePress(props: PressHookProps): PressResult { return () => restoreTextSelection(); }, []); - let styles = preventTextSelectionOnPress ? {userSelect: 'none'} : null; - return { isPressed: isPressedProp || isPressed, - pressProps: mergeProps(domProps, pressProps, {style: styles}) + pressProps: mergeProps(domProps, pressProps) }; } diff --git a/packages/@react-aria/interactions/test/usePress.test.js b/packages/@react-aria/interactions/test/usePress.test.js index 9a2dfbbaeec..338e73c4847 100644 --- a/packages/@react-aria/interactions/test/usePress.test.js +++ b/packages/@react-aria/interactions/test/usePress.test.js @@ -21,9 +21,9 @@ import {theme} from '@react-spectrum/theme-default'; import {usePress} from '../'; function Example(props) { - let {elementType: ElementType = 'div', ...otherProps} = props; + let {elementType: ElementType = 'div', style, ...otherProps} = props; let {pressProps} = usePress(otherProps); - return test; + return test; } function pointerEvent(type, opts) { @@ -2213,7 +2213,7 @@ describe('usePress', function () { document.documentElement.style.webkitUserSelect = oldUserSelect; }); - it('should add user-select: none to the pressable element by default', function () { + it('should add user-select: none to the page on press start (iOS)', function () { let {getByText} = render( - ); - - let el = getByText('test'); + fireEvent.touchStart(el, {targetTouches: [{identifier: 1}]}); + expect(document.documentElement.style.webkitUserSelect).toBe('none'); expect(el).not.toHaveStyle('user-select: none'); + fireEvent.touchEnd(el, {targetTouches: [{identifier: 1}]}); }); - it('should add user-select: none to the page on press start (iOS)', function () { + it('should not add user-select: none to the page when press start (non-iOS)', function () { + platformGetter.mockReturnValue('Android'); let {getByText} = render( {jest.advanceTimersByTime(300);}); + expect(document.documentElement.style.webkitUserSelect).toBe(mockUserSelect); + + // Checkbox doesn't remove `user-select: none;` style from HTML Element issue + // see https://github.com/adobe/react-spectrum/issues/862 + fireEvent.touchStart(el, {targetTouches: [{identifier: 1}]}); + fireEvent.touchEnd(el, {changedTouches: [{identifier: 1}]}); + fireEvent.touchStart(el, {targetTouches: [{identifier: 1}]}); + fireEvent.touchMove(el, {changedTouches: [{identifier: 1, clientX: 100, clientY: 100}]}); + act(() => {jest.advanceTimersByTime(300);}); + fireEvent.touchEnd(el, {changedTouches: [{identifier: 1, clientX: 100, clientY: 100}]}); + act(() => {jest.advanceTimersByTime(300);}); + expect(document.documentElement.style.webkitUserSelect).toBe(mockUserSelect); }); - it('should remove user-select: none from the page when press end (iOS)', function () { + it('should remove user-select: none from the element when press end (non-iOS)', function () { + platformGetter.mockReturnValue('Android'); let {getByText} = render( {jest.advanceTimersByTime(300);}); - expect(document.documentElement.style.webkitUserSelect).toBe(mockUserSelect); + expect(el).toHaveStyle('user-select: text'); // Checkbox doesn't remove `user-select: none;` style from HTML Element issue // see https://github.com/adobe/react-spectrum/issues/862 @@ -2297,11 +2303,8 @@ describe('usePress', function () { fireEvent.touchEnd(el, {changedTouches: [{identifier: 1}]}); fireEvent.touchStart(el, {targetTouches: [{identifier: 1}]}); fireEvent.touchMove(el, {changedTouches: [{identifier: 1, clientX: 100, clientY: 100}]}); - act(() => {jest.advanceTimersByTime(300);}); fireEvent.touchEnd(el, {changedTouches: [{identifier: 1, clientX: 100, clientY: 100}]}); - act(() => {jest.advanceTimersByTime(300);}); - - expect(document.documentElement.style.webkitUserSelect).toBe(mockUserSelect); + expect(el).toHaveStyle('user-select: text'); }); it('should not remove user-select: none when pressing two different elements quickly (iOS)', function () { From 202ade1f218aef97ff75716a76e9bc2f519b6cfa Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Mon, 18 Oct 2021 10:29:32 -0700 Subject: [PATCH 03/13] moving shared logic out of usePress and into textSelection --- .../interactions/src/textSelection.ts | 68 +++++++++++-------- .../@react-aria/interactions/src/usePress.ts | 46 +++---------- 2 files changed, 47 insertions(+), 67 deletions(-) diff --git a/packages/@react-aria/interactions/src/textSelection.ts b/packages/@react-aria/interactions/src/textSelection.ts index bfb708daf6f..603be8cf720 100644 --- a/packages/@react-aria/interactions/src/textSelection.ts +++ b/packages/@react-aria/interactions/src/textSelection.ts @@ -25,48 +25,58 @@ type State = 'default' | 'disabled' | 'restoring'; let state: State = 'default'; let savedUserSelect = ''; +let modifiedElementMap = new WeakMap(); -export function disableTextSelection() { - // Limit this behavior to iOS only. Android devices don't text select nearby element - // when long pressing on a different element. - if (!isIOS()) { - return; - } - +export function disableTextSelection(target?: HTMLElement) { if (state === 'default') { - savedUserSelect = document.documentElement.style.webkitUserSelect; - document.documentElement.style.webkitUserSelect = 'none'; + if (isIOS()) { + savedUserSelect = document.documentElement.style.webkitUserSelect; + document.documentElement.style.webkitUserSelect = 'none'; + } else if (target) { + modifiedElementMap.set(target, target.style.userSelect); + target.style.userSelect = 'none'; + } } state = 'disabled'; } -export function restoreTextSelection() { +export function restoreTextSelection(target?: HTMLElement) { // If the state is already default, there's nothing to do. // If it is restoring, then there's no need to queue a second restore. - // Limit this behavior to iOS only. Android devices don't text select nearby element - // when long pressing on a different element. - if (state !== 'disabled' || !isIOS()) { + if (state !== 'disabled') { return; } state = 'restoring'; - // There appears to be a delay on iOS where selection still might occur - // after pointer up, so wait a bit before removing user-select. - setTimeout(() => { - // Wait for any CSS transitions to complete so we don't recompute style - // for the whole page in the middle of the animation and cause jank. - runAfterTransition(() => { - // Avoid race conditions - if (state === 'restoring') { - if (document.documentElement.style.webkitUserSelect === 'none') { - document.documentElement.style.webkitUserSelect = savedUserSelect || ''; - } - - savedUserSelect = ''; - state = 'default'; + if (!isIOS()) { + if (target) { + let targetOldUserSelect = modifiedElementMap.get(target); + target.style.userSelect = targetOldUserSelect; + if (target.getAttribute('style') === '') { + target.removeAttribute('style'); } - }); - }, 300); + modifiedElementMap.delete(target); + } + + state = 'default'; + } else { + // There appears to be a delay on iOS where selection still might occur + // after pointer up, so wait a bit before removing user-select. + setTimeout(() => { + // Wait for any CSS transitions to complete so we don't recompute style + // for the whole page in the middle of the animation and cause jank. + runAfterTransition(() => { + // Avoid race conditions + if (state === 'restoring') { + if (document.documentElement.style.webkitUserSelect === 'none') { + document.documentElement.style.webkitUserSelect = savedUserSelect || ''; + } + savedUserSelect = ''; + state = 'default'; + } + }); + }, 300); + } } diff --git a/packages/@react-aria/interactions/src/usePress.ts b/packages/@react-aria/interactions/src/usePress.ts index 0fee660cecf..8d06499f7a2 100644 --- a/packages/@react-aria/interactions/src/usePress.ts +++ b/packages/@react-aria/interactions/src/usePress.ts @@ -16,7 +16,7 @@ // See https://github.com/facebook/react/tree/cc7c1aece46a6b69b41958d731e0fd27c94bfc6c/packages/react-interactions import {disableTextSelection, restoreTextSelection} from './textSelection'; -import {focusWithoutScrolling, isIOS, mergeProps, useGlobalListeners, useSyncRef} from '@react-aria/utils'; +import {focusWithoutScrolling, mergeProps, useGlobalListeners, useSyncRef} from '@react-aria/utils'; import {HTMLAttributes, RefObject, useContext, useEffect, useMemo, useRef, useState} from 'react'; import {isVirtualClick} from './utils'; import {PointerType, PressEvents} from '@react-types/shared'; @@ -121,8 +121,7 @@ export function usePress(props: PressHookProps): PressResult { activePointerId: null, target: null, isOverTarget: false, - pointerType: null, - userSelect: '' + pointerType: null }); let {addGlobalListener, removeAllGlobalListeners} = useGlobalListeners(); @@ -224,14 +223,7 @@ export function usePress(props: PressHookProps): PressResult { state.activePointerId = null; state.pointerType = null; removeAllGlobalListeners(); - restoreTextSelection(); - // If non-iOS and we have modified the user-select of the target, revert to the original user-select value - if (!isIOS() && state.target.style.userSelect === 'none' && state.userSelect !== 'none' ) { - state.target.style.userSelect = state.userSelect; - if (state.target.getAttribute('style') == '') { - state.target.removeAttribute('style'); - } - } + restoreTextSelection(state.target); } }; @@ -336,11 +328,7 @@ export function usePress(props: PressHookProps): PressResult { } if (preventTextSelectionOnPress) { - disableTextSelection(); - if (!isIOS()) { - state.userSelect = state.target.style.userSelect; - state.target.style.userSelect = 'none'; - } + disableTextSelection(state.target); } triggerPressStart(e, state.pointerType); @@ -416,14 +404,7 @@ export function usePress(props: PressHookProps): PressResult { state.activePointerId = null; state.pointerType = null; removeAllGlobalListeners(); - restoreTextSelection(); - // If non-iOS and we have modified the user-select of the target, revert to the original user-select value - if (!isIOS() && state.target.style.userSelect === 'none' && state.userSelect !== 'none' ) { - state.target.style.userSelect = state.userSelect; - if (state.target.getAttribute('style') == '') { - state.target.removeAttribute('style'); - } - } + restoreTextSelection(state.target); } }; @@ -555,11 +536,7 @@ export function usePress(props: PressHookProps): PressResult { } if (preventTextSelectionOnPress) { - disableTextSelection(); - if (!isIOS()) { - state.userSelect = state.target.style.userSelect; - state.target.style.userSelect = 'none'; - } + disableTextSelection(state.target); } triggerPressStart(e, state.pointerType); @@ -614,15 +591,8 @@ export function usePress(props: PressHookProps): PressResult { state.activePointerId = null; state.isOverTarget = false; state.ignoreEmulatedMouseEvents = true; - restoreTextSelection(); + restoreTextSelection(state.target); removeAllGlobalListeners(); - // If non-iOS and we have modified the user-select of the target, revert to the original user-select value - if (!isIOS() && state.target.style.userSelect === 'none' && state.userSelect !== 'none' ) { - state.target.style.userSelect = state.userSelect; - if (state.target.getAttribute('style') == '') { - state.target.removeAttribute('style'); - } - } }; pressProps.onTouchCancel = (e) => { @@ -663,7 +633,7 @@ export function usePress(props: PressHookProps): PressResult { // Remove user-select: none in case component unmounts immediately after pressStart // eslint-disable-next-line arrow-body-style useEffect(() => { - return () => restoreTextSelection(); + return () => restoreTextSelection(ref.current.target); }, []); return { From f8bc103501e39696b97f67799b1f4dad4a276e2d Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Mon, 18 Oct 2021 10:42:31 -0700 Subject: [PATCH 04/13] add comment --- packages/@react-aria/interactions/src/textSelection.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/@react-aria/interactions/src/textSelection.ts b/packages/@react-aria/interactions/src/textSelection.ts index 603be8cf720..63f8f1ae013 100644 --- a/packages/@react-aria/interactions/src/textSelection.ts +++ b/packages/@react-aria/interactions/src/textSelection.ts @@ -21,6 +21,10 @@ import {isIOS, runAfterTransition} from '@react-aria/utils'; // There are three possible states due to the delay before removing user-select: none after // pointer up. The 'default' state always transitions to the 'disabled' state, which transitions // to 'restoring'. The 'restoring' state can either transition back to 'disabled' or 'default'. + +// For non-iOS devices, we apply user-select: none to the pressed element instead to avoid possible +// performance issues that arise from applying and removing user-select: none to the entire page +// (see https://github.com/adobe/react-spectrum/issues/1609). type State = 'default' | 'disabled' | 'restoring'; let state: State = 'default'; From e73fb528773e5415d437bab38f6b232b89acb514 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Mon, 18 Oct 2021 11:47:24 -0700 Subject: [PATCH 05/13] fixing textselection cleanup on multipress for non iOS and adding test --- .../interactions/src/textSelection.ts | 51 ++++++------- .../@react-aria/interactions/src/usePress.ts | 20 +++-- .../interactions/test/usePress.test.js | 74 +++++++++++++++++++ 3 files changed, 115 insertions(+), 30 deletions(-) diff --git a/packages/@react-aria/interactions/src/textSelection.ts b/packages/@react-aria/interactions/src/textSelection.ts index 63f8f1ae013..bcbe4ef1cd6 100644 --- a/packages/@react-aria/interactions/src/textSelection.ts +++ b/packages/@react-aria/interactions/src/textSelection.ts @@ -32,40 +32,30 @@ let savedUserSelect = ''; let modifiedElementMap = new WeakMap(); export function disableTextSelection(target?: HTMLElement) { - if (state === 'default') { - if (isIOS()) { + if (isIOS()) { + if (state === 'default') { savedUserSelect = document.documentElement.style.webkitUserSelect; document.documentElement.style.webkitUserSelect = 'none'; - } else if (target) { - modifiedElementMap.set(target, target.style.userSelect); - target.style.userSelect = 'none'; } - } - state = 'disabled'; + state = 'disabled'; + } else if (target) { + // If not iOS, store the target's original user-select and change to user-select: none + modifiedElementMap.set(target, target.style.userSelect); + target.style.userSelect = 'none'; + } } export function restoreTextSelection(target?: HTMLElement) { - // If the state is already default, there's nothing to do. - // If it is restoring, then there's no need to queue a second restore. - if (state !== 'disabled') { - return; - } - - state = 'restoring'; - - if (!isIOS()) { - if (target) { - let targetOldUserSelect = modifiedElementMap.get(target); - target.style.userSelect = targetOldUserSelect; - if (target.getAttribute('style') === '') { - target.removeAttribute('style'); - } - modifiedElementMap.delete(target); + if (isIOS()) { + // If the state is already default, there's nothing to do. + // If it is restoring, then there's no need to queue a second restore. + if (state !== 'disabled') { + return; } - state = 'default'; - } else { + state = 'restoring'; + // There appears to be a delay on iOS where selection still might occur // after pointer up, so wait a bit before removing user-select. setTimeout(() => { @@ -77,10 +67,21 @@ export function restoreTextSelection(target?: HTMLElement) { if (document.documentElement.style.webkitUserSelect === 'none') { document.documentElement.style.webkitUserSelect = savedUserSelect || ''; } + savedUserSelect = ''; state = 'default'; } }); }, 300); + } else { + // If not iOS, restore the target's original user-select if any + if (target && modifiedElementMap.has(target)) { + let targetOldUserSelect = modifiedElementMap.get(target); + target.style.userSelect = targetOldUserSelect; + if (target.getAttribute('style') === '') { + target.removeAttribute('style'); + } + modifiedElementMap.delete(target); + } } } diff --git a/packages/@react-aria/interactions/src/usePress.ts b/packages/@react-aria/interactions/src/usePress.ts index 7af9099bfea..326b767bb58 100644 --- a/packages/@react-aria/interactions/src/usePress.ts +++ b/packages/@react-aria/interactions/src/usePress.ts @@ -223,7 +223,9 @@ export function usePress(props: PressHookProps): PressResult { state.activePointerId = null; state.pointerType = null; removeAllGlobalListeners(); - restoreTextSelection(state.target); + if (preventTextSelectionOnPress) { + restoreTextSelection(state.target); + } } }; @@ -405,7 +407,9 @@ export function usePress(props: PressHookProps): PressResult { state.activePointerId = null; state.pointerType = null; removeAllGlobalListeners(); - restoreTextSelection(state.target); + if (preventTextSelectionOnPress) { + restoreTextSelection(state.target); + } } }; @@ -592,7 +596,9 @@ export function usePress(props: PressHookProps): PressResult { state.activePointerId = null; state.isOverTarget = false; state.ignoreEmulatedMouseEvents = true; - restoreTextSelection(state.target); + if (preventTextSelectionOnPress) { + restoreTextSelection(state.target); + } removeAllGlobalListeners(); }; @@ -634,8 +640,12 @@ export function usePress(props: PressHookProps): PressResult { // Remove user-select: none in case component unmounts immediately after pressStart // eslint-disable-next-line arrow-body-style useEffect(() => { - return () => restoreTextSelection(ref.current.target); - }, []); + return () => { + if (preventTextSelectionOnPress) { + restoreTextSelection(ref.current.target); + } + }; + }, [preventTextSelectionOnPress]); return { isPressed: isPressedProp || isPressed, diff --git a/packages/@react-aria/interactions/test/usePress.test.js b/packages/@react-aria/interactions/test/usePress.test.js index 338e73c4847..06c0d7cb72e 100644 --- a/packages/@react-aria/interactions/test/usePress.test.js +++ b/packages/@react-aria/interactions/test/usePress.test.js @@ -2343,6 +2343,80 @@ describe('usePress', function () { expect(document.documentElement.style.webkitUserSelect).toBe(mockUserSelect); }); + it('should not remove user-select: none when pressing two different elements quickly (iOS)', function () { + let {getAllByText} = render( + <> + + + + ); + + let els = getAllByText('test'); + + fireEvent.touchStart(els[0], {targetTouches: [{identifier: 1}]}); + fireEvent.touchEnd(els[0], {changedTouches: [{identifier: 1}]}); + + expect(document.documentElement.style.webkitUserSelect).toBe('none'); + + fireEvent.touchStart(els[1], {targetTouches: [{identifier: 1}]}); + + act(() => {jest.advanceTimersByTime(300);}); + expect(document.documentElement.style.webkitUserSelect).toBe('none'); + + fireEvent.touchEnd(els[1], {changedTouches: [{identifier: 1}]}); + + act(() => {jest.advanceTimersByTime(300);}); + expect(document.documentElement.style.webkitUserSelect).toBe(mockUserSelect); + }); + + it('should clean up user-select: none when pressing and releasing two different elements (non-iOS)', function () { + platformGetter.mockReturnValue('Android'); + let {getAllByText} = render( + <> + + + + ); + + let els = getAllByText('test'); + + fireEvent.touchStart(els[0], {targetTouches: [{identifier: 1}]}); + fireEvent.touchStart(els[1], {targetTouches: [{identifier: 1}]}); + + expect(els[0]).toHaveStyle('user-select: none'); + expect(els[1]).toHaveStyle('user-select: none'); + + fireEvent.touchEnd(els[0], {changedTouches: [{identifier: 1}]}); + expect(els[0]).toHaveStyle('user-select: text'); + expect(els[1]).toHaveStyle('user-select: none'); + + fireEvent.touchEnd(els[1], {changedTouches: [{identifier: 1}]}); + expect(els[0]).toHaveStyle('user-select: text'); + expect(els[1]).toHaveStyle('user-select: text'); + }); + it('should remove user-select: none from the page if pressable component unmounts (iOS)', function () { let {getByText, unmount} = render( Date: Mon, 25 Oct 2021 17:43:35 -0700 Subject: [PATCH 06/13] review comments --- packages/@react-aria/interactions/src/textSelection.ts | 4 ++++ packages/@react-aria/interactions/test/usePress.test.js | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/@react-aria/interactions/src/textSelection.ts b/packages/@react-aria/interactions/src/textSelection.ts index bcbe4ef1cd6..18160b7ebc7 100644 --- a/packages/@react-aria/interactions/src/textSelection.ts +++ b/packages/@react-aria/interactions/src/textSelection.ts @@ -27,6 +27,8 @@ import {isIOS, runAfterTransition} from '@react-aria/utils'; // (see https://github.com/adobe/react-spectrum/issues/1609). type State = 'default' | 'disabled' | 'restoring'; +// Note that state only matters here for iOS. Non-iOS gets user-select: none applied to the target element +// rather than at the document level so we just need to apply/remove user-select: none for each pressed element individually let state: State = 'default'; let savedUserSelect = ''; let modifiedElementMap = new WeakMap(); @@ -41,6 +43,7 @@ export function disableTextSelection(target?: HTMLElement) { state = 'disabled'; } else if (target) { // If not iOS, store the target's original user-select and change to user-select: none + // Ignore state since it doesn't apply for non iOS modifiedElementMap.set(target, target.style.userSelect); target.style.userSelect = 'none'; } @@ -75,6 +78,7 @@ export function restoreTextSelection(target?: HTMLElement) { }, 300); } else { // If not iOS, restore the target's original user-select if any + // Ignore state since it doesn't apply for non iOS if (target && modifiedElementMap.has(target)) { let targetOldUserSelect = modifiedElementMap.get(target); target.style.userSelect = targetOldUserSelect; diff --git a/packages/@react-aria/interactions/test/usePress.test.js b/packages/@react-aria/interactions/test/usePress.test.js index 06c0d7cb72e..10b443ac4a6 100644 --- a/packages/@react-aria/interactions/test/usePress.test.js +++ b/packages/@react-aria/interactions/test/usePress.test.js @@ -2403,7 +2403,7 @@ describe('usePress', function () { let els = getAllByText('test'); fireEvent.touchStart(els[0], {targetTouches: [{identifier: 1}]}); - fireEvent.touchStart(els[1], {targetTouches: [{identifier: 1}]}); + fireEvent.touchStart(els[1], {targetTouches: [{identifier: 2}]}); expect(els[0]).toHaveStyle('user-select: none'); expect(els[1]).toHaveStyle('user-select: none'); @@ -2412,7 +2412,7 @@ describe('usePress', function () { expect(els[0]).toHaveStyle('user-select: text'); expect(els[1]).toHaveStyle('user-select: none'); - fireEvent.touchEnd(els[1], {changedTouches: [{identifier: 1}]}); + fireEvent.touchEnd(els[1], {changedTouches: [{identifier: 2}]}); expect(els[0]).toHaveStyle('user-select: text'); expect(els[1]).toHaveStyle('user-select: text'); }); From 35eb3c5c45e3c66bc9e306c61c62edccd453e0f8 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Wed, 27 Oct 2021 14:18:17 -0700 Subject: [PATCH 07/13] addressing review comments --- .../@react-aria/interactions/src/usePress.ts | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/packages/@react-aria/interactions/src/usePress.ts b/packages/@react-aria/interactions/src/usePress.ts index 326b767bb58..110a9aafd27 100644 --- a/packages/@react-aria/interactions/src/usePress.ts +++ b/packages/@react-aria/interactions/src/usePress.ts @@ -36,11 +36,8 @@ export interface PressProps extends PressEvents { * when the pointer leaves the target and onPressStart will not be fired if the pointer returns. */ shouldCancelOnPointerExit?: boolean, - /** - * Whether text selection should be disabled on the pressable element. - * @default true - */ - preventTextSelectionOnPress?: boolean + /** Whether text selection should be enabled on the pressable element. */ + allowTextSelectionOnPress?: boolean } export interface PressHookProps extends PressProps { @@ -104,7 +101,7 @@ export function usePress(props: PressHookProps): PressResult { isPressed: isPressedProp, preventFocusOnPress, shouldCancelOnPointerExit, - preventTextSelectionOnPress = true, + allowTextSelectionOnPress, // eslint-disable-next-line @typescript-eslint/no-unused-vars ref: _, // Removing `ref` from `domProps` because TypeScript is dumb, ...domProps @@ -223,7 +220,7 @@ export function usePress(props: PressHookProps): PressResult { state.activePointerId = null; state.pointerType = null; removeAllGlobalListeners(); - if (preventTextSelectionOnPress) { + if (!allowTextSelectionOnPress) { restoreTextSelection(state.target); } } @@ -330,7 +327,7 @@ export function usePress(props: PressHookProps): PressResult { focusWithoutScrolling(e.currentTarget); } - if (preventTextSelectionOnPress) { + if (!allowTextSelectionOnPress) { disableTextSelection(state.target); } @@ -407,7 +404,7 @@ export function usePress(props: PressHookProps): PressResult { state.activePointerId = null; state.pointerType = null; removeAllGlobalListeners(); - if (preventTextSelectionOnPress) { + if (!allowTextSelectionOnPress) { restoreTextSelection(state.target); } } @@ -540,7 +537,7 @@ export function usePress(props: PressHookProps): PressResult { focusWithoutScrolling(e.currentTarget); } - if (preventTextSelectionOnPress) { + if (!allowTextSelectionOnPress) { disableTextSelection(state.target); } @@ -596,7 +593,7 @@ export function usePress(props: PressHookProps): PressResult { state.activePointerId = null; state.isOverTarget = false; state.ignoreEmulatedMouseEvents = true; - if (preventTextSelectionOnPress) { + if (!allowTextSelectionOnPress) { restoreTextSelection(state.target); } removeAllGlobalListeners(); @@ -635,17 +632,17 @@ export function usePress(props: PressHookProps): PressResult { } return pressProps; - }, [addGlobalListener, isDisabled, preventFocusOnPress, removeAllGlobalListeners, preventTextSelectionOnPress]); + }, [addGlobalListener, isDisabled, preventFocusOnPress, removeAllGlobalListeners, allowTextSelectionOnPress]); // Remove user-select: none in case component unmounts immediately after pressStart // eslint-disable-next-line arrow-body-style useEffect(() => { return () => { - if (preventTextSelectionOnPress) { + if (!allowTextSelectionOnPress) { restoreTextSelection(ref.current.target); } }; - }, [preventTextSelectionOnPress]); + }, [allowTextSelectionOnPress]); return { isPressed: isPressedProp || isPressed, From 819820026988427accde694c78f0642608e7b76a Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Wed, 17 Nov 2021 13:33:28 -0800 Subject: [PATCH 08/13] if user select is no longer none upon restore selection, dont overwrite with old value --- packages/@react-aria/interactions/src/textSelection.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/@react-aria/interactions/src/textSelection.ts b/packages/@react-aria/interactions/src/textSelection.ts index 18160b7ebc7..4a5ebc53a03 100644 --- a/packages/@react-aria/interactions/src/textSelection.ts +++ b/packages/@react-aria/interactions/src/textSelection.ts @@ -81,7 +81,11 @@ export function restoreTextSelection(target?: HTMLElement) { // Ignore state since it doesn't apply for non iOS if (target && modifiedElementMap.has(target)) { let targetOldUserSelect = modifiedElementMap.get(target); - target.style.userSelect = targetOldUserSelect; + + if (target.style.userSelect === 'none') { + target.style.userSelect = targetOldUserSelect; + } + if (target.getAttribute('style') === '') { target.removeAttribute('style'); } From 1304695a49309ade0ea227764af93432fb3a0a83 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Wed, 17 Nov 2021 13:33:50 -0800 Subject: [PATCH 09/13] example to test rerender during button press --- .../button/stories/Button.stories.tsx | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/@react-spectrum/button/stories/Button.stories.tsx b/packages/@react-spectrum/button/stories/Button.stories.tsx index 4a651ff3dd8..32bec2b8439 100644 --- a/packages/@react-spectrum/button/stories/Button.stories.tsx +++ b/packages/@react-spectrum/button/stories/Button.stories.tsx @@ -18,6 +18,7 @@ import React, {ElementType} from 'react'; import {SpectrumButtonProps} from '@react-types/button'; import {storiesOf} from '@storybook/react'; import {Text} from '@react-spectrum/text'; +import {useButton} from '@react-aria/button'; storiesOf('Button', module) .addParameters({providerSwitcher: {status: 'positive'}}) @@ -89,6 +90,10 @@ storiesOf('Button', module) .add( 'element: a, rel: \'noopener noreferrer\'', () => render({elementType: 'a', href: '//example.com', rel: 'noopener noreferrer', variant: 'primary'}) + ) + .add( + 'user-select test', + () => ); function render(props: SpectrumButtonProps = {variant: 'primary'}) { @@ -122,3 +127,11 @@ function render(props: SpectrumButtonProps ); } + +function Example() { + let [show, setShow] = React.useState(false); + + return ( + + ); +} From ce28bd70a7af75d1dbb498bb2818fb3902404fb9 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Wed, 17 Nov 2021 13:38:09 -0800 Subject: [PATCH 10/13] fix lint --- .../@react-spectrum/button/stories/Button.stories.tsx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/@react-spectrum/button/stories/Button.stories.tsx b/packages/@react-spectrum/button/stories/Button.stories.tsx index 32bec2b8439..7b4a31d91e3 100644 --- a/packages/@react-spectrum/button/stories/Button.stories.tsx +++ b/packages/@react-spectrum/button/stories/Button.stories.tsx @@ -18,7 +18,6 @@ import React, {ElementType} from 'react'; import {SpectrumButtonProps} from '@react-types/button'; import {storiesOf} from '@storybook/react'; import {Text} from '@react-spectrum/text'; -import {useButton} from '@react-aria/button'; storiesOf('Button', module) .addParameters({providerSwitcher: {status: 'positive'}}) @@ -130,8 +129,11 @@ function render(props: SpectrumButtonProps function Example() { let [show, setShow] = React.useState(false); - + let [show2, setShow2] = React.useState(false); return ( - + <> + + + ); } From 9270d285b4b6a675450d3cd21b28487db9d3d1ae Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Wed, 17 Nov 2021 13:40:45 -0800 Subject: [PATCH 11/13] fix lint for sure --- packages/@react-spectrum/button/stories/Button.stories.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@react-spectrum/button/stories/Button.stories.tsx b/packages/@react-spectrum/button/stories/Button.stories.tsx index 7b4a31d91e3..b846fc72593 100644 --- a/packages/@react-spectrum/button/stories/Button.stories.tsx +++ b/packages/@react-spectrum/button/stories/Button.stories.tsx @@ -132,8 +132,8 @@ function Example() { let [show2, setShow2] = React.useState(false); return ( <> - - + + ); } From 1e8b22bb9789e0c06355fbabb5271e69241f3c88 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Wed, 17 Nov 2021 14:51:07 -0800 Subject: [PATCH 12/13] Adding tests for style changes during press down --- .../interactions/test/usePress.test.js | 71 +++++++++++++++++++ .../button/stories/Button.stories.tsx | 21 ++++-- 2 files changed, 87 insertions(+), 5 deletions(-) diff --git a/packages/@react-aria/interactions/test/usePress.test.js b/packages/@react-aria/interactions/test/usePress.test.js index 10b443ac4a6..e4fa2b85fb6 100644 --- a/packages/@react-aria/interactions/test/usePress.test.js +++ b/packages/@react-aria/interactions/test/usePress.test.js @@ -2195,6 +2195,14 @@ describe('usePress', function () { let mockUserSelect = 'contain'; let oldUserSelect = document.documentElement.style.webkitUserSelect; let platformGetter; + function TestStyleChange(props) { + let {styleToApply, ...otherProps} = props; + let [show, setShow] = React.useState(false); + let {pressProps} = usePress({...otherProps, onPressStart: () => setTimeout(() => setShow(true), 3000)}); + return ( +
test
+ ); + }; beforeAll(() => { platformGetter = jest.spyOn(window.navigator, 'platform', 'get'); @@ -2435,6 +2443,69 @@ describe('usePress', function () { act(() => {jest.advanceTimersByTime(300);}); expect(document.documentElement.style.webkitUserSelect).toBe(mockUserSelect); }); + + it('non related style changes during press down shouldn\'t overwrite user-select on press end (non-iOS)', function () { + platformGetter.mockReturnValue('Android'); + let {getByText} = render( + + ); + + let el = getByText('test'); + fireEvent.touchStart(el, {targetTouches: [{identifier: 1}]}); + expect(el).toHaveStyle(` + user-select: none; + `); + + act(() => jest.runAllTimers()); + + expect(el).toHaveStyle(` + user-select: none; + background: red; + `); + + fireEvent.touchEnd(el, {changedTouches: [{identifier: 1}]}); + expect(el).toHaveStyle(` + background: red; + `); + }); + + it('changes to user-select during press down remain on press end (non-iOS)', function () { + platformGetter.mockReturnValue('Android'); + let {getByText} = render( + + ); + + let el = getByText('test'); + fireEvent.touchStart(el, {targetTouches: [{identifier: 1}]}); + expect(el).toHaveStyle(` + user-select: none; + `); + + act(() => jest.runAllTimers()); + + expect(el).toHaveStyle(` + user-select: text; + background: red; + `); + + fireEvent.touchEnd(el, {changedTouches: [{identifier: 1}]}); + expect(el).toHaveStyle(` + user-select: text; + background: red; + `); + }); }); describe('portal event bubbling', () => { diff --git a/packages/@react-spectrum/button/stories/Button.stories.tsx b/packages/@react-spectrum/button/stories/Button.stories.tsx index b846fc72593..9b13721b2c3 100644 --- a/packages/@react-spectrum/button/stories/Button.stories.tsx +++ b/packages/@react-spectrum/button/stories/Button.stories.tsx @@ -91,7 +91,7 @@ storiesOf('Button', module) () => render({elementType: 'a', href: '//example.com', rel: 'noopener noreferrer', variant: 'primary'}) ) .add( - 'user-select test', + 'user-select:none on press test', () => ); @@ -130,10 +130,21 @@ function render(props: SpectrumButtonProps function Example() { let [show, setShow] = React.useState(false); let [show2, setShow2] = React.useState(false); + return ( - <> - - - + + + + ); } From c62a7a1f77f1a1d6a22844ba1fb67cfb1343cf9a Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Wed, 17 Nov 2021 14:51:47 -0800 Subject: [PATCH 13/13] fixing lint --- packages/@react-aria/interactions/test/usePress.test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/@react-aria/interactions/test/usePress.test.js b/packages/@react-aria/interactions/test/usePress.test.js index e4fa2b85fb6..1cfd4c485d7 100644 --- a/packages/@react-aria/interactions/test/usePress.test.js +++ b/packages/@react-aria/interactions/test/usePress.test.js @@ -2195,6 +2195,7 @@ describe('usePress', function () { let mockUserSelect = 'contain'; let oldUserSelect = document.documentElement.style.webkitUserSelect; let platformGetter; + function TestStyleChange(props) { let {styleToApply, ...otherProps} = props; let [show, setShow] = React.useState(false); @@ -2202,7 +2203,7 @@ describe('usePress', function () { return (
test
); - }; + } beforeAll(() => { platformGetter = jest.spyOn(window.navigator, 'platform', 'get');