17047 migrate PressableWithSecondaryInteraction to PressableWithFeedback component #20251

robertKozik · 2023-06-06T10:40:57Z

Details

Fixed Issues

Tests

PressableWithSecondaryInteraction is used in three places: reports in LHN, options in RHN, and emoji bubbles and links/mails in Chats

LHN
- log in
- verify that styling of each individual report visible in LHN is unchanged
- longPress(touchable devices)/ rightClick (desktop) the report and verify that popover is opened correctly
- press/click the report and verify that user is directed to this report correctly
RHN
- log in
- click the profile avatar in LHN
- verify that each option visible on opened RHN has unchanged styling
- press/click and verify that menu item redirects correctly
ReportScreen
- log in
- navigate to chat with another user
- type and send any link
- verify that styling of the link remain unchanged
- longPress(touchable devices)/ rightClick (desktop) the link and verify that popover is opened correctly
- add reaction to message
- longPress(touchable devices)/ rightClick (desktop) reacted emoji and verify that popover is opened correctly

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

Screenshots/Videos

Web

Desktop.-.web.mov

Mobile Web - Chrome

android.-.web.mov

Mobile Web - Safari

iOS.-.web.mov

Desktop

Desktop.-.native.mov

iOS

iOS.-.native.mov

Android

android.-.native.mov

melvin-bot · 2023-06-06T21:33:42Z

@luacmartins @0xmiroslav One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

0xmiros

@robertKozik please link correct issue. It's not 14047 but #17047

src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js

src/components/LHNOptionsList/OptionRowLHN.js

src/components/MenuItem.js

robertKozik · 2023-06-07T09:21:28Z

Created the CONST values for accessibility roles 🚀

luacmartins

LGTM! Thanks for adding the const.

luacmartins · 2023-06-07T15:43:09Z

@0xmiroslav all yours

luacmartins · 2023-06-08T17:55:33Z

@robertKozik we have conflicts. Could you please resolve those?

0xmiros · 2023-06-09T16:03:10Z

BUG?: opacity change on hover

Before:

Screen.Recording.2023-06-09.at.6.01.46.PM.mov

After:

Screen.Recording.2023-06-09.at.6.00.44.PM.mov

…eraction

robertKozik · 2023-06-13T18:33:48Z

@0xmiroslav @luacmartins I had difficulties with reproduction the problem on mSafari, but it turned out that it is more visible on the mChrome. When user tried to longPress over the text, selection of it has priority over our defined handler. I came up with the following solution:
When we are on touch device, selection of the text inside PressableWithSecondaryInteraction is disabled by styles.userSelectNone and styles.noSelect.

The problem from the prod is still visible tho, it only occurs on the mSafari. With above styles applied, it's less intrusive, but still visible.

0xmiros · 2023-06-13T18:36:56Z

When we are on touch device, selection of the text inside PressableWithSecondaryInteraction is disabled by styles.userSelectNone and styles.noSelect.

This will prevent text selection on all buttons throughout the app. I am not sure if this causes any unknown problem.

robertKozik · 2023-06-13T18:44:14Z

PressableWithSecondaryInteraction is only used in three places, which are listed here. If my assumptions of the nature of the problem are correct (text selection), it can occur on every instance of it when it wraps text.

luacmartins · 2023-06-14T15:25:07Z

@robertKozik we have conflicts.

src/components/Reactions/EmojiReactionBubble.js

robertKozik · 2023-06-19T10:53:20Z

@0xmiroslav bumping, as I said in my comment before, the no selection policy is only referring to the PressableWithSecondaryInteraction (3 places in the app). On the other hand, if you prefer, that prohibition of text selection can only be added inside the LHN instance of the PressableWithSecondaryInteraction.

luacmartins · 2023-06-21T20:54:29Z

@robertKozik we have conflicts. Could you please resolve? I'll do a final review once that's done.

…ableWithSecondaryInteraction

robertKozik · 2023-06-21T22:17:17Z

@luacmartins merged with main

0xmiros · 2023-06-22T05:26:33Z

src/components/PressableWithSecondaryInteraction/index.js

