Grid edit mode #2375

LFDanLu · 2021-09-23T18:27:24Z

Closes #2328

Some known issues:

The tableview example with editable cells renders a bit strangely on Safari and android (the searchfield shifts when typing in it)
pageup/down/home/end will scroll the table when used inside a searchfield

✅ 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:

🧢 Your Project:

… when inside editable cell

adobe-bot · 2021-09-23T18:49:54Z

Build successful! 🎉

…ahead if in edit mode

…edit mode also moved some edit mode logic out of keydowncapture in useGridCell and into useSelectableCollection so it is in a more general place

adobe-bot · 2021-09-28T01:43:08Z

Build successful! 🎉

also stopping arrow keys from scrolling the table when edit mode is active. Not sure why page up/down/home/end still scroll if focused on a searchfield

… and tooltips

…he DOM

LFDanLu · 2021-09-29T16:48:41Z

packages/@react-aria/selection/src/useSelectableCollection.ts

      case 'Home':
-        if (delegate.getFirstKey) {
+        if (editModeEnabled) {
+          // Prevent scrolling from happening if in edit mode
+          // TODO: not sure why this doesn't stop table scrolling when in a searchfield in editmode...
+          e.preventDefault();
+        } else if (delegate.getFirstKey) {


I believe this can be fixed with an approach like #1519, but will need to discuss if we want to prevent that behavior on all components that use useTextFields

Additionally, scrolling will still happen if the user hits home/end when the cursor is at the start/end of the input text, I figure we will want to suppress that. Open to opinions

turns out we need the stop propagation so that row selection does not happen if you click on a textfield in a editable cell for the first time

adobe-bot · 2021-09-29T19:34:11Z

Build successful! 🎉

LFDanLu · 2021-09-29T20:29:13Z

packages/@react-aria/textfield/src/useTextField.ts

+  // TODO: should we support press events on textfield?
+  // This is added here so that pressing on a textfield in a table properly focuses the textfield
+  // and that selection doesn't trigger since usePress will stop propagation. However, usePress prevents default
+  // and thus stops text selection via mouse from working....
+  // let {pressProps} = usePress({
+  //   ref
+  // });
+  // The below is an alternative that allows the user to click into a textfield in a editable cell
+  let onPointerDown = (e) => {
+    ref.current.focus();
+    e.stopPropagation();
+  };
+
+


Kinda iffy on this. If we don't manually focus the textfield on pointer down, clicking on a textfield in a tableview won't move focus into it. Buttons and other elements don't have this issue because they have usePress which manually focuses the element on pointer down, but we don't really want to use usePress in textfields because it will preventDefault and stop text selection via mouse.

Open to opinions on this, personally I want to have this focus behavior so that the user can do the following cases:

click into a textfield in a tableview without needing to activate edit mode on the cell via keyboard first (this is consistent with how other components like buttons behave in a tableview right now)

move focus between textfields in a tableview via clicking

Yeah this feels weird though. It's not clear you're in edit mode and can't move focus out of the cell without pressing escape. 🤔

Let's gather more opinions here.

adobe-bot · 2021-09-29T23:28:30Z

Build successful! 🎉

devongovett

initial comments

devongovett · 2021-10-01T22:08:26Z

packages/@react-aria/grid/src/useGridCell.ts

    }

-    let walker = getFocusableTreeWalker(ref.current);
+    let walker = getFocusableTreeWalker(ref.current, {tabbable: true});


We could probably use createFocusManager here? It has focusNext and focusPrevious functions which are a bit nicer to work with than tree walker.

Hrm, the ArrowLeft/ArrowRight logic relies on checking if the next element is a child node or not, but createFocusManager's focusNext and focusPrevious will attempt to focus stuff immediately

devongovett · 2021-10-01T22:17:50Z

packages/@react-aria/grid/src/useGridCell.ts

        state.selectionManager.setFocusedKey(node.key);
      }
+
+      // Activating edit mode if user focuses a child of the editable cell (e.g. via click)


Still not sure about this one. It feels strange. You don't realize you're in edit mode at all... 🤔

devongovett · 2021-10-01T22:41:44Z

packages/@react-aria/grid/src/useGridRow.ts

  let {pressProps} = usePress({...itemProps, isDisabled});

+  let containsEditCell = state.collection.getItem(state.editModeKey)?.parentKey === node.key;
+  if (containsEditCell) {


In #2363 I added an isDisabled prop to useSelectableItem which does the same thing as this. Could probably combine those.

Sounds good, I'll pull the bare minimum isDisabled changes from that PR so it doesn't break behavior here

Never mind, looking at it further it might be best if I wait till that PR goes in
Note to self regarding the change to be made:

remove the if statement and its contents

pass isDisabled to useSelectableItem if containsEditCell

devongovett · 2021-10-01T22:44:42Z

packages/@react-aria/selection/src/useSelectableCollection.ts

+  /**
+   * Whether something in the collection is in edit mode and should disable typeahead and other collection keyboard behavior.
+   */
+  editModeEnabled?: boolean


Maybe we can call this isDisabled? Not sure I want edit mode specific stuff to leak into the general useSelectableCollection since it's pretty grid specific...

devongovett · 2021-10-01T22:59:06Z

packages/@react-aria/textfield/src/useTextField.ts

+  // TODO: should we support press events on textfield?
+  // This is added here so that pressing on a textfield in a table properly focuses the textfield
+  // and that selection doesn't trigger since usePress will stop propagation. However, usePress prevents default
+  // and thus stops text selection via mouse from working....
+  // let {pressProps} = usePress({
+  //   ref
+  // });
+  // The below is an alternative that allows the user to click into a textfield in a editable cell
+  let onPointerDown = (e) => {
+    ref.current.focus();
+    e.stopPropagation();
+  };
+
+


Yeah this feels weird though. It's not clear you're in edit mode and can't move focus out of the cell without pressing escape. 🤔

Let's gather more opinions here.

devongovett · 2021-10-01T23:02:23Z

packages/@react-spectrum/card/src/BaseLayout.tsx

    }

