From c672c1cf8ab95671eb3118387c9d8aedf0b7ba55 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Mon, 8 Jun 2020 22:29:35 -0700 Subject: [PATCH 1/6] Track if the mouse is moving as opposed to checking focus start This way we don't pointer focus unless we're actually moving the mouse actively on/to the element --- packages/@react-aria/listbox/src/useOption.ts | 19 +++++++++++-------- packages/@react-aria/select/src/useSelect.ts | 2 ++ .../listbox/src/ListBoxBase.tsx | 2 +- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/packages/@react-aria/listbox/src/useOption.ts b/packages/@react-aria/listbox/src/useOption.ts index 8ad4c296322..f9485b2c428 100644 --- a/packages/@react-aria/listbox/src/useOption.ts +++ b/packages/@react-aria/listbox/src/useOption.ts @@ -10,11 +10,12 @@ * governing permissions and limitations under the License. */ -import {HTMLAttributes, Key, RefObject} from 'react'; +import {HTMLAttributes, Key, RefObject, useState} from 'react'; +import {isFocusVisible} from '@react-aria/interactions'; import {ListState} from '@react-stately/list'; +import {mergeProps, useSlotId} from '@react-aria/utils'; import {useHover, usePress} from '@react-aria/interactions'; import {useSelectableItem} from '@react-aria/selection'; -import {useSlotId} from '@react-aria/utils'; interface OptionAria { /** Props for the option element. */ @@ -94,17 +95,19 @@ export function useOption(props: AriaOptionProps, state: ListState, ref: R let {pressProps} = usePress({...itemProps, isDisabled}); let {hoverProps} = useHover({ isDisabled: isDisabled || !shouldFocusOnHover, - onHoverStart() { - state.selectionManager.setFocused(true); - state.selectionManager.setFocusedKey(key); - } }); return { optionProps: { ...optionProps, - ...pressProps, - ...hoverProps + ...mergeProps(mergeProps(pressProps, hoverProps), { + onMouseMove() { + if (state.selectionManager.focusedKey !== key) { + state.selectionManager.setFocused(true); + state.selectionManager.setFocusedKey(key); + } + } + }) }, labelProps: { id: labelId diff --git a/packages/@react-aria/select/src/useSelect.ts b/packages/@react-aria/select/src/useSelect.ts index f6930e67dee..6e58c7be12a 100644 --- a/packages/@react-aria/select/src/useSelect.ts +++ b/packages/@react-aria/select/src/useSelect.ts @@ -20,6 +20,7 @@ import {SelectState} from '@react-stately/select'; import {useCollator} from '@react-aria/i18n'; import {useLabel} from '@react-aria/label'; import {useMenuTrigger} from '@react-aria/menu'; +import {usePress} from "@react-aria/interactions"; interface AriaSelectOptions extends AriaSelectProps { /** @@ -84,6 +85,7 @@ export function useSelect(props: AriaSelectOptions, state: SelectState, let triggerProps = mergeProps(mergeProps(menuTriggerProps, fieldProps), typeSelectProps); let valueId = useId(); + return { labelProps, triggerProps: mergeProps(domProps, { diff --git a/packages/@react-spectrum/listbox/src/ListBoxBase.tsx b/packages/@react-spectrum/listbox/src/ListBoxBase.tsx index b8e9d7a46e1..2e7997292c4 100644 --- a/packages/@react-spectrum/listbox/src/ListBoxBase.tsx +++ b/packages/@react-spectrum/listbox/src/ListBoxBase.tsx @@ -22,7 +22,7 @@ import {ListLayout} from '@react-stately/layout'; import {ListState} from '@react-stately/list'; import {mergeProps} from '@react-aria/utils'; import {ProgressCircle} from '@react-spectrum/progress'; -import React, {HTMLAttributes, ReactElement, RefObject, useMemo} from 'react'; +import React, {HTMLAttributes, ReactElement, RefObject, useMemo, useState} from 'react'; import {ReusableView} from '@react-stately/virtualizer'; import styles from '@adobe/spectrum-css-temp/components/menu/vars.css'; import {useCollator, useMessageFormatter} from '@react-aria/i18n'; From b63eb81819876ae1668ce9be99b54e7dbd1d9639 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Mon, 8 Jun 2020 22:32:53 -0700 Subject: [PATCH 2/6] remove unused lines from debugging --- packages/@react-aria/select/src/useSelect.ts | 2 -- packages/@react-spectrum/listbox/src/ListBoxBase.tsx | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/@react-aria/select/src/useSelect.ts b/packages/@react-aria/select/src/useSelect.ts index 6e58c7be12a..f6930e67dee 100644 --- a/packages/@react-aria/select/src/useSelect.ts +++ b/packages/@react-aria/select/src/useSelect.ts @@ -20,7 +20,6 @@ import {SelectState} from '@react-stately/select'; import {useCollator} from '@react-aria/i18n'; import {useLabel} from '@react-aria/label'; import {useMenuTrigger} from '@react-aria/menu'; -import {usePress} from "@react-aria/interactions"; interface AriaSelectOptions extends AriaSelectProps { /** @@ -85,7 +84,6 @@ export function useSelect(props: AriaSelectOptions, state: SelectState, let triggerProps = mergeProps(mergeProps(menuTriggerProps, fieldProps), typeSelectProps); let valueId = useId(); - return { labelProps, triggerProps: mergeProps(domProps, { diff --git a/packages/@react-spectrum/listbox/src/ListBoxBase.tsx b/packages/@react-spectrum/listbox/src/ListBoxBase.tsx index 2e7997292c4..b8e9d7a46e1 100644 --- a/packages/@react-spectrum/listbox/src/ListBoxBase.tsx +++ b/packages/@react-spectrum/listbox/src/ListBoxBase.tsx @@ -22,7 +22,7 @@ import {ListLayout} from '@react-stately/layout'; import {ListState} from '@react-stately/list'; import {mergeProps} from '@react-aria/utils'; import {ProgressCircle} from '@react-spectrum/progress'; -import React, {HTMLAttributes, ReactElement, RefObject, useMemo, useState} from 'react'; +import React, {HTMLAttributes, ReactElement, RefObject, useMemo} from 'react'; import {ReusableView} from '@react-stately/virtualizer'; import styles from '@adobe/spectrum-css-temp/components/menu/vars.css'; import {useCollator, useMessageFormatter} from '@react-aria/i18n'; From 8f3efad28db9e085aadd4f530f1a846760d77dba Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Tue, 9 Jun 2020 09:59:05 -0700 Subject: [PATCH 3/6] Handle disabled case, add comment explaining why I'm doing this, fix tests --- packages/@react-aria/listbox/src/useOption.ts | 27 ++++++++++--------- .../picker/test/Picker.test.js | 5 +++- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/packages/@react-aria/listbox/src/useOption.ts b/packages/@react-aria/listbox/src/useOption.ts index f9485b2c428..ed1d43bb72c 100644 --- a/packages/@react-aria/listbox/src/useOption.ts +++ b/packages/@react-aria/listbox/src/useOption.ts @@ -10,11 +10,10 @@ * governing permissions and limitations under the License. */ -import {HTMLAttributes, Key, RefObject, useState} from 'react'; -import {isFocusVisible} from '@react-aria/interactions'; +import {HTMLAttributes, Key, RefObject} from 'react'; import {ListState} from '@react-stately/list'; import {mergeProps, useSlotId} from '@react-aria/utils'; -import {useHover, usePress} from '@react-aria/interactions'; +import {usePress} from '@react-aria/interactions'; import {useSelectableItem} from '@react-aria/selection'; interface OptionAria { @@ -93,20 +92,22 @@ export function useOption(props: AriaOptionProps, state: ListState, ref: R }); let {pressProps} = usePress({...itemProps, isDisabled}); - let {hoverProps} = useHover({ - isDisabled: isDisabled || !shouldFocusOnHover, - }); + // use mouse movement as opposed to hover to determine if we should focus the option + // hover can't distinguish between the mouse being moved over the object vs the object being moved under the mouse + // we only want the case where the mouse moved over the object + let onMovement = () => { + if (state.selectionManager.focusedKey !== key && !(isDisabled || !shouldFocusOnHover)) { + state.selectionManager.setFocused(true); + state.selectionManager.setFocusedKey(key); + } + }; return { optionProps: { ...optionProps, - ...mergeProps(mergeProps(pressProps, hoverProps), { - onMouseMove() { - if (state.selectionManager.focusedKey !== key) { - state.selectionManager.setFocused(true); - state.selectionManager.setFocusedKey(key); - } - } + ...mergeProps(pressProps, { + onMouseMove: onMovement, + onPointerMove: onMovement }) }, labelProps: { diff --git a/packages/@react-spectrum/picker/test/Picker.test.js b/packages/@react-spectrum/picker/test/Picker.test.js index f9568898e6a..5d4ad2529b8 100644 --- a/packages/@react-spectrum/picker/test/Picker.test.js +++ b/packages/@react-spectrum/picker/test/Picker.test.js @@ -986,7 +986,10 @@ describe('Picker', function () { expect(document.activeElement).toBe(listbox); - act(() => {fireEvent.mouseEnter(items[1]);}); + act(() => { + fireEvent.mouseEnter(items[1]); + fireEvent.mouseMove(items[1]); + }); expect(document.activeElement).toBe(items[1]); act(() => {fireEvent.keyDown(listbox, {key: 'ArrowDown'});}); From 4aa10937ec58ca94bdf87ce618168e8661861d00 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Tue, 9 Jun 2020 11:18:05 -0700 Subject: [PATCH 4/6] Use modality instead --- packages/@react-aria/listbox/src/useOption.ts | 24 +++++++++---------- .../picker/test/Picker.test.js | 5 +--- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/packages/@react-aria/listbox/src/useOption.ts b/packages/@react-aria/listbox/src/useOption.ts index ed1d43bb72c..84430bae75d 100644 --- a/packages/@react-aria/listbox/src/useOption.ts +++ b/packages/@react-aria/listbox/src/useOption.ts @@ -13,7 +13,7 @@ import {HTMLAttributes, Key, RefObject} from 'react'; import {ListState} from '@react-stately/list'; import {mergeProps, useSlotId} from '@react-aria/utils'; -import {usePress} from '@react-aria/interactions'; +import {isFocusVisible, useHover, usePress} from '@react-aria/interactions'; import {useSelectableItem} from '@react-aria/selection'; interface OptionAria { @@ -92,23 +92,21 @@ export function useOption(props: AriaOptionProps, state: ListState, ref: R }); let {pressProps} = usePress({...itemProps, isDisabled}); - // use mouse movement as opposed to hover to determine if we should focus the option - // hover can't distinguish between the mouse being moved over the object vs the object being moved under the mouse - // we only want the case where the mouse moved over the object - let onMovement = () => { - if (state.selectionManager.focusedKey !== key && !(isDisabled || !shouldFocusOnHover)) { - state.selectionManager.setFocused(true); - state.selectionManager.setFocusedKey(key); + + let {hoverProps} = useHover({ + isDisabled: isDisabled || !shouldFocusOnHover, + onHoverStart() { + if (!isFocusVisible()) { + state.selectionManager.setFocused(true); + state.selectionManager.setFocusedKey(key); + } } - }; + }); return { optionProps: { ...optionProps, - ...mergeProps(pressProps, { - onMouseMove: onMovement, - onPointerMove: onMovement - }) + ...mergeProps(pressProps, hoverProps) }, labelProps: { id: labelId diff --git a/packages/@react-spectrum/picker/test/Picker.test.js b/packages/@react-spectrum/picker/test/Picker.test.js index 5d4ad2529b8..f9568898e6a 100644 --- a/packages/@react-spectrum/picker/test/Picker.test.js +++ b/packages/@react-spectrum/picker/test/Picker.test.js @@ -986,10 +986,7 @@ describe('Picker', function () { expect(document.activeElement).toBe(listbox); - act(() => { - fireEvent.mouseEnter(items[1]); - fireEvent.mouseMove(items[1]); - }); + act(() => {fireEvent.mouseEnter(items[1]);}); expect(document.activeElement).toBe(items[1]); act(() => {fireEvent.keyDown(listbox, {key: 'ArrowDown'});}); From bb21e8491bb55bde2606aeddf28abb2389884643 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Tue, 9 Jun 2020 11:18:46 -0700 Subject: [PATCH 5/6] fix lint --- packages/@react-aria/listbox/src/useOption.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@react-aria/listbox/src/useOption.ts b/packages/@react-aria/listbox/src/useOption.ts index 84430bae75d..a963a1089c4 100644 --- a/packages/@react-aria/listbox/src/useOption.ts +++ b/packages/@react-aria/listbox/src/useOption.ts @@ -11,9 +11,9 @@ */ import {HTMLAttributes, Key, RefObject} from 'react'; +import {isFocusVisible, useHover, usePress} from '@react-aria/interactions'; import {ListState} from '@react-stately/list'; import {mergeProps, useSlotId} from '@react-aria/utils'; -import {isFocusVisible, useHover, usePress} from '@react-aria/interactions'; import {useSelectableItem} from '@react-aria/selection'; interface OptionAria { From 465d319517f5f9d45031cc6a86f950fa5c4b04e0 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Wed, 10 Jun 2020 14:50:55 -0700 Subject: [PATCH 6/6] add same logic to useMenuItem --- packages/@react-aria/menu/src/useMenuItem.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/@react-aria/menu/src/useMenuItem.ts b/packages/@react-aria/menu/src/useMenuItem.ts index 017fd8b3691..2677c824150 100644 --- a/packages/@react-aria/menu/src/useMenuItem.ts +++ b/packages/@react-aria/menu/src/useMenuItem.ts @@ -11,10 +11,10 @@ */ import {HTMLAttributes, Key, RefObject} from 'react'; +import {isFocusVisible, useHover, usePress} from '@react-aria/interactions'; import {mergeProps, useSlotId} from '@react-aria/utils'; import {PressEvent} from '@react-types/shared'; import {TreeState} from '@react-stately/tree'; -import {useHover, usePress} from '@react-aria/interactions'; import {useSelectableItem} from '@react-aria/selection'; interface MenuItemAria { @@ -149,16 +149,17 @@ export function useMenuItem(props: AriaMenuItemProps, state: TreeState, re let {hoverProps} = useHover({ isDisabled, onHoverStart() { - state.selectionManager.setFocused(true); - state.selectionManager.setFocusedKey(key); + if (!isFocusVisible()) { + state.selectionManager.setFocused(true); + state.selectionManager.setFocusedKey(key); + } } }); return { menuItemProps: { ...ariaProps, - ...pressProps, - ...hoverProps + ...mergeProps(pressProps, hoverProps) }, labelProps: { id: labelId