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.

Sign up for GitHub

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

Jump to bottom

Disables the Button during onPress call in PressableWithFeedback #18122

Merged
merged 15 commits into from
May 12, 2023
Merged

Disables the Button during onPress call in PressableWithFeedback #18122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4471e8b
added disabling of Button during onPress call in PressableWithFeedback
priyeshshah11 Apr 28, 2023
90d9b72
Merge branch 'main' of https://github.com/priyeshshah11/App into priy…
priyeshshah11 Apr 28, 2023
0c045be
code cleanup
priyeshshah11 Apr 28, 2023
fcee719
Merge branch 'main' of https://github.com/priyeshshah11/App into priy…
priyeshshah11 Apr 30, 2023
76500fc
code cleanup
priyeshshah11 Apr 30, 2023
cb06109
fixed hover & pressed styles
priyeshshah11 May 1, 2023
11a0929
replace View with frgament
priyeshshah11 May 1, 2023
ee9e4d2
changed the pressDimValue back to 0.5 as advised by the design team
priyeshshah11 May 1, 2023
ec2b5c2
passing the click events down from BaseGenericPressable to fix a bug
priyeshshah11 May 1, 2023
e1640db
restricts press events when the button is disabled
priyeshshah11 May 2, 2023
2d14e33
removed passing down the disabled prop in BaseGenericPressable & fixe…
priyeshshah11 May 2, 2023
1bca5e6
cleanup related to hover & pressed styles
priyeshshah11 May 2, 2023
690cf59
fixes unread indicator test
priyeshshah11 May 3, 2023
738cf17
Update src/components/Button/index.js
priyeshshah11 May 3, 2023
bba4458
Merge branch 'main' of https://github.com/priyeshshah11/App into priy…
priyeshshah11 May 10, 2023
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
71 changes: 35 additions & 36 deletions src/components/Button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import React, {Component} from 'react';
import {Pressable, ActivityIndicator, View} from 'react-native';
import {ActivityIndicator, View} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../../styles/styles';
import themeColors from '../../styles/themes/default';
import OpacityView from '../OpacityView';
import Text from '../Text';
import KeyboardShortcut from '../../libs/KeyboardShortcut';
import Icon from '../Icon';
Expand All @@ -15,6 +14,7 @@ import compose from '../../libs/compose';
import * as Expensicons from '../Icon/Expensicons';
import withNavigationFocus from '../withNavigationFocus';
import validateSubmitShortcut from './validateSubmitShortcut';
import PressableWithFeedback from '../Pressable/PressableWithFeedback';

