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

fix: Fix Touchable with href is still clickable if disabled #2299

Closed
wants to merge 2 commits into from

Conversation

rnike
Copy link
Contributor

@rnike rnike commented Jun 8, 2022

fix #2298

According to the implementation of View

if (props.href != null) {
component = 'a';

div will be changed to a if props.href is provided, the a will be clickable no matter if props.disabled is true or not.

TouchableHighlight and TouchableOpacity are both using View as a base component


This PR will add pointerEvents="none" to the component if props.disabled is true in TouchableHighlight and TouchableOpacity to make them actually disabled.

Tested on the example app and also our app.

@rnike rnike marked this pull request as draft June 8, 2022 03:57
@rnike rnike marked this pull request as ready for review June 8, 2022 05:10
@rnike rnike marked this pull request as draft June 8, 2022 07:45
@rnike rnike marked this pull request as ready for review June 8, 2022 07:49
@necolas necolas added this to the 0.18: StyleSheet milestone Jun 8, 2022
@necolas
Copy link
Owner

necolas commented Jun 8, 2022

The issue is because the href is still followed by the browser, so ideally there'd be no href set when the element is disabled. However, that does either require each Touchable/Pressable component to put some conditional logic around href, or for View and Text to understand the concept of being disabled so that they can avoid setting href.

If we go with the pointerEvents solution, is the intent of your patch that the pointerEvents value could be overridden? I would have assumed we want it enforced, in which case we should place pointerEvents={disabled ? 'none' : null} after the {...rest} spread.

@rnike
Copy link
Contributor Author

rnike commented Jun 9, 2022

@necolas I'm letting pointerEvents be overridable in purpose because forcing it to none if disabled is going to change these components' behaviour. For example, someone who is currently using TouchableOpacity with pointerEvents will find it working otherwise after having this upgrade. But thinking it through again, this seems to be an over-design because after all pointerEvents is not available for Touchables, I'm changing it back to pointerEvents={disabled ? 'none' : undefined}(null is incompatible with propertiy pointerEvents ), thanks for the review.

@rnike rnike force-pushed the master branch 2 times, most recently from 4c84fdf to e8bc59a Compare June 9, 2022 02:45
necolas pushed a commit that referenced this pull request Jun 10, 2022
@necolas necolas added the has: pr Subject of a pull request label Jun 10, 2022
@necolas
Copy link
Owner

necolas commented Jun 10, 2022

This patch is included in the latest canary release: [email protected]

@necolas necolas closed this in 094bd0e Jun 10, 2022
rnike added a commit to VeryBuy/react-native-web that referenced this pull request Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has: pr Subject of a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Touchable with href can still be clicked when disabled is true
2 participants