[Accessible Pressable] Create GenericPressable, PressableWithFeedback, PressableWithoutFeedback #17404

robertKozik · 2023-04-13T18:07:06Z

Details

Implementation of GenericPressable, PressableWithFeedback, PressableWithoutFeedback components

Fixed Issues

$ #16988
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

Verify that no errors appear in the JS console

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

src/components/OpacityView.js

roryabraham · 2023-04-18T19:56:42Z

src/libs/Accessibility/moveAccessibilityFocus/index.native.js

+    if (!ref) {
+        return;
+    }
+    AccessibilityInfo.sendAccssibilityEvent(ref, 'focus');


AFAICT this API isn't documented. Why do we need to use this instead of ref.current.focus on native?

When screen reader is active ref.current.focus on native won't shift screen reader (f.e. VoiceOver on iOS) focus.
You're right I can't find it too, but as I was doing the design doc I've only found two approaches:

AccessibilityInfo.sendAccessibilityEvent: currently implemented here, but not documented

AccessibilityInfo.setAccessibilityFocus: documented but needs reactTag of the next focused component. Obtaining the reactTag is connected with using findNodeHandle – which is going to be (or is) deprecated, you sent me a video from conference which mentions it while I was writing the design doc.

I'll do more research about it.

Maybe we can create an issue in the react-native repo to ask why AccessibilityInfo.sendAccessibilityEvent is not documented and what the alternative for AccessibilityInfo.setAccessibilityFocus is in a post-findNodeHandle world

facebook/react-native#37015

Ok, given the response in that issue, let's not block on this.

src/libs/Accessibility/index.js

src/components/Pressable/GenericPressable/PropTypes.js

src/components/Pressable/GenericPressable/GenericPressable.js

roryabraham · 2023-04-18T20:28:27Z

src/components/Pressable/GenericPressable/GenericPressable.js

