Skip to content

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented Jul 11, 2022

Closes #3028

Splits the MenuTrigger from the Header. This makes for some better more reasonable styling and should improve screen reader announcements as well.

I've changed the role=separator to an input type=range, just changing to slider wasn't enough, TalkBack wouldn't fire onChange events with swipe gestures. This brings it more in line with SliderThumb. (See https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/slider/src/SliderThumb.tsx#L46 for rendering and https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/slider/src/useSliderThumb.ts#L179 for aria similarities)

As a result, I've been able to simplify the aria-* attributes on the input, some styling needed changes as well to compensate for the new structure.

I've included a new prop to useTableColumnHeader, optional so backwards compatible, non-breaking hasMenu which will help us in the future when we allow people to pass in their own menu items. It also removes knowledge of resizing from useTableColumnHeader.

I've hooked up all the id's for better screen reader announcements, the resizer will now be labelled by the column header. This helps with the entire table announcing again as well.

I've limited the max value of the width from Infinity to the max safe integer, that's all we can really support anyways, and a max value of Infinity causes TalkBack to stop firing onChange.

Open issues:
There is an issue where VO reads off a large negative number when focusing the resizer that I haven't been able to solve yet.
FF still doesn't display focus ring for mouse users

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

@snowystinger snowystinger marked this pull request as ready for review July 11, 2022 22:29
@adobe-bot
Copy link

@snowystinger snowystinger mentioned this pull request Jul 11, 2022
61 tasks
@snowystinger snowystinger changed the title Table Column Resize via screen readers [WIP] - Table Column Resize via screen readers Jul 14, 2022
@adobe-bot
Copy link

LFDanLu and others added 5 commits July 15, 2022 11:47
changed resizer to input w/ onChange since Android Talkback doesnt fire keyboad events when swiping up/down. aria-valuemax Infinitiy breaks android talkback slider interactions
@adobe-bot
Copy link

@adobe-bot
Copy link

@adobe-bot
Copy link

@snowystinger snowystinger changed the title [WIP] - Table Column Resize via screen readers Table Column Resize via screen readers Jul 15, 2022
@adobe-bot
Copy link

# Conflicts:
#	packages/@react-aria/table/src/useTableColumnHeader.ts
#	packages/@react-aria/table/src/useTableColumnResize.ts
#	packages/@react-spectrum/table/src/Resizer.tsx
#	packages/@react-spectrum/table/src/TableView.tsx
# Conflicts:
#	packages/@react-aria/table/src/useTableColumnResize.ts
@adobe-bot
Copy link

@adobe-bot
Copy link

# Conflicts:
#	packages/@react-aria/table/src/useTableColumnResize.ts
#	packages/@react-spectrum/table/package.json
#	packages/@react-spectrum/table/src/TableView.tsx
@adobe-bot
Copy link

Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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

I tested a few tables with OSX Chrome VO and this worked nicely.

@adobe-bot
Copy link

ktabors
ktabors previously approved these changes Aug 4, 2022
reidbarber
reidbarber previously approved these changes Aug 9, 2022
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

LGTM on VO. I did see the large number read off you mentioned

@adobe-bot
Copy link

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

Some things I noted when testing:

  • The resize handles don’t appear after tapping the “Resize column” menu option in Android
    • Only happen in the storybook iframe, works fine if it is outside the iframe
  • Pretty hard to drag the handles in mobile but that will be handled in followups
  • Unable to confirm a resize operation when double tapping in VoiceOver sometimes (happened with “File Name” column in the “some columns not allowed resizing story”). Other times double tapping will cause a shift in position before confirming the resize operation

Comment on lines 155 to 159
onMouseDown: (e: React.MouseEvent<HTMLElement>) => {
if (e.button !== 0 || e.altKey || e.ctrlKey || e.metaKey) {
return;
}
onDown();
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing these are used instead of usePress because of the extra conditional early returns? Seems to be a pretty common pattern that is cropping up, might be something we could add to usePress as an option?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is copied from the slider code
which is track listener code, so this may not be necessary? https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/slider/src/useSlider.ts#L198
I'll double check

Copy link
Member Author

@snowystinger snowystinger Aug 10, 2022

Choose a reason for hiding this comment

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

sorry i linked the wrong one, ignore my previous comment, it's from slider thumb
https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/slider/src/useSliderThumb.ts#L212 which makes more sense
I've switched to usePress and it appears to work just fine. It may just be old code that needed altKey etc before that existed in the PressEvent and keyboard was already covered elsewhere and usePress would double that coverage

Comment on lines 584 to 585
// focusSafely won't actually focus because the focus moves from the menuitem to the body during the after transition wait
resizingRef.current.focus();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe linked to the weird The resize handles don’t appear after tapping the “Resize column” menu option in Android in storybook iframe behavior I noted. I can try poking around

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the issue goes away if I wrap this focus in a setTimeout,

Copy link
Member Author

Choose a reason for hiding this comment

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

hahaha, but in a RAF it was failing for me

@snowystinger snowystinger dismissed stale reviews from reidbarber and ktabors via 4825f50 August 10, 2022 22:17
@adobe-bot
Copy link

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

I can't seem to confirm a resize operation via Voiceover now, not entirely sure how the changes broke that hrm

@adobe-bot
Copy link

Comment on lines 154 to 156
if (e.pointerType === 'virtual' && columnState.currentlyResizingColumn != null) {
stateRef.current.onColumnResizeEnd(item);
}
Copy link
Member

Choose a reason for hiding this comment

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

Confirmed that double tapping to trigger a press on the visually hidden input via VO and Talkback now "confirm"/exit the resize operation, but there seems to be an issue in Android Chrome now where the focus is lost to the body upon triggering the press. VO is a bit better but it suffers from an issue where the press happens on the element under the visually hidden input which could be an entirely different column depending on how much resizing happened.

What if you have move focus to the column header in this case instead since the resize handles go away? I'm fine with handling this in a followup or after we confirm the overall behavior with accessibility if you'd like

Copy link
Member Author

Choose a reason for hiding this comment

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

Focus should be moving to the header on column resize end, so I'm not sure what you're seeing. Maybe we can hop on a call tomorrow and discuss it more.

@adobe-bot
Copy link

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

Tested w/ Rob, Android Chrome and VoiceOver issues have been resolved

@snowystinger snowystinger merged commit f4c916e into main Aug 12, 2022
@snowystinger snowystinger deleted the table-col-resize-voiceover branch August 12, 2022 16:08
columnHeaderProps: {
...mergeProps(gridCellProps, pressProps, focusableProps, descriptionProps),
...mergeProps(gridCellProps, pressProps, focusableProps, descriptionProps, {
onPointerDown: (e) => console.log(e.target.outerHTML)
Copy link
Member

Choose a reason for hiding this comment

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

Assume this was not meant to go in.

isVirtualized?: boolean
isVirtualized?: boolean,
/** Whether the column has a menu in the header, this changes interactions with the header. */
hasMenu?: boolean
Copy link
Member

Choose a reason for hiding this comment

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

Should this be less specific to menus? Should it be more like an override to disable the default sorting behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, what are the use cases other than a menu where we'd want to disable the default sorting behavior?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, but I mainly wondered if menus were too spectrum-specific?

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe, it was the ask for a future feature, custom menu options on columns. so that's where the name came from. I'm fine changing it though to disableDefaultSort or disableDefaultInteraction?

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.

Column resizing followup - task: Support for screen readers
6 participants