Skip to content

Add separate mode for triggering resizing manually via Enter#4061

Merged
LFDanLu merged 9 commits into
aria_table_docsfrom
aria_table_resize_approach_2
Feb 17, 2023
Merged

Add separate mode for triggering resizing manually via Enter#4061
LFDanLu merged 9 commits into
aria_table_docsfrom
aria_table_resize_approach_2

Conversation

@LFDanLu
Copy link
Copy Markdown
Member

@LFDanLu LFDanLu commented Feb 14, 2023

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:

🧢 Your Project:

RSP

@LFDanLu LFDanLu changed the base branch from main to aria_table_docs February 14, 2023 21:52
border: '2px',
borderStyle: 'none solid',
borderColor: layoutState.resizingColumn === column.key ? 'orange' : 'grey',
borderColor: layoutState.resizingColumn === column.key || isFocusVisible ? 'orange' : 'grey',
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

One problem with this approach: how to convey to the user the difference between the resizer having focus vs the resizer being "active"/movable via the arrow keys? At the moment there isn't a difference visually, not sure the best way to do so tbh since usually a slider is always "active"

Comment thread packages/@react-aria/table/src/useTableColumnResize.ts
@rspbot
Copy link
Copy Markdown

rspbot commented Feb 14, 2023

}

export interface MoveProps extends MoveEvents {
disableLeftRightHandling?: boolean
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

When editMode is disabled, I need the keyboard events for ArrowLeft/Right to bubble up to useSelectableCollection to handle moving from table header to table header. They get propagation stopped below if I don't special case it here somehow.

Handling it here also means I can also stop triggerKeyboardMove from being called (aka stopping resizing from handling on ArrowRight) but I could handle that in the onMove handlers provided to useMove

@rspbot
Copy link
Copy Markdown

rspbot commented Feb 16, 2023

@rspbot
Copy link
Copy Markdown

rspbot commented Feb 16, 2023

Comment on lines -1261 to -1263
expect(onResizeEnd).toHaveBeenCalledTimes(1);
expect(onResizeEnd).toHaveBeenCalledWith(new Map<string, ColumnSize>([['foo', 600], ['bar', '1fr'], ['baz', '1fr']]));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The changes made in this PR mean that onResizeStart/End aren't triggered unless a resize operation actually happens. Entering and leaving resize mode without changing the size doesn't cause those event to fire.

Open to opinions on this change. Note that If we want to preserve this behavior then we'll need a new prop like in a691280 so we can start resizing when the resizer is focused by some external action

LFDanLu added a commit that referenced this pull request Feb 16, 2023
split out from #4061, see that PR for more details and alternative approaches
@rspbot
Copy link
Copy Markdown

rspbot commented Feb 16, 2023

@rspbot
Copy link
Copy Markdown

rspbot commented Feb 16, 2023

## API Changes

unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }

@internationalized/date

parseDuration

-parseDuration {
-  value: string
-  returnVal: undefined
-}
+

@react-stately/layout

TableLayout

 TableLayout<T> {
   addVisibleLayoutInfos: (Array<LayoutInfo>, LayoutNode, Rect) => void
   binarySearch: (Array<LayoutNode>, Point, 'x' | 'y') => void
   buildBody: (number) => LayoutNode
   buildCell: (GridNode<T>, number, number) => LayoutNode
   buildCollection: () => Array<LayoutNode>
   buildColumn: (GridNode<T>, number, number) => LayoutNode
   buildHeader: () => LayoutNode
   buildHeaderRow: (GridNode<T>, number, number) => LayoutNode
   buildNode: (GridNode<T>, number, number) => LayoutNode
   buildPersistedIndices: () => void
   buildRow: (GridNode<T>, number, number) => LayoutNode
   collection: TableCollection<T>
   columnLayout: TableColumnLayout<T>
   columnWidths: Map<Key, number>
   constructor: (TableLayoutOptions<T>) => void
   controlledColumns: Map<Key, GridNode<unknown>>
-  endResize: () => void
   getColumnMaxWidth: (Key) => number
   getColumnMinWidth: (Key) => number
   getColumnWidth: (Key) => number
   getEstimatedHeight: (GridNode<T>, number, number, number) => void
   getInitialLayoutInfo: (LayoutInfo) => void
   getRenderedColumnWidth: (GridNode<T>) => void
   getResizerPosition: () => Key
   getVisibleLayoutInfos: (Rect) => void
   isLoading: any
   lastCollection: TableCollection<T>
   lastPersistedKeys: Set<Key>
+  onColumnResize: (Key, number) => Map<Key, ColumnSize>
+  onColumnResizeEnd: () => void
+  onColumnResizeStart: (Key) => void
   persistedIndices: Map<Key, Array<number>>
   resizingColumn: Key | null
   setChildHeights: (Array<LayoutNode>, number) => void
-  startResize: (Key) => void
   stickyColumnIndices: Array<number>
   uncontrolledColumns: Map<Key, GridNode<unknown>>
   uncontrolledWidths: Map<Key, ColumnSize>
-  updateResizedColumns: (Key, number) => Map<Key, ColumnSize>
   wasLoading: any
 }

