Add support for long press to MenuTrigger #780 #789

intergalacticspacehighway · 2020-07-19T19:31:40Z

Closes

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Run MenuTrigger.test.js to run trigger="longPress" test suite.

🧢 Your Project:

None

intergalacticspacehighway · 2020-07-20T03:56:22Z

Hi @devongovett, just signed the CLA and pushed lint fixes 😅 . Let me know if there are any changes needed.

snowystinger

Thanks for working on this, great work so far :) couple of notes on the tests.

snowystinger · 2020-07-20T16:27:25Z

packages/@react-spectrum/menu/test/MenuTrigger.test.js

+          triggerPress(button);
+        } else {
+          act(() => {
+            fireEvent.keyDown(button, {key: ' '});


I believe fireEvent will already wrap in an act, you should just be able to wrap the runAllTimers in it. Followup to that statement, is this runAllTimers needed?

react-spectrum/packages/@react-spectrum/menu/test/MenuTrigger.test.js

Line 112 in aba3652

jest.runAllTimers();

Can we get a keyUp event as well, after the timer? that way we are more like real behavior in the browser
These aren't quite testing longPress, we're running all timers, which is the same thing we did for all the previous non-longPress tests. I think we should change it up a bit and only run the timers as far as we need, or we need to verify how far we advanced in time.
We don't need the it.each syntax, we only have one line in it.

Same comments for the following tests

devongovett

Great start! Left a few suggestions. 😄

devongovett · 2020-07-21T04:55:01Z

packages/@react-aria/menu/src/useMenuTrigger.ts

 * @param state - State for the menu trigger.
 */
-export function useMenuTrigger(props: MenuTriggerAriaProps, state: MenuTriggerState, ref: RefObject<HTMLElement>): MenuTriggerAria {
+export function useMenuTrigger(props: MenuTriggerAriaProps, state: MenuTriggerState, ref: RefObject<HTMLElement>, trigger?: MenuTriggerType): MenuTriggerAria {


I think the trigger argument should be moved to a property of the props object.

devongovett · 2020-07-21T04:56:36Z

packages/@react-aria/interactions/src/useLongPress.ts

+// Portions of the code in this file are based on code from react.
+// Original licensing for the following can be found in the
+// NOTICE file in the root directory of this source tree.
+// See https://github.com/facebook/react/tree/cc7c1aece46a6b69b41958d731e0fd27c94bfc6c/packages/react-interactions


I don't think this comment is necessary. There's no useLongPress hook in react-interactions as far as I can tell.

devongovett · 2020-07-21T05:02:19Z

packages/@react-aria/interactions/src/useLongPress.ts

+
+  let {pressProps} = usePress({
+    onPressStart(e) {
+      timeRef.current = setTimeout(() => {


I think we should only handle long press events for touch and mouse pointer types. Right now I noticed you can press and hold the enter or space keys on the keyboard as well. You could add a check here for e.pointerType === 'mouse' || e.ponterType === 'touch'.

I agree with @devongovett, Enter and Space keys are tricky for a longPress on a menu trigger that opens a menu and sets focus to the first menuitem, because the keyup event might also trigger selection on the menuitem that received focus.

devongovett · 2020-07-21T05:04:31Z

packages/@react-aria/menu/src/useMenuTrigger.ts

-      },
-      onKeyDown
+  const longPressProps = useLongPress({
+    onLongPress(e) {


If you make the change above and only allow mouse and touch pointer types for long presses, you could always call state.toggle('first') here.

devongovett · 2020-07-21T05:06:21Z

packages/@react-aria/menu/src/useMenuTrigger.ts

+  if (trigger === 'longPress') {
+    menuTriggerProps = mergeProps(menuTriggerProps, longPressProps, {onKeyDown});
+  } else {
+    menuTriggerProps = mergeProps(menuTriggerProps, pressProps, {onKeyDown});
+  }


Suggested change

if (trigger === 'longPress') {

menuTriggerProps = mergeProps(menuTriggerProps, longPressProps, {onKeyDown});

} else {

menuTriggerProps = mergeProps(menuTriggerProps, pressProps, {onKeyDown});

}

menuTriggerProps = mergeProps(menuTriggerProps, trigger === 'longPress' ? longPressProps : pressProps, {onKeyDown});

devongovett · 2020-07-21T05:15:42Z

@intergalacticspacehighway one other thing that's a bit tricky: I noticed that the onPress event still fires after a long press. I think we need to find a way to suppress that. For example, a button could have both a press action as well as opening a menu on long press, so after long pressing, the press action shouldn't fire.

@majornista what's the best way to indicate to screen readers that a long press action is available? VoiceOver still reads that the button has a popup, but I guess we should indicate that there's both a primary click action as well as a long press action. Double tap and hold seems to work to activate the menu. Is there a pattern here that we should follow?

majornista · 2020-07-24T22:29:59Z

@devongovett and @intergalacticspacehighway, I'm not sure there is a "best way" to indicate to screen readers that a long press action is available, particularly for mobile screen reader users. MenuTrigger already provides the aria-haspopup attribute to the trigger, so a screen reader user should know that a menu is available, but to communicate that it opens differently than other menus, we may need to add some additional instruction. A visual "Hold icon" affordance has been defined in Spectrum https://spectrum.adobe.com/page/action-button/#Hold-icon, perhaps we could add an aria-label with instructional text like, "Press and hold to expand menu" to this icon and reference it by id using aria-describedby on the parent button. Depending on context, it may make more sense on mobile to simply toggle the menu without a long press when an item is already selected in a toolbar.

cc @jnurthen

…k example

…#780

…ongPress #780

intergalacticspacehighway · 2020-08-03T13:35:53Z

@intergalacticspacehighway one other thing that's a bit tricky: I noticed that the onPress event still fires after a long press. I think we need to find a way to suppress that. For example, a button could have both a press action as well as opening a menu on long press, so after long pressing, the press action shouldn't fire.

@devongovett The onPress event is fired as it's passed in the ActionButton as a prop in the story book example. I think someone might pass it deliberately. Do you think we should prevent it?
Yeah, it's a bit tricky as ActionButton has no idea of the trigger="longPress" and it uses useButton which triggers the onPress. Apologies for the delay in getting back and thanks for reviewing the PR.

devongovett · 2020-08-13T04:02:59Z

Yeah I think we can move forward without fixing that issue and we can think about it more after this PR is merged.

…stcases

…ng test cases

intergalacticspacehighway · 2020-08-15T11:09:15Z

packages/@react-spectrum/test-utils/package.json

  },
  "dependencies": {
    "@babel/runtime": "^7.6.2",
+    "@react-aria/interactions": "3.1.0",


Created a constant in useLongPress for long press threshold which is used in MenuTrigger test-case

intergalacticspacehighway · 2020-08-15T11:15:36Z

packages/@react-aria/menu/src/useMenuTrigger.ts

+  const longPressProps = useLongPress({
+    // Close on press start as menu can be in a open state after onLongPress.
+    onPressStart() {
+      state.close();


Recent changes for fixing #925 updated the behaviour of useOverlay. Earlier, onBlurWithin used to get called which calls onClose but now the toggle is only handled by onPressStart from pressProps. (For trigger buttons)
To replicate a similar behavior, I added state.close here. Let me know if it can be done in a better way.

dannify · 2021-12-23T20:52:21Z

Thanks for getting this started. Most of this code was pulled into other PRs done by the team.

intergalacticspacehighway requested review from dannify, devongovett and snowystinger as code owners July 19, 2020 19:31

snowystinger reviewed Jul 20, 2020

View reviewed changes

devongovett reviewed Jul 21, 2020

View reviewed changes

nishan added 9 commits July 27, 2020 20:59

Add support for long press to MenuTrigger #780

1180de2

Add support for long press to MenuTrigger #780 | Cleanup and Storyboo…

c9ce0aa

…k example

use mergeProps - MenuTrigger longPress #780

50b4938

Remove unnecessary condition from useLongPress - MenuTrigger longPress …

8af1223

…#780

Remove unnecessary prop definitions from useLongPress - MenuTrigger l…

665deec

…ongPress #780

Updated testcases- MenuTrigger longPress #780

437b01f

Updated documentation - MenuTrigger longPress #780

8a3516f

Copyright text in useLongPress - MenuTrigger longPress #780

0e7fb6d

Lint fixes - #780

a8d1db1

Nishan added 3 commits August 15, 2020 14:54

MenuTrigger longPress #780 - only mouse or touch events and update te…

d531280

…stcases

MenuTrigger longPress #780 - Merge conflicts resolved and fixed faili…

08c165c

…ng test cases

MenuTrigger longPress #780 - Merge conflicts resolved with main

c8046ea

intergalacticspacehighway commented Aug 15, 2020

View reviewed changes

intergalacticspacehighway requested review from devongovett and snowystinger August 15, 2020 11:22

Nishan and others added 4 commits September 10, 2020 12:48

780-longpress | merge conflicts resolved

9580958

780-longpress | Lint issues fix

9af5afc

Merge branch 'main' into 780-longpress-menuTrigger

94ad476

Fix lint error

24bb97f

Michael Jordan added 2 commits January 11, 2021 14:28

Fix lint error

43f1c0b

780-longpress-menuTrigger | Fix lint errors

f078f9f

devongovett mentioned this pull request Sep 21, 2021

Add support for highlight selection and onAction to TableView and ListView #2363

Merged

3 tasks

dannify closed this Dec 23, 2021

Add support for long press to MenuTrigger #780 #789

Add support for long press to MenuTrigger #780 #789

Uh oh!

Conversation

intergalacticspacehighway commented Jul 19, 2020

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

intergalacticspacehighway commented Jul 20, 2020

Uh oh!

snowystinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devongovett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devongovett commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

majornista commented Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

intergalacticspacehighway commented Aug 3, 2020

Uh oh!

devongovett commented Aug 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

intergalacticspacehighway Aug 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannify commented Dec 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

devongovett commented Jul 21, 2020 •

edited

Loading

majornista commented Jul 24, 2020 •

edited

Loading

intergalacticspacehighway Aug 15, 2020 •

edited

Loading