const propTypes = {
/** The text for the button label */
Expand Down Expand Up @@ -106,6 +106,9 @@ const propTypes = {

/** Id to use for this button */
nativeID: PropTypes.string,

/** Accessibility label for the component */
accessibilityLabel: PropTypes.string,
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 required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does that mean all Buttons will need an accessibility label? & should adding all of them be in scope of 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.

Well since accessibilityLabel is required on GenericPressable it would make sense to have this required on Button too. I think it would be better to split this to two PRs:

  1. Migrate from Pressable to PressableWithFeedback for Button
  2. Disable PressableWithFeedback on press

cc @roryabraham thoughts on this?

Copy link
Contributor Author

@priyeshshah11 priyeshshah11 Apr 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Or given that I have already done the majority of the work for both the things above, I would prefer to move updating all usages of Button to add accessibilityLabels to a seperate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that accessibilityLabel should be required. I also agree that we should start by disabling PressableWithFeedback on press in this PR, then create a separate PR to migrate from Pressable to PressableWithFeedback for Button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@roryabraham I have already done the migration part in this PR as we didn't get a reply for ~2 weeks, so what's the plan of action here?

};

const defaultProps = {
Expand Down Expand Up @@ -137,6 +140,7 @@ const defaultProps = {
shouldRemoveLeftBorderRadius: false,
shouldEnableHapticFeedback: false,
nativeID: '',
accessibilityLabel: '',
};

class Button extends Component {
Expand Down Expand Up @@ -235,7 +239,7 @@ class Button extends Component {

render() {
return (
<Pressable
<PressableWithFeedback
onPress={(e) => {
if (e && e.type === 'click') {
e.currentTarget.blur();
Expand All @@ -256,47 +260,42 @@ class Button extends Component {
onPressOut={this.props.onPressOut}
onMouseDown={this.props.onMouseDown}
disabled={this.props.isLoading || this.props.isDisabled}
style={[
wrapperStyle={[
this.props.isDisabled ? {...styles.cursorDisabled, ...styles.noSelect} : {},
styles.buttonContainer,
this.props.shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
this.props.shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined,
...StyleUtils.parseStyleAsArray(this.props.style),
]}
style={[
styles.button,
this.props.small ? styles.buttonSmall : undefined,
this.props.medium ? styles.buttonMedium : undefined,
this.props.large ? styles.buttonLarge : undefined,
this.props.success ? styles.buttonSuccess : undefined,
this.props.danger ? styles.buttonDanger : undefined,
this.props.isDisabled && (this.props.success || this.props.danger) ? styles.buttonOpacityDisabled : undefined,
this.props.isDisabled && !this.props.danger && !this.props.success ? styles.buttonDisabled : undefined,
this.props.shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
this.props.shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined,
...this.props.innerStyles,
]}
hoverStyle={[
this.props.success && !this.props.isDisabled ? styles.buttonSuccessHovered : undefined,
this.props.danger && !this.props.isDisabled ? styles.buttonDangerHovered : undefined,
]}
nativeID={this.props.nativeID}
accessibilityLabel={this.props.accessibilityLabel}
hoverDimmingValue={1}
priyeshshah11 marked this conversation as resolved.
Show resolved Hide resolved
>
{({pressed, hovered}) => {
const activeAndHovered = !this.props.isDisabled && hovered;
return (
<OpacityView
shouldDim={pressed}
style={[
styles.button,
this.props.small ? styles.buttonSmall : undefined,
this.props.medium ? styles.buttonMedium : undefined,
this.props.large ? styles.buttonLarge : undefined,
this.props.success ? styles.buttonSuccess : undefined,
this.props.danger ? styles.buttonDanger : undefined,
this.props.isDisabled && (this.props.success || this.props.danger) ? styles.buttonOpacityDisabled : undefined,
this.props.isDisabled && !this.props.danger && !this.props.success ? styles.buttonDisabled : undefined,
this.props.success && activeAndHovered ? styles.buttonSuccessHovered : undefined,
this.props.danger && activeAndHovered ? styles.buttonDangerHovered : undefined,
this.props.shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
this.props.shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined,
...this.props.innerStyles,
]}
>
{this.renderContent()}
{this.props.isLoading && (
<ActivityIndicator
color={this.props.success || this.props.danger ? themeColors.textLight : themeColors.text}
style={[styles.pAbsolute, styles.l0, styles.r0]}
/>
)}
</OpacityView>
);
}}
</Pressable>
{this.renderContent()}
{this.props.isLoading && (
<ActivityIndicator
color={this.props.success || this.props.danger ? themeColors.textLight : themeColors.text}
style={[styles.pAbsolute, styles.l0, styles.r0]}
/>
)}
</PressableWithFeedback>
);
}
}
Expand Down
35 changes: 16 additions & 19 deletions src/components/Pressable/GenericPressable/BaseGenericPressable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ const GenericPressable = forwardRef((props, ref) => {
return props.disabled || shouldBeDisabledByScreenReader;
}, [isScreenReaderActive, enableInScreenReaderStates, props.disabled]);

const onLongPressHandler = useCallback(() => {
const onLongPressHandler = useCallback((event) => {
if (isDisabled) {
return;
}
Expand All @@ -76,12 +76,12 @@ const GenericPressable = forwardRef((props, ref) => {
if (ref && ref.current) {
ref.current.blur();
}
onLongPress();
onLongPress(event);

Accessibility.moveAccessibilityFocus(nextFocusRef);
}, [shouldUseHapticsOnLongPress, onLongPress, nextFocusRef, ref, isDisabled]);

const onPressHandler = useCallback(() => {
const onPressHandler = useCallback((event) => {
if (isDisabled) {
return;
}
Expand All @@ -91,20 +91,17 @@ const GenericPressable = forwardRef((props, ref) => {
if (ref && ref.current) {
ref.current.blur();
}
onPress();
onPress(event);

Accessibility.moveAccessibilityFocus(nextFocusRef);
}, [shouldUseHapticsOnPress, onPress, nextFocusRef, ref, isDisabled]);

const onKeyPressHandler = useCallback(
(event) => {
if (event.key !== 'Enter') {
return;
}
onPressHandler();
},
[onPressHandler],
);
const onKeyPressHandler = useCallback((event) => {
if (event.key !== 'Enter') {
return;
}
onPressHandler(event);
}, [onPressHandler]);

useEffect(() => {
if (!keyboardShortcut) {
Expand All @@ -119,14 +116,14 @@ const GenericPressable = forwardRef((props, ref) => {
hitSlop={shouldUseAutoHitSlop && hitSlop}
onLayout={onLayout}
ref={ref}
onPress={!isDisabled && onPressHandler}
onLongPress={!isDisabled && onLongPressHandler}
onKeyPress={!isDisabled && onKeyPressHandler}
onPressIn={!isDisabled && onPressIn}
onPressOut={!isDisabled && onPressOut}
onPress={!isDisabled ? onPressHandler : undefined}
onLongPress={!isDisabled ? onLongPressHandler : undefined}
onKeyPress={!isDisabled ? onKeyPressHandler : undefined}
onPressIn={!isDisabled ? onPressIn : undefined}
onPressOut={!isDisabled ? onPressOut : undefined}
style={(state) => [
getCursorStyle(isDisabled, [props.accessibilityRole, props.role].includes('text')),
props.style,
StyleUtils.parseStyleFromFunction(props.style, state),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is required to apply styles based on the component states like isHovered, isDisabled, etc just like the lines below it. @robertKozik told me to add it so maybe he can add more context here if needed

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea inside BaseGenericPressable was to spread styling based on Pressable state into different props (HoverStyle, focusStyle etc.). So in place of:

style={({isHovered, isPressed}) => ({
    isHovered && <<hoverStyle>>,
    isPressed && <<pressStyle>>
})}

We would get:

pressedStyle={<<pressStyle>>}
hoverStyle={<<hoverStyle>>}

But as PressableWithFeedback shifts all these state-aware styling into opacityView and passes wrapperStyle as the style prop to GenericPressable, I suggested this change in order to still have the possibility of state-aware styling inside the wrapperStyle prop.

During our convo with @priyeshshah11 I came up with this change because I thought It could be used there.
All in all, even when there is no use here, this change is beneficial.

isScreenReaderActive && StyleUtils.parseStyleFromFunction(props.screenReaderActiveStyle, state),
state.focused && StyleUtils.parseStyleFromFunction(props.focusStyle, state),
state.hovered && StyleUtils.parseStyleFromFunction(props.hoverStyle, state),
Expand Down
29 changes: 25 additions & 4 deletions src/components/Pressable/PressableWithFeedback.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React, {forwardRef} from 'react';
import React, {forwardRef, useEffect, useState} from 'react';
import _ from 'underscore';
import propTypes from 'prop-types';
import {InteractionManager} from 'react-native';
import GenericPressable from './GenericPressable';
import GenericPressablePropTypes from './GenericPressable/PropTypes';
import OpacityView from '../OpacityView';
Expand All @@ -24,21 +25,41 @@ const PressableWithFeedbackDefaultProps = {

const PressableWithFeedback = forwardRef((props, ref) => {
const propsWithoutStyling = _.omit(props, omittedProps);
const [disabled, setDisabled] = useState(props.disabled);

useEffect(() => {
setDisabled(props.disabled);
}, [props.disabled]);

return (
<GenericPressable
ref={ref}
style={props.wrapperStyle}
// eslint-disable-next-line react/jsx-props-no-spreading
{...propsWithoutStyling}
disabled={disabled}
onPress={(e) => {
setDisabled(true);
const onPress = props.onPress(e);
InteractionManager.runAfterInteractions(() => {
if (!(onPress instanceof Promise)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nab, but if onPress is not an instance of Promise, does it still have to be inside InteractionManager?

could you elaborate difference between the above code and the following:

const onPress = props.onPress(e);
if (!(onPress instanceof Promise)) {
   setDisabled(props.disabled);
   return;
}

InteractionManager.runAfterInteractions(() =>
  onPress.then(() => {
                          setDisabled(props.disabled);
                      });
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I would prefer for it to be inside InteractionManager to be sure it runs only once as on some android devices, there is still a possibility for it to capture multiple interactions before onPress's completion

Copy link
Collaborator

Choose a reason for hiding this comment

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

on some android devices, there is still a possibility for it to capture multiple interactions before onPress's completion

I am not sure about this when we have sync calls, but it's okay to keep what it is right now.

setDisabled(props.disabled);
Comment on lines +44 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

The props.disabled that we are using inside the InteractionManager.runAfterInteractions callback or the one after the promise is resolved (few lines below) is evaluated at the time we call onPress and that value is used later. Meaning props.disabled does not reflect the current state but the state that we evaluated when onPress was initially called.

To better explain, here is an example:

  1. props.disabled is false
  2. Call onPress
  3. Evaluate the promise callback, After the promise is resolved we will call setDisabled(false) // props.disabled is evaluated now
  4. Promise is not resolved yet (button action still being executed)
  5. props.disabled is set to true
  6. Promise is resolved
  7. Now we will call the previously evaluated callback which is setDisabled(false) even though the current props.disabled is true

This logic is copied from OptionRow but we missed that case in both PRs which lead to a regression #20983.

return;
}
onPress.then(() => {
setDisabled(props.disabled);
});
});
}}
>
{(state) => (
<OpacityView
shouldDim={state.pressed || state.hovered}
shouldDim={!disabled && (state.pressed || state.hovered)}
dimmingValue={state.pressed ? props.pressDimmingValue : props.hoverDimmingValue}
style={[
StyleUtils.parseStyleFromFunction(props.style, state),
state.pressed && StyleUtils.parseStyleFromFunction(props.pressStyle, state),
state.hovered && StyleUtils.parseStyleAsArray(props.hoverStyle, state),
!disabled && state.pressed && StyleUtils.parseStyleFromFunction(props.pressStyle, state),
!disabled && state.hovered && StyleUtils.parseStyleAsArray(props.hoverStyle, state),
state.focused && StyleUtils.parseStyleAsArray(props.focusStyle, state),
]}
>
Expand Down
2 changes: 1 addition & 1 deletion src/libs/Accessibility/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const useScreenReaderStatus = () => {
useEffect(() => {
const subscription = AccessibilityInfo.addEventListener('screenReaderChanged', setIsScreenReaderEnabled);

return subscription.remove;
return subscription && subscription.remove;
priyeshshah11 marked this conversation as resolved.
Show resolved Hide resolved
}, []);

return isScreenReaderEnabled;
Expand Down
3 changes: 2 additions & 1 deletion src/libs/actions/Policy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,7 @@ function generatePolicyID() {
* @param {Boolean} [makeMeAdmin] Optional, leave the calling account as an admin on the policy
* @param {String} [policyName] Optional, custom policy name we will use for created workspace
* @param {Boolean} [transitionFromOldDot] Optional, if the user is transitioning from old dot
* @returns {Promise}
*/
function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '', transitionFromOldDot = false) {
const policyID = generatePolicyID();
Expand Down Expand Up @@ -1078,7 +1079,7 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '',
},
);

Navigation.isNavigationReady().then(() => {
return Navigation.isNavigationReady().then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Coming from #17452 (comment), this return promise is not used anywhere. So this change is unnecessary.
Can you please confirm?
cc: @roryabraham @priyeshshah11 @s77rt @mananjadhav

Copy link
Contributor

@s77rt s77rt May 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Oops we were actually supposed to return a value on Button. We still need the promise though. Actually we may not need it. I raised this here #18122 (comment) already. Gven that this worked even that the promise was never used then it's probably not needed.

@priyeshshah11 Can you please raise a quick follow up PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

But I couldn't reproduce the original issue with this PR. Still need promise though?

Copy link
Contributor

Choose a reason for hiding this comment

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

cc: @tgolen @ntdiary

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we don't need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@s77rt what's the conclusion here? do you still want me to raise another PR? to remove the promise part or instead return it from the onPress? My view is that we should keep it & return it but up to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

@priyeshshah11 Raise a PR that simply return the onPress on Button. (promise may be removed on the other PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@s77rt here it is #19378

if (transitionFromOldDot) {
Navigation.dismissModal(); // Dismiss /transition route for OldDot to NewDot transitions
}
Expand Down
1 change: 1 addition & 0 deletions src/pages/workspace/WorkspacesListPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ class WorkspacesListPage extends Component {
)}
<FixedFooter style={[styles.flexGrow0]}>
<Button
accessibilityLabel={this.props.translate('workspace.new.newWorkspace')}
success
text={this.props.translate('workspace.new.newWorkspace')}
onPress={() => Policy.createWorkspace()}
Expand Down
2 changes: 1 addition & 1 deletion src/styles/variables.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,5 +135,5 @@ export default {
// This is calculated based on the values specified in the 'getGoogleListViewStyle' function of the 'StyleUtils' utility
googleEmptyListViewHeight: 14,
hoverDimValue: 0.5,
pressDimValue: 0.2,
pressDimValue: 0.8,
priyeshshah11 marked this conversation as resolved.
Show resolved Hide resolved
};