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 2 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
70 changes: 34 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 @@ -109,6 +109,9 @@ const propTypes = {

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

/** accessibility label for the component */
priyeshshah11 marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -140,6 +143,7 @@ const defaultProps = {
shouldRemoveLeftBorderRadius: false,
shouldEnableHapticFeedback: false,
nativeID: '',
accessibilityLabel: '',
};

class Button extends Component {
Expand Down Expand Up @@ -233,7 +237,7 @@ class Button extends Component {

render() {
return (
<Pressable
<PressableWithFeedback
onPress={(e) => {
if (e && e.type === 'click') {
e.currentTarget.blur();
Expand All @@ -254,47 +258,41 @@ 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} : {},
(this.props.isDisabled && (this.props.success || this.props.danger)) ? styles.buttonOpacityDisabled : undefined,
(this.props.isDisabled && !this.props.danger && !this.props.success) ? styles.buttonDisabled : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we are moving those from inner style to wrapper style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's how the new component works, it'll make sense when you try it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please elaborate? Those styles were being passed to <OpacityView /> why we are passing them to <Pressable />

styles.buttonContainer,
this.props.shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
this.props.shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined,
...StyleUtils.parseStyleAsArray(this.props.style),
]}
style={({isHovered, isDisabled}) => [
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.success && isHovered && !isDisabled) ? styles.buttonSuccessHovered : undefined,
(this.props.danger && isHovered && !isDisabled) ? styles.buttonDangerHovered : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

The style here is not correct. Previously we had const activeAndHovered = !this.props.isDisabled && hovered; where the active state was solely base on the isDisabled prop. Now it's based on the isDisabled state which is based on both isDisabled prop and isLoading prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right for both the above comments, changes applied.

this.props.shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
this.props.shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined,
...this.props.innerStyles,
]}
nativeID={this.props.nativeID}
accessibilityLabel={this.props.accessibilityLabel}
>
{({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>
<View>
{this.renderContent()}
{this.props.isLoading && (
<ActivityIndicator
color={(this.props.success || this.props.danger) ? themeColors.textLight : themeColors.text}
style={[styles.pAbsolute, styles.l0, styles.r0]}
/>
)}
</View>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<View>
{this.renderContent()}
{this.props.isLoading && (
<ActivityIndicator
color={(this.props.success || this.props.danger) ? themeColors.textLight : themeColors.text}
style={[styles.pAbsolute, styles.l0, styles.r0]}
/>
)}
</View>
<>
{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
12 changes: 6 additions & 6 deletions src/components/Pressable/GenericPressable/BaseGenericPressable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,14 +122,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
32 changes: 29 additions & 3 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,9 +25,34 @@ 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 (
// eslint-disable-next-line react/jsx-props-no-spreading
<GenericPressable ref={ref} style={props.wrapperStyle} {...propsWithoutStyling}>
<GenericPressable
ref={ref}
style={props.wrapperStyle}
// eslint-disable-next-line react/jsx-props-no-spreading
{...propsWithoutStyling}
disabled={disabled}
onPress={(e) => {
if (disabled) { return; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this required? If the button is disabled how would the onPress even get called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're correct, I had added it as for some reason I was still able to click it while initial testing but couldn't reproduce it now so have removed it.

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}
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 @@ -742,6 +742,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 @@ -949,7 +950,7 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '',
],
});

Navigation.isNavigationReady()
return Navigation.isNavigationReady()
Copy link
Contributor

Choose a reason for hiding this comment

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

Actions should not return a promise. Just revert this. The InteractionManager seems to be enough for our case, so no promise is actually needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but didn't @roryabraham say so here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

This may create a pattern for further devs which I'm not sure if we want that.

But do we actually need a promise here?

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 we do need it, as InteractionManager is not enough. And I think we were aware of this pattern change & accepted it before assigning/commencing work on this PR as per @roryabraham's comments in Slack.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I understand that this is a new pattern. In general @s77rt is right that actions should not return a promise, but the spirit of that rule is that we should never be waiting for API requests to complete before doing something, because all our API interactions are completed optimistically.

What we need is a way to wait for the optimistic data to be written to Onyx and for subscribers to be updated before returning. But because that's async we need to use a Promise.

Copy link
Contributor

@s77rt s77rt May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Actually at this point (the following statement may change) we are waiting for onyx data update since the optimistic data use onyx SET method which is blocking (sync).

Copy link
Contributor

Choose a reason for hiding this comment

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

The promise used here does not wait for onyx. It waits for navigation, it just happen that at that point onyx data is updated.

Yes, that's a very good point. I think this is maybe fine for now, but long-term we do need a better way to wait for just the optimistic data to be written to cache and for subscribers to be updated.

optimistic data use onyx SET method which is blocking (sync).

Onyx.set is async:

We could feasibly make a sync Onyx API on iOS and Android right now using JSI, but have no way to do so on web currently. We make Onyx.set async to make it more consistent with Onyx.merge

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, sorry for the confusion I was actually referring to another behaviour. See https://expensify.slack.com/archives/C01GTK53T8Q/p1682938682704609 but that's unrelated to the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes we do need it, as InteractionManager is not enough. And I think we were aware of this pattern change & accepted it before assigning/commencing work on this PR as per @roryabraham's comments in Slack.

@priyeshshah11, hi, very sorry for the interruption. We are a little confused about the Promise here. Do you have any description or links that can show us the actual effect of this Promise? If we remove it, what problems may occur? 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ntdiary If this is not a promise then we don't have a simple way of knowing when this action completed & when to re-enable the button.

.then(() => {
if (transitionFromOldDot) {
Navigation.dismissModal(); // Dismiss /transition route for OldDot to NewDot transitions
Expand Down
3 changes: 2 additions & 1 deletion src/pages/workspace/WorkspacesListPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,9 +200,10 @@ 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()}
onPress={Policy.createWorkspace}
priyeshshah11 marked this conversation as resolved.
Show resolved Hide resolved
/>
</FixedFooter>
</ScreenWrapper>
Expand Down