Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

17047 migrate PressableWithSecondaryInteraction to PressableWithFeedback component #20251

Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -2460,6 +2460,13 @@ const CONST = {
EXPENSIFY_LOGO_SIZE_RATIO: 0.22,
EXPENSIFY_LOGO_MARGIN_RATIO: 0.03,
},
ACCESSIBILITY_ROLE: {
BUTTON: 'button',
LINK: 'link',
MENUITEM: 'menuitem',
TEXT: 'text',
RADIO: 'radio',
},
};

export default CONST;
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import * as DeviceCapabilities from '../../libs/DeviceCapabilities';
import styles from '../../styles/styles';
import withWindowDimensions, {windowDimensionsPropTypes} from '../withWindowDimensions';
import {propTypes as anchorForCommentsOnlyPropTypes, defaultProps as anchorForCommentsOnlyDefaultProps} from './anchorForCommentsOnlyPropTypes';
import CONST from '../../CONST';

const propTypes = {
/** Press in handler for the link */
Expand Down Expand Up @@ -64,6 +65,8 @@ function BaseAnchorForCommentsOnly(props) {
onPress={linkProps.onPress}
onPressIn={props.onPressIn}
onPressOut={props.onPressOut}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.LINK}
accessibilityLabel={props.href}
>
<Tooltip text={props.href}>
<Text
Expand Down
7 changes: 3 additions & 4 deletions src/components/LHNOptionsList/OptionRowLHN.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,10 @@ function OptionRowLHN(props) {
props.isFocused ? styles.sidebarLinkActive : null,
hovered && !props.isFocused ? props.hoverStyle : null,
]}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}
accessibilityLabel={props.translate('accessibilityHints.navigatesToChat')}
>
<View
accessibilityHint={props.translate('accessibilityHints.navigatesToChat')}
style={sidebarInnerRowStyle}
>
<View style={sidebarInnerRowStyle}>
<View style={[styles.flexRow, styles.alignItemsCenter]}>
{!_.isEmpty(optionItem.icons) &&
(optionItem.shouldShowSubscript ? (
Expand Down
2 changes: 2 additions & 0 deletions src/components/MenuItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ function MenuItem(props) {
]}
disabled={props.disabled}
ref={props.forwardedRef}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.MENUITEM}
accessibilityLabel={props.title}
Copy link
Contributor

@abdulrahuman5196 abdulrahuman5196 Sep 4, 2023

Choose a reason for hiding this comment

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

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.

>
{({hovered, pressed}) => (
<>
Expand Down
9 changes: 5 additions & 4 deletions src/components/PressableWithSecondaryInteraction/index.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import _ from 'underscore';
import React, {Component} from 'react';
import {Pressable} from 'react-native';
import * as pressableWithSecondaryInteractionPropTypes from './pressableWithSecondaryInteractionPropTypes';
import styles from '../../styles/styles';
import * as DeviceCapabilities from '../../libs/DeviceCapabilities';
import * as StyleUtils from '../../styles/StyleUtils';
import PressableWithFeedback from '../Pressable/PressableWithFeedback';

/**
* This is a special Pressable that calls onSecondaryInteraction when LongPressed, or right-clicked.
Expand Down Expand Up @@ -76,19 +76,20 @@ 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
style={StyleUtils.combineStyles(this.props.inline ? styles.dInline : this.props.style)}
wrapperStyle={StyleUtils.combineStyles(DeviceCapabilities.canUseTouchScreen() ? [styles.userSelectNone, styles.noSelect] : [])}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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()}

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, this is not a blocker for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

onPressIn={this.props.onPressIn}
onLongPress={this.props.onSecondaryInteraction ? this.executeSecondaryInteraction : undefined}
activeOpacity={this.props.activeOpacity}
pressDimmingValue={this.props.activeOpacity}
onPressOut={this.props.onPressOut}
onPress={this.props.onPress}
ref={(el) => (this.pressableRef = el)}
// eslint-disable-next-line react/jsx-props-no-spreading
{...defaultPressableProps}
>
{this.props.children}
</Pressable>
</PressableWithFeedback>
);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import _ from 'underscore';
import React, {forwardRef} from 'react';
import {Pressable} from 'react-native';
import * as pressableWithSecondaryInteractionPropTypes from './pressableWithSecondaryInteractionPropTypes';
import Text from '../Text';
import HapticFeedback from '../../libs/HapticFeedback';
import PressableWithFeedback from '../Pressable/PressableWithFeedback';

/**
* This is a special Pressable that calls onSecondaryInteraction when LongPressed.
Expand All @@ -13,22 +12,20 @@ import HapticFeedback from '../../libs/HapticFeedback';
*/
function PressableWithSecondaryInteraction(props) {
// Use Text node for inline mode to prevent content overflow.
const Node = props.inline ? Text : Pressable;
const Node = props.inline ? Text : PressableWithFeedback;
const executeSecondaryInteraction = (e) => {
e.preventDefault();
props.onSecondaryInteraction(e);
};

return (
<Node
ref={props.forwardedRef}
onPress={props.onPress}
onLongPress={(e) => {
if (!props.onSecondaryInteraction) {
return;
}
e.preventDefault();
HapticFeedback.longPress();
props.onSecondaryInteraction(e);
}}
onLongPress={props.onSecondaryInteraction ? executeSecondaryInteraction : undefined}
onPressIn={props.onPressIn}
onPressOut={props.onPressOut}
activeOpacity={props.activeOpacity}
pressDimmingValue={props.activeOpacity}
// eslint-disable-next-line react/jsx-props-no-spreading
{..._.omit(props, 'onLongPress')}
>
Expand Down
2 changes: 2 additions & 0 deletions src/components/Reactions/EmojiReactionBubble.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ function EmojiReactionBubble(props) {
enableLongPressWithHover={props.isSmallScreenWidth}
// Prevent text input blur when emoji reaction is clicked
onMouseDown={(e) => e.preventDefault()}
accessibilityRole="button"
robertKozik marked this conversation as resolved.
Show resolved Hide resolved
accessibilityLabel={props.emojiCodes.join('')}
>
<Text style={[styles.emojiReactionBubbleText, styles.userSelectNone, StyleUtils.getEmojiReactionBubbleTextStyle(props.isContextMenu)]}>{props.emojiCodes.join('')}</Text>
{props.count > 0 && <Text style={[styles.reactionCounterText, styles.userSelectNone, StyleUtils.getEmojiReactionCounterTextStyle(props.hasUserReacted)]}>{props.count}</Text>}
Expand Down
3 changes: 2 additions & 1 deletion src/pages/home/report/ReportActionItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -430,10 +430,11 @@ function ReportActionItem(props) {
onSecondaryInteraction={showPopover}
preventDefaultContextMenu={!props.draftMessage && !hasErrors}
withoutFocusOnSecondaryInteraction
accessibilityLabel={props.translate('accessibilityHints.chatMessage')}
>
<Hoverable disabled={Boolean(props.draftMessage)}>
{(hovered) => (
<View accessibilityLabel={props.translate('accessibilityHints.chatMessage')}>
<View>
{props.shouldDisplayNewMarker && <UnreadActionIndicator reportActionID={props.action.reportActionID} />}
<MiniReportActionContextMenu
reportID={props.report.reportID}
Expand Down