+    // If there is an active editable card, make sure it remains in the DOM even when scrolled out of view
+    // so that we don't lose the card's focused child. This really only matters for virtualized card view


Wonder how this affects screen readers. There'd be a gap in the rows that are "visible" potentially. We should test this.

LFDanLu · 2021-10-11T18:02:38Z

packages/@react-spectrum/table/src/TableView.tsx

  } = useFocusRing({within: true});
  let {isFocusVisible, focusProps} = useFocusRing();
  let {hoverProps, isHovered} = useHover({isDisabled});
+  let containsEditCell = state.collection.getItem(state.editModeKey)?.parentKey === item.key;


Note to self:
If #2363 is merged with the useSelectableItem returning isPressed, perhaps we can get rid of the usePress above in TableRow? Will need to propagate isDisabled/containsEditCell through useTableRow -> useGridRow perhaps?

adobe-bot · 2021-10-11T18:04:32Z

Build successful! 🎉

LFDanLu · 2021-10-11T19:02:48Z

packages/@react-aria/selection/src/useSelectableCollection.ts

+          // Prevent scrolling from happening if in edit mode
+          // TODO: not sure why this doesn't stop table scrolling when in a searchfield in editmode...
+          e.preventDefault();


Can stop scrolling if we do something similar to #1519 and always prevent default on Home/End/PageUp/PageDown but we'd only want to do this if it were in a grid cell in edit mode...

Alternatively we could add Home/End/PageUp/PageDown cases to the keydown in useGridCell and prevent default if we are in edit mode but then this would disable default behavior of these keydowns on the child elements...

LFDanLu · 2022-03-02T20:52:30Z

Closing for now, will revisit in the future

LFDanLu added 2 commits September 23, 2021 09:52

initial progress on edit mode

be70493

preventing selection on edit mode cells and fixing entering edit mode…

9bae662

… when inside editable cell

LFDanLu requested review from dannify, devongovett and snowystinger as code owners September 23, 2021 18:27

fixing tests

5d05d4e

LFDanLu added 4 commits September 27, 2021 16:36

tracking the current edit mode cell in grid state and disabling type …

07f598c

…ahead if in edit mode

making it so user need to escape twice to clear searchfield and exit …

43fe231

…edit mode also moved some edit mode logic out of keydowncapture in useGridCell and into useSelectableCollection so it is in a more general place

fixing lint and making cards editable

8757405

fix test

ac723d8

LFDanLu added 7 commits September 28, 2021 10:40

Merge branch 'main' of github.com:adobe/react-spectrum into issue_2328

0e39b0c

suppressing useSelectableCollection behavior if editmode is enabled

62c810d

also stopping arrow keys from scrolling the table when edit mode is active. Not sure why page up/down/home/end still scroll if focused on a searchfield

making user have to press escape twice to leave editMode for combobox…

cffe7ef

… and tooltips

activating edit mode if user clicks on a child of a editiable cell

4f997ed

updating table and card layouts so that active edit cells remain in t…

2e97488

…he DOM

updating useGridCell edit mode tab behavior to skip tabindex=-1 children

356c2a3

hacky text selection enablement

ab65f47

LFDanLu commented Sep 29, 2021

View reviewed changes

LFDanLu added 3 commits September 29, 2021 11:51

removing uneeded stop propagation

7dcfd6a

bringing back stop propagation to stop row selection in editable cell

525974f

turns out we need the stop propagation so that row selection does not happen if you click on a textfield in a editable cell for the first time

fixing lint and tests

8b6d3e7

LFDanLu commented Sep 29, 2021

View reviewed changes

adding tests

7c3c746

LFDanLu changed the title ~~(WIP) Grid edit mode~~ Grid edit mode Sep 29, 2021

devongovett reviewed Oct 1, 2021

View reviewed changes

changing useSelectableCollection editModeEnabled to isDisabled

09babad

LFDanLu commented Oct 11, 2021

View reviewed changes

LFDanLu closed this Mar 2, 2022

LFDanLu deleted the issue_2328 branch January 11, 2023 01:35

Grid edit mode #2375

Grid edit mode #2375

Uh oh!

Conversation

LFDanLu commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

adobe-bot commented Sep 23, 2021

Uh oh!

adobe-bot commented Sep 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Sep 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Sep 29, 2021

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!

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!

adobe-bot commented Oct 11, 2021

Uh oh!

LFDanLu Oct 11, 2021 • 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!

LFDanLu commented Mar 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LFDanLu commented Sep 23, 2021 •

edited

Loading

LFDanLu Oct 11, 2021 •

edited

Loading