@react-stately/table

TableColumnResizeState

 TableColumnResizeState<T> {
-  endResize: () => void
   getColumnMaxWidth: (Key) => number
   getColumnMinWidth: (Key) => number
   getColumnWidth: (Key) => number
+  onColumnResize: (Key, number) => Map<Key, ColumnSize>
+  onColumnResizeEnd: () => void
+  onColumnResizeStart: (Key) => void
   resizingColumn: Key | null
-  startResize: (Key) => void
   tableState: TableState<T>
-  updateResizedColumns: (Key, number) => Map<Key, ColumnSize>
   widths: Map<Key, number>
 }

it changed:

  • useTableColumnResizeState

this make it so the user would have to hit Enter to start resizing. This allows us to have the resizers always be visible and still preserve grid navigation between columns.
…via menu

still one bug where manager.isFocused is set to false for somereason after confirming the resize
fixes bug where selection manager would stop tracking if the manager is focused or not if keyboard nav was disabled. This was a bug where the user wouldnt be able to move table focus after confirming a resize operation. Adds new prop to useTableColumnResize for triggering resizeStart if resizer if focused. This is needed for our table since we dont have a way to call resizeStart programmatically out side of useTableColumnResize and we dont want to call it on focus for the aria example since that should require a manual trigger of Enter to enter resize mode
turns out the simulated resize operation was blurring before pressEnd finished
also updates copy and handles case where trigger ref isnt provided
@LFDanLu LFDanLu force-pushed the aria_table_resize_approach_2 branch from a6db6a3 to d8cf944 Compare February 17, 2023 01:06
@LFDanLu LFDanLu changed the title (WIP) Add separate mode for triggering resizing manually via Enter Add separate mode for triggering resizing manually via Enter Feb 17, 2023
@LFDanLu
Copy link
Copy Markdown
Member Author

LFDanLu commented Feb 17, 2023

Gonna merge this into the original aria table docs branch since we've decided on the path forward for the aria example. Will get rid of code changes once #4077 is finalized

@LFDanLu LFDanLu merged commit 7353190 into aria_table_docs Feb 17, 2023
@LFDanLu LFDanLu deleted the aria_table_resize_approach_2 branch February 17, 2023 01:13
LFDanLu added a commit that referenced this pull request Mar 17, 2023
#4077)

* Updating column resize to support mode where resizer is always visible

split out from #4061, see that PR for more details and alternative approaches

* update to match latest changes to api

* mimic docs example

* forgot to clean up some things

* pulling in code changes from docs PR

get rid of inline styles and fix case where there isnt a separate trigger for starting column resize

* remove sorting story and cleanup

* starting resize on press for indicator

this unfortunately causes a difference in behavior between starting a drag on menu (no resizestart until move) and starting a drag via mouse/touch (resizestart immediately on press)

* using triggerRef existance to determine if behavior is resize on focus

one test is still breaking, debugging

* fixing test

test would blur on rerender causing a column width update even though resizing wasnt happening. Changed conditonal so calling endResize only causes value updates if we are resizing

* make resizer single line for focus

* nit reorganizing

* mimic docs example

remove selection from example to mirror docs

* adding description for keyboard users for Enter to start resizing

this is for the aria table example where resizing is entered manually via Enter while focused on the resizer

* fixing issue where tab wasnt exiting the table when focused on the reizer

* adding min width for columns to avoid weirdness with trying to collapse 0

* fix lint

* propagate all keydown events if we arent in resize mode and have always visible resizers

forgot that we also have other keyboard combos like cmd + a or escape that should also be handled by useSelectableCollection

* removing ref read in render

* add aria description to input for virtual modality too

if the user enters the table using control+option+arrow keys in voiceover, they will be virtual modality so we want the description for press enter to resize to be audible there

* addressing review comments

* prevent extraneous scrolling when keyboard navigating to the resizer

margin applied on the visually hidden input makes scrollIntoView think it needs to scroll it

* address review comments

* fix logic
LFDanLu added a commit that referenced this pull request Apr 5, 2023
* progress for making aria table resizing example