+
+    return (
+        <Pressable
+            hitSlop={shouldUseAutoHitSlop && hitslop}


Do you know if there's any time we would use shuoldUseAutoHitSlop={false}? I'm wondering if we can just simplify and always use autoHitSlop

F.e. link inside the text block. Applying hitslop could break the layout consistency in it.

Ok ... my understanding with hitSlop is that it would never affect layout and never extend beyond the bounds of the parent. I'll play with this some more and see if I can break the layout of a text block with hitSlop

there is another point, maybe more meaningful. The minimal size of the accessible component is stated inside https://www.w3.org/WAI/WCAG21/Understanding/target-size.html says that it can be omited for the inline

also two inline links which are close to each other would have overlapping touchableAreas after applying hitslops

src/components/Pressable/GenericPressable/GenericPressable.js

src/components/Pressable/GenericPressable/index.js

…andlers in useCallback

…pacity values

…ble component prohibited cursor change

…pForSize func

amyevans

Sorry, few more quick comments 😅

src/components/Pressable/GenericPressable/BaseGenericPressable.js

src/components/Pressable/GenericPressable/PropTypes.js

src/components/Pressable/PressableWithoutFeedback.js

Co-authored-by: Amy Evans <amy@expensify.com>

roryabraham · 2023-04-27T19:02:44Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

No screenshots for now, but we'll do more manual testing as we implement these components throughout the app.

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

roryabraham · 2023-04-27T19:05:24Z

Looks like @dangrous is partially OOO the next couple days, and his comments have been addressed here, so as this is blocking a few other things I'm going to go ahead and merge this so we can keep this project moving forward.

@dangrous feel free to leave any remaining comments and I'm sure @robertKozik would be happy to address them in a follow-up if needed.

see above comment

OSBotify · 2023-04-27T19:12:09Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-04-28T08:52:42Z

🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.8-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

aimane-chnaif · 2023-04-28T20:54:27Z

This PR caused another regression of #17073. It was fixed in #17908 but happening again after this PR.
@robertKozik can you please raise follow-up PR to fix it? And feel free to tag me as a reviewer.

OSBotify · 2023-05-02T19:50:55Z

🚀 Deployed to production by https://github.com/marcaaron in version: 1.3.8-8 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

puneetlath · 2023-05-03T14:02:03Z

Just some feedback for the future: let's make sure we have test/QA steps in the PR. Even if it's just a list of pages to go and test the buttons to make sure they are working as expected.

Santhosh-Sellavel · 2023-05-25T20:51:26Z

src/components/Pressable/PressableWithFeedback.js

+const PressableWithFeedbackPropTypes = {
+    ..._.omit(GenericPressablePropTypes.pressablePropTypes, omittedProps),
+    pressDimmingValue: propTypes.number,
+    hoverDimmingValue: propTypes.number,
+};


Why there are no comments for propTypes?

robertKozik force-pushed the generic-pressable branch from 6e88681 to 892dee8 Compare April 17, 2023 19:00

robertKozik mentioned this pull request Apr 17, 2023

[HOLD for payment 2023-05-09] [Accessible Pressable] Create GenericPressable, PressableWithFeedback, PressableWithoutFeedback; #16988

Closed

roryabraham reviewed Apr 18, 2023

View reviewed changes

This was referenced Apr 19, 2023

[Tracking] Migrate away from Touchables and create a standardized, accessible button library using Pressables #14589

Closed

[$1000] Android chrome does not provide response to click on invite and remove buttons on manage members page #17471

Closed

robertKozik added 25 commits April 24, 2023 21:15

create move focus functions for web and native

8c60699

create useScreenReaderStatus hook

8c823da

create base pressable files structure

737f6dc

create base GenericPressable component

ecec5eb

add hitSlop calculation for pressable component

d3b843e

calculate disabled state before GenericPressable return, wrap press h…

d8eec98

…andlers in useCallback

fix wrong import of propTypes to GenericPressable

dba339f

update OpacityView to use reanimated and enable dimming to multiple o…

47035c3

…pacity values

fix GenericPressable implementation – passing disabled prop to Pressa…

46241f1

…ble component prohibited cursor change

correctly set up hitslop for useAutoHitSlop

d270158

set up PressableWithFeedback component

ee2e3b0

set up PressableWithoutFeedback component

fd21d4e

comment GenericPressable propTypes

3dbea09

fix unsubscribing screen reader listener

784889b

update GenericPresssable implementation

2830923

undim opacityView with timing

038960a

update comments on OpacityView propTypes

0d81a56

simplify useScreenReaderStatus useEffect hook

7b1fe9a

extract possible screen reader states to const file

d5204e9

fix typo

0491515

simplify switch statment to two if's

83718b7

hoist pareStyleFromFunction to StyleUtils file

6cc7fd7

add JSDoc for getCursorStyle function

19ebe88

rename GenericPressable to BaseGenericPressable

bd9bbbb

split accessibility props to native and web implementations

92c826e

address the arbitrary minimum value of tappable area inside getHitSlo…

8ad2923

…pForSize func

robertKozik dismissed roryabraham’s stale review via 8ad2923 April 27, 2023 10:07

robertKozik added 3 commits April 27, 2023 12:08

Merge branch 'main' into generic-pressable

6ba7f1b

fix typo

7bfc422

fix wrong prop name inside default props

2006a89

robertKozik requested a review from amyevans April 27, 2023 10:56

use platform specific cursor utility in BaseGenericPressable

27ba2fa

amyevans requested changes Apr 27, 2023

View reviewed changes

src/components/Pressable/GenericPressable/BaseGenericPressable.js Outdated Show resolved Hide resolved

src/components/Pressable/GenericPressable/PropTypes.js Outdated Show resolved Hide resolved

src/components/Pressable/PressableWithoutFeedback.js Show resolved Hide resolved

robertKozik and others added 3 commits April 27, 2023 18:20

Change order of imports

0e85364

Co-authored-by: Amy Evans <amy@expensify.com>

Remove unintended whitespace

72af27b

Co-authored-by: Amy Evans <amy@expensify.com>

update Pressable proppType example

66782e5

amyevans approved these changes Apr 27, 2023

View reviewed changes

roryabraham approved these changes Apr 27, 2023

View reviewed changes

roryabraham merged commit 1b4e892 into Expensify:main Apr 27, 2023

roryabraham mentioned this pull request Apr 27, 2023

[Accessible Pressable] fix checking current field on undefined ref in BaseGenericPressable #18106

Merged

57 tasks

OSBotify mentioned this pull request Apr 28, 2023

Deploy Checklist: New Expensify 2023-04-28 #18132

Closed

30 tasks

aimane-chnaif mentioned this pull request Apr 28, 2023

Fix applying styling when it want to apply opacity on OpacityView #18153

Merged

57 tasks

priyeshshah11 mentioned this pull request May 1, 2023

Disables the Button during onPress call in PressableWithFeedback #18122

Merged

57 tasks

Santhosh-Sellavel reviewed May 25, 2023

View reviewed changes

consuelo-sanna mentioned this pull request Sep 13, 2023

Correct way to move accessibility focus facebook/react-native#37015

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Accessible Pressable] Create GenericPressable, PressableWithFeedback, PressableWithoutFeedback #17404

[Accessible Pressable] Create GenericPressable, PressableWithFeedback, PressableWithoutFeedback #17404

robertKozik commented Apr 13, 2023 •

edited

Loading

roryabraham Apr 18, 2023

robertKozik Apr 19, 2023

roryabraham Apr 19, 2023

robertKozik Apr 24, 2023

roryabraham Apr 24, 2023

roryabraham Apr 18, 2023

robertKozik Apr 19, 2023

roryabraham Apr 19, 2023

robertKozik Apr 19, 2023

robertKozik Apr 19, 2023

amyevans left a comment

roryabraham commented Apr 27, 2023

roryabraham commented Apr 27, 2023

OSBotify commented Apr 27, 2023

OSBotify commented Apr 28, 2023

aimane-chnaif commented Apr 28, 2023

OSBotify commented May 2, 2023

puneetlath commented May 3, 2023

Santhosh-Sellavel May 25, 2023

[Accessible Pressable] Create GenericPressable, PressableWithFeedback, PressableWithoutFeedback #17404

[Accessible Pressable] Create GenericPressable, PressableWithFeedback, PressableWithoutFeedback #17404

Conversation

robertKozik commented Apr 13, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amyevans left a comment

Choose a reason for hiding this comment

roryabraham commented Apr 27, 2023

Reviewer Checklist

Screenshots/Videos

roryabraham commented Apr 27, 2023

OSBotify commented Apr 27, 2023

OSBotify commented Apr 28, 2023

aimane-chnaif commented Apr 28, 2023

OSBotify commented May 2, 2023

puneetlath commented May 3, 2023

Choose a reason for hiding this comment

robertKozik commented Apr 13, 2023 •

edited

Loading