@@ -76,10 +76,11 @@ class PressableWithSecondaryInteraction extends Component {

        // On Web, Text does not support LongPress events thus manage inline mode with styling instead of using Text.
        return (
-            <Pressable
+            <PressableWithFeedback
+                wrapperStyle={StyleUtils.combineStyles(DeviceCapabilities.canUseTouchScreen() ? [styles.userSelectNone, styles.noSelect] : [])}


This should be consistent with others. They're using ControlSelection to prevent selection on long-click.

App/src/components/MenuItem.js

Lines 113 to 114 in e6c4b4e

onPressIn={() => props.shouldBlockSelection && props.isSmallScreenWidth && DeviceCapabilities.canUseTouchScreen() && ControlSelection.block()}

onPressOut={ControlSelection.unblock}

App/src/components/AnchorForAttachmentsOnly/index.js

Lines 12 to 13 in e6c4b4e

onPressIn={() => DeviceCapabilities.canUseTouchScreen() && ControlSelection.block()}

onPressOut={() => ControlSelection.unblock()}

App/src/pages/home/report/ReportActionItem.js

Lines 450 to 451 in e6c4b4e

onPressIn={() => props.isSmallScreenWidth && DeviceCapabilities.canUseTouchScreen() && ControlSelection.block()}

onPressOut={() => ControlSelection.unblock()}

I think we can generalize this. How about introducing shouldBlockSelection prop in PressableWithSecondaryInteraction component? And all above code can be removed and simply set shouldBlockSelection to true.

Anyway, this is not a blocker for this PR.

0xmiros · 2023-06-22T06:09:17Z

As a follow-up, I suggest to

generalize solution to prevent selection on mWeb (17047 migrate PressableWithSecondaryInteraction to PressableWithFeedback component #20251 (comment))
use CONST for all static values of accessibilityRole (17047 migrate PressableWithSecondaryInteraction to PressableWithFeedback component #20251 (comment))

0xmiros · 2023-06-22T06:12:45Z

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

Web

web.mov

msafari2.mov

Mobile Web - Chrome

mchrome.mov

Mobile Web - Safari

msafari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

robertKozik · 2023-06-22T13:13:38Z

As a follow-up, I suggest to

generalize solution to prevent selection on mWeb (17047 migrate PressableWithSecondaryInteraction to PressableWithFeedback component #20251 (comment))

use CONST for all static values of accessibilityRole (17047 migrate PressableWithSecondaryInteraction to PressableWithFeedback component #20251 (comment))

The issue for the const values is already created (LINK)

OSBotify · 2023-06-22T15:29:57Z

✋ 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-06-22T17:20:50Z

🚀 Deployed to staging by https://github.com/luacmartins in version: 1.3.31-0 🚀

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

OSBotify · 2023-06-23T21:05:31Z

🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.31-3 🚀

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

OSBotify · 2023-06-23T21:25:16Z

🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.31-3 🚀

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

aimane-chnaif · 2023-08-13T03:21:19Z

src/components/PressableWithSecondaryInteraction/index.js

@@ -76,10 +76,11 @@ class PressableWithSecondaryInteraction extends Component {

        // On Web, Text does not support LongPress events thus manage inline mode with styling instead of using Text.
        return (
-            <Pressable
+            <PressableWithFeedback
+                wrapperStyle={StyleUtils.combineStyles(DeviceCapabilities.canUseTouchScreen() ? [styles.userSelectNone, styles.noSelect] : [])}


Coming from [HOLD for payment 2023-08-07] [$1000] Chat - Word that follows after the multiline hyperlink is located between multiline hyperlink #22362:

If inline is true, this should be passed to wrapperStyle as well, not only to style.

More details about the root cause: #22362 (comment)

abdulrahuman5196 · 2023-09-04T10:17:46Z

src/components/MenuItem.js

@@ -122,6 +122,8 @@ function MenuItem(props) {
            ]}
            disabled={props.disabled}
            ref={props.forwardedRef}
+            accessibilityRole={CONST.ACCESSIBILITY_ROLE.MENUITEM}
+            accessibilityLabel={props.title}


props.title was of number 0 in some cases which caused app crash as mentioned here #25451 (comment).
We should make sure the values we pass are string.

tienifr and others added 2 commits June 6, 2023 12:57

fix 19360 app crashes on clicking on replying thread after deleting ws

33bfa83

migrate PressableWithSecondaryInteraction to PressableWithFeedback

ef0dc5a

robertKozik force-pushed the 17047-migrate-PressableWithSecondaryInteraction branch from c810db2 to c558bcd Compare June 6, 2023 10:59

robertKozik mentioned this pull request Jun 6, 2023

[Group 5][Accessible Pressable] Migrate MenuItem #17053

Closed

add missing accessibilityLabels

b901d25

robertKozik force-pushed the 17047-migrate-PressableWithSecondaryInteraction branch from c558bcd to b901d25 Compare June 6, 2023 11:33

Skalakid mentioned this pull request Jun 6, 2023

17014 - migrate OptionRowLHN to PressableWithFeedback #19660

Closed

57 tasks

prettier

014eb8b

robertKozik marked this pull request as ready for review June 6, 2023 21:33

robertKozik requested a review from a team as a code owner June 6, 2023 21:33

melvin-bot bot requested review from 0xmiros and luacmartins and removed request for a team June 6, 2023 21:33

0xmiros reviewed Jun 6, 2023

View reviewed changes

luacmartins previously approved these changes Jun 6, 2023

View reviewed changes

src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js Outdated Show resolved Hide resolved

src/components/LHNOptionsList/OptionRowLHN.js Outdated Show resolved Hide resolved

src/components/MenuItem.js Outdated Show resolved Hide resolved

robertKozik dismissed luacmartins’s stale review via 7803209 June 7, 2023 09:20

aimane-chnaif mentioned this pull request Jun 7, 2023

17033 - migrated HeaderView to PressableWithoutFeedback #19958

Merged

57 tasks

Skalakid mentioned this pull request Jun 7, 2023

17049 - migrate RadioButton to PressableWithFeedback #20250

Merged

57 tasks

create const values for accessibility roles

5cd97a3

robertKozik force-pushed the 17047-migrate-PressableWithSecondaryInteraction branch from 7803209 to 5cd97a3 Compare June 7, 2023 14:18

Merge branch 'main' into 17047-migrate-PressableWithSecondaryInteraction

07e40d1

luacmartins previously approved these changes Jun 7, 2023

View reviewed changes

luacmartins mentioned this pull request Jun 7, 2023

17038 - migrate AddReactionBubble to PressableWithFeedback #20158

Merged

57 tasks

Merge branch 'main' into 17047-migrate-PressableWithSecondaryInteraction

a453a84

robertKozik dismissed luacmartins’s stale review via a453a84 June 8, 2023 17:59

disable text selection for touch devices in PressableWithSecondaryInt…

42fb09d

…eraction

conorpendergrast mentioned this pull request Jun 14, 2023

[HOLD for payment 2023-06-30] [$250] [Group 5][Accessible Pressable] Migrate PressableWithSecondaryInteraction #17047

Closed

Merge branch 'main' into 17047-migrate-PressableWithSecondaryInteraction

b9d38f5

luacmartins requested changes Jun 15, 2023

View reviewed changes

src/components/Reactions/EmojiReactionBubble.js Outdated Show resolved Hide resolved

define EmojiReactionBubble accessibilityRole by CONST value

235b5fc

Merge remote-tracking branch 'upstream/main' into 17047-migrate-Press…

4572235

…ableWithSecondaryInteraction

robertKozik requested a review from luacmartins June 21, 2023 22:14

0xmiros reviewed Jun 22, 2023

View reviewed changes

0xmiros approved these changes Jun 22, 2023

View reviewed changes

luacmartins approved these changes Jun 22, 2023

View reviewed changes

luacmartins merged commit a777544 into Expensify:main Jun 22, 2023

OSBotify mentioned this pull request Jun 22, 2023

Deploy Checklist: New Expensify 2023-06-22 #21330

Closed

39 tasks

melvin-bot bot mentioned this pull request Jun 23, 2023

[HOLD for payment 2023-06-30] [HOLD for payment 2023-02-17] Test Preferences section disappear when the android app is reloaded in offline mode #14047

Closed

1 task

aimane-chnaif mentioned this pull request Jul 18, 2023

[HOLD for payment 2023-08-07] [$1000] Chat - Word that follows after the multiline hyperlink is located between multiline hyperlink #22362

Closed

6 tasks

aimane-chnaif reviewed Aug 13, 2023

View reviewed changes

abdulrahuman5196 reviewed Sep 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

17047 migrate PressableWithSecondaryInteraction to PressableWithFeedback component #20251

17047 migrate PressableWithSecondaryInteraction to PressableWithFeedback component #20251

robertKozik commented Jun 6, 2023 •

edited

Loading

melvin-bot bot commented Jun 6, 2023

0xmiros left a comment

robertKozik commented Jun 7, 2023

luacmartins left a comment

luacmartins commented Jun 7, 2023

luacmartins commented Jun 8, 2023

0xmiros commented Jun 9, 2023

robertKozik commented Jun 13, 2023

0xmiros commented Jun 13, 2023

robertKozik commented Jun 13, 2023

luacmartins commented Jun 14, 2023

robertKozik commented Jun 19, 2023

luacmartins commented Jun 21, 2023

robertKozik commented Jun 21, 2023

0xmiros Jun 22, 2023

0xmiros Jun 22, 2023

0xmiros Jun 22, 2023

0xmiros commented Jun 22, 2023

0xmiros commented Jun 22, 2023

robertKozik commented Jun 22, 2023

OSBotify commented Jun 22, 2023

OSBotify commented Jun 22, 2023

OSBotify commented Jun 23, 2023

OSBotify commented Jun 23, 2023

aimane-chnaif Aug 13, 2023

abdulrahuman5196 Sep 4, 2023 •

edited

Loading

	onPressIn={() => props.shouldBlockSelection && props.isSmallScreenWidth && DeviceCapabilities.canUseTouchScreen() && ControlSelection.block()}
	onPressOut={ControlSelection.unblock}

	onPressIn={() => DeviceCapabilities.canUseTouchScreen() && ControlSelection.block()}
	onPressOut={() => ControlSelection.unblock()}

	onPressIn={() => props.isSmallScreenWidth && DeviceCapabilities.canUseTouchScreen() && ControlSelection.block()}
	onPressOut={() => ControlSelection.unblock()}

17047 migrate PressableWithSecondaryInteraction to PressableWithFeedback component #20251

17047 migrate PressableWithSecondaryInteraction to PressableWithFeedback component #20251

Conversation

robertKozik commented Jun 6, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jun 6, 2023

0xmiros left a comment

Choose a reason for hiding this comment

robertKozik commented Jun 7, 2023

luacmartins left a comment

Choose a reason for hiding this comment

luacmartins commented Jun 7, 2023

luacmartins commented Jun 8, 2023

0xmiros commented Jun 9, 2023

robertKozik commented Jun 13, 2023

0xmiros commented Jun 13, 2023

robertKozik commented Jun 13, 2023

luacmartins commented Jun 14, 2023

robertKozik commented Jun 19, 2023

luacmartins commented Jun 21, 2023

robertKozik commented Jun 21, 2023

0xmiros Jun 22, 2023

Choose a reason for hiding this comment

0xmiros Jun 22, 2023

Choose a reason for hiding this comment

0xmiros Jun 22, 2023

Choose a reason for hiding this comment

0xmiros commented Jun 22, 2023

0xmiros commented Jun 22, 2023

Reviewer Checklist

Screenshots/Videos

robertKozik commented Jun 22, 2023

OSBotify commented Jun 22, 2023

OSBotify commented Jun 22, 2023

OSBotify commented Jun 23, 2023

OSBotify commented Jun 23, 2023

aimane-chnaif Aug 13, 2023

Choose a reason for hiding this comment

abdulrahuman5196 Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

robertKozik commented Jun 6, 2023 •

edited

Loading

abdulrahuman5196 Sep 4, 2023 •

edited

Loading