-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Disables the Button during onPress call in PressableWithFeedback #18122
Disables the Button during onPress call in PressableWithFeedback #18122
Conversation
@roryabraham @s77rt 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] |
…eshshah11-disable-buttons
@priyeshshah11 Thank you. I will review asap, in the meantime can you please fill the details section of the PR highlighting the main changes here? (e.g. refactor Button to use PressableWithFeedback, etc.) |
@s77rt sorry had totally missed that, updated now! |
@@ -109,6 +109,9 @@ const propTypes = { | |||
|
|||
/** Id to use for this button */ | |||
nativeID: PropTypes.string, | |||
|
|||
/** accessibility label for the component */ | |||
accessibilityLabel: PropTypes.string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be required
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- Migrate from Pressable to PressableWithFeedback for Button
- Disable PressableWithFeedback on press
cc @roryabraham thoughts on this?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
src/components/Button/index.js
Outdated
(this.props.isDisabled && (this.props.success || this.props.danger)) ? styles.buttonOpacityDisabled : undefined, | ||
(this.props.isDisabled && !this.props.danger && !this.props.success) ? styles.buttonDisabled : undefined, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 />
style={state => [ | ||
getCursorStyle(isDisabled, [props.accessibilityRole, props.role].includes('text')), | ||
props.style, | ||
StyleUtils.parseStyleFromFunction(props.style, state), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
src/libs/actions/Policy.js
Outdated
@@ -949,7 +950,7 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '', | |||
], | |||
}); | |||
|
|||
Navigation.isNavigationReady() | |||
return Navigation.isNavigationReady() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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:
- https://github.com/Expensify/react-native-onyx/blob/0fbe0a4d077f96c4bc30b0b14359f634b271b03b/lib/Onyx.js#L862
- https://github.com/Expensify/react-native-onyx/blob/0fbe0a4d077f96c4bc30b0b14359f634b271b03b/lib/Onyx.js#L778
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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? 🙂
There was a problem hiding this comment.
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.
@priyeshshah11 Can you please fix the hover style (and press style). The button looks too much dimmed than it should |
So earlier we were using the same dimming value for hover & press which was 0.5 & now cc: Hi 👋 @shawnborton 😄 |
…eshshah11-disable-buttons
It looks too dim for a hover state 😅 Let's get this to reflect the staging values |
src/components/Button/index.js
Outdated
(this.props.isDisabled && (this.props.success || this.props.danger)) ? styles.buttonOpacityDisabled : undefined, | ||
(this.props.isDisabled && !this.props.danger && !this.props.success) ? styles.buttonDisabled : undefined, |
There was a problem hiding this comment.
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 />
src/components/Button/index.js
Outdated
(this.props.success && isHovered && !isDisabled) ? styles.buttonSuccessHovered : undefined, | ||
(this.props.danger && isHovered && !isDisabled) ? styles.buttonDangerHovered : undefined, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
{...propsWithoutStyling} | ||
disabled={disabled} | ||
onPress={(e) => { | ||
if (disabled) { return; } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
happy to change it to whatever needed, but can we please get a confirmation from @robertKozik & @shawnborton first? As I believe @robertKozik added that value here |
Hmm we already have a value for the hover color right? And then when it is actively being pressed, we can use the .2 opacity dim. |
yup that's correct, hover has greenHover colour & a 0.5 opacity dim & press has 0.2 opacity dim. |
We should not be dimming opacity on hover, we just use a different color for that. We should just have a global dim on the pressable stuff. |
@robertKozik could you please share your thoughts on why you had added/used |
@s77rt I have made the changes as per Shawn's comments above, i.e. use hover colours when hovered (without any dim) & use 0.2 opacity dim when pressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a slight change
src/components/Button/index.js
Outdated
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<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]} | |
/> | |
)} | |
</> |
@priyeshshah11 Thank you @shawnborton Isn't 0.2 too dim compared to staging or this is actually intended? |
Hello guys, I'm working on an issue that need to use PressableWithFeedback and I realized when user first try to click the Submit button, the cursor will briefly change into none-select cursor for a while. Is this a feature? Screen.Recording.2023-05-04.at.15.30.02.mov |
That happens when a button is disabled, and we disable the button when initially clicked until the onPress is completed to avoid multiple quick clicks. But that should be only in this PR, are you using this PR in your case? could you link that issue for reference? |
@priyeshshah11 I'm working on this issue (#17103), I need to replace TouchableOpacity with Pressable but still want to keep the activeOpacity behavior, so I decided to use PressableWithFeedback, it's working fine with mWeb, but with Web version, it will look like this: Screen.Recording.2023-05-04.at.15.44.47.movSo I have questions:
|
@priyeshshah11 @roryabraham Can you please address @hungvu193's concern. Should we add an option to opt out from the "disable button on click" functionality? |
@hungvu193 I cannot answer the 1st point as that's for @roryabraham to decide but I don't see any harm in adding it if you need it. For the second point, currently there is no way to prevent this. Either way, it doesn't need to hold up this PR. cc: @s77rt |
If @shawnborton thinks this is best, I think we can make that change without further discussion |
@priyeshshah11 the issue is the better place to ask this question than in the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting concern from @hungvu193, and not one that I considered before. I think it is a feature that:
- When a Pressable becomes disabled, then we show the none-select cursor.
- When a Pressable is pressed, it becomes disabled until
onPress
completes to prevent multiple repeated interactions.
However, when these things both happen in a very short time period, it looks janky, and that's a problem we should solve.
In order to solve that problem, I suggest a naïve workaround to only change the cursor style after a brief timeout. Something like this (haven't tested but this should illustrate the idea):
const isDisabled = useMemo(() => {
let shouldBeDisabledByScreenReader = false;
if (enableInScreenReaderStates === CONST.SCREEN_READER_STATES.ACTIVE) {
shouldBeDisabledByScreenReader = !isScreenReaderActive;
}
if (enableInScreenReaderStates === CONST.SCREEN_READER_STATES.DISABLED) {
shouldBeDisabledByScreenReader = isScreenReaderActive;
}
return props.disabled || shouldBeDisabledByScreenReader;
}, [isScreenReaderActive, enableInScreenReaderStates, props.disabled]);
const [shouldUseDisabledCursor, setShouldUseDisabledCursor] = useState(isDisabled);
useEffect(() => {
if (!isDisabled) {
setShouldUseDisabledCursor(isDisabled);
} else {
setTimeout(() => setShouldUseDisabledCursor(isDisabled), 1000);
}
}, [isDisabled]);
...
...
getCursorStyle(shouldUseDisabledCursor, [props.accessibilityRole, props.role].includes('text')),
@roryabraham I assume you're asking for this feature to be implemented in a seperate PR as it wasn't in the scope for this PR, right? |
…eshshah11-disable-buttons
@roryabraham could you please confirm? or merge this PR if it's good to go? cc: @s77rt @mananjadhav |
I'm okay going either way. Deferring to @roryabraham |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we can handle this in a follow-up
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.14-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.14-14 🚀
|
@@ -1078,7 +1079,7 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '', | |||
}, | |||
); | |||
|
|||
Navigation.isNavigationReady().then(() => { | |||
return Navigation.isNavigationReady().then(() => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InteractionManager.runAfterInteractions(() => { | ||
if (!(onPress instanceof Promise)) { | ||
setDisabled(props.disabled); |
There was a problem hiding this comment.
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:
- props.disabled is false
- Call onPress
- Evaluate the promise callback, After the promise is resolved we will call setDisabled(false) // props.disabled is evaluated now
- Promise is not resolved yet (button action still being executed)
- props.disabled is set to true
- Promise is resolved
- 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.
Details
The main purpose of this PR is to disable the button to prevent multiple clicks during onPress call execution.
It updates the
PressableWithFeedback
component to achieve this. It also updates theButton
component to replace the usage ofPressable
withPressableWithFeedback
component to make use of this for all buttons. It also updates thecreateWorkspace
action to return a Promise.There are some minor changes to
BaseGenericPressable
component as well to fix some errors discovered during implementation.Fixed Issues
$ #14572
PROPOSAL: #14572 (comment)
Tests
Offline tests
Same as above
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
mweb-chrome.mov
Mobile Web - Safari
mweb-safari.mov
Desktop
desktop.mov
iOS
ios.mov
Android
disable-button-android.mov