* styling updates and addition of menu button for resizing

* fixing it so user doesnt focus the resizer immediately when using left/right arrows

still a bit iffy, cant seem to get focus to move between the columns, def something with the menutrigger...

* making column resize actually get called if provided

* fixing bugs, testing various cases, adding description to useTableColumnResizeState

* Fix crash if onColumnResize is undef

* figure out break

* fix docs build break, add rest of code

* fix docs example render issues

* remove onColumnResize/Start/End from useTableColumnResizeState

these are unecessary since the state returns the resizing column already and we have onResize handlers in useTableColumnResize

* docs first draft

* remove local test story for now

* small cleanup

* updating resizer in docs

* fixing talkback and Safari aria resizer focus issues

* fixing checkbox rendering in iOS Safari

* editing/proofreading

* move useTableColumnResizeState to useTableState docs

* moving resizable table section and addressing smaller changes

* simplifying example

* collapsing some parts of the code and trying to display the code blocks as is

* addressing review comments

* add tailwind example

* forgot to add some additional comments

* override native focus ring for button

* update copy as per review and re-collapse code blocks

collapsed code blocks feel better on mobile so I changed it back to that

* uncollapsing code blocks

* Rough example of press header to start resizing

* Updating column resize to support mode where resizer is always visible

split out from #4061, see that PR for more details and alternative approaches

* update to match latest changes to api

* mimic docs example

* forgot to clean up some things

* pulling in code changes from docs PR

get rid of inline styles and fix case where there isnt a separate trigger for starting column resize

* Add separate mode for triggering resizing manually via Enter (#4061)

* adding separate mode for enabling resize

this make it so the user would have to hit Enter to start resizing. This allows us to have the resizers always be visible and still preserve grid navigation between columns.

* skip test for now so build works for testing

* wrap useMove so it doesnt trigger keyboard handlers when edit mode isnt enabled

* update tableview so that it enters edit mode when choosing to resize via menu

still one bug where manager.isFocused is set to false for somereason after confirming the resize

* fixing bugs

fixes bug where selection manager would stop tracking if the manager is focused or not if keyboard nav was disabled. This was a bug where the user wouldnt be able to move table focus after confirming a resize operation. Adds new prop to useTableColumnResize for triggering resizeStart if resizer if focused. This is needed for our table since we dont have a way to call resizeStart programmatically out side of useTableColumnResize and we dont want to call it on focus for the aria example since that should require a manual trigger of Enter to enter resize mode

* updating description and missing logic

* getting rid of shouldResizeOnFocus prop

* fix skipped test

turns out the simulated resize operation was blurring before pressEnd finished

* updated docs to move inline css into classnames

also updates copy and handles case where trigger ref isnt provided

* remove sorting story and cleanup

* starting resize on press for indicator

this unfortunately causes a difference in behavior between starting a drag on menu (no resizestart until move) and starting a drag via mouse/touch (resizestart immediately on press)

* using triggerRef existance to determine if behavior is resize on focus

one test is still breaking, debugging

* fixing test

test would blur on rerender causing a column width update even though resizing wasnt happening. Changed conditonal so calling endResize only causes value updates if we are resizing

* make resizer single line for focus

* nit reorganizing

* update docs example for resizer styling

* mimic docs example

remove selection from example to mirror docs

* adding description for keyboard users for Enter to start resizing

this is for the aria table example where resizing is entered manually via Enter while focused on the resizer

* fixing issue where tab wasnt exiting the table when focused on the reizer

* adding min width for columns to avoid weirdness with trying to collapse 0

* fix lint

* avoid weirdness with width 0 by setting a min width

* propagate all keydown events if we arent in resize mode and have always visible resizers

forgot that we also have other keyboard combos like cmd + a or escape that should also be handled by useSelectableCollection

* removing ref read in render

* add aria description to input for virtual modality too

if the user enters the table using control+option+arrow keys in voiceover, they will be virtual modality so we want the description for press enter to resize to be audible there

* addressing review comments

* fix some copy

* prevent extraneous scrolling when keyboard navigating to the resizer

margin applied on the visually hidden input makes scrollIntoView think it needs to scroll it

* fixing extraneous scrolling behavior

* address review comments

* fix logic

* removing margin style in favor of hook provided style

* fix typescript lint

* make resize handle larger

* fix resizer style on drag

* Simplify aria table resizing docs example (#4253)

* update example to match updated hooks

* remove VisuallyHidden from resizer in favor of styles from hook

* adding version badge

---------

Co-authored-by: Robert Snow <rsnow@adobe.com>
Co-authored-by: Devon Govett <devongovett@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants