Opacity in TouchableOpacity properly react to state change #32956

hetanthakkar · 2022-01-23T12:49:01Z

Summary

This PR fixes the opacity bug where it fails to properly react to state change. This PR resolves the issue detailed in #32476

Changelog

[General] [Fixed] - Fixed opacity value in TouchableOpacity

Test Plan

The code I added in componentDidUpdate does solve the issue and passes all the test cases

hetanthakkar · 2022-01-23T12:52:49Z

This is a continuation of PR 32477.

I don’t think componendidupdate will cause additional rerender because it's calling Animate.Timing and I think that Animated.Timing doesn't cause rerender each time it updates state. I verified it by console logging a statement in render method and got to know that this approach doesn’t causes additional re-renders

analysis-bot · 2022-01-23T13:31:10Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: ea74c57
Branch: main

gabrieldonadel · 2022-01-23T13:31:11Z

Libraries/Components/Touchable/TouchableOpacity.js

-    if (this.props.disabled !== prevProps.disabled) {
+    if (
+      this.props.disabled !== prevProps.disabled ||
+      this.props.style !== prevProps.style


One question, wouldn't this be true if inline styles were provided?

@gabrieldonadel This would be true only if the component recieved styles which are different than the previous/existing style

Sorry for not being clear, my question is regarding the shallow comparison between props.style and prevProps.style. If the user provides inline styles, e.g. <TouchableOpacity style={{backgroundColor:'blue'}}/> this if statement would always be true when a rerender occurs in the parent component, thus triggering _opacityInactive

@gabrieldonadel Now does it look good?

analysis-bot · 2022-01-23T13:51:19Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,300,483	+81
android	hermes	armeabi-v7a	7,639,031	+89
android	hermes	x86	8,775,929	+81
android	hermes	x86_64	8,712,830	+73
android	jsc	arm64-v8a	9,784,841	+6
android	jsc	armeabi-v7a	8,770,731	+10
android	jsc	x86	9,751,476	+16
android	jsc	x86_64	10,347,644	+17

Base commit: ea74c57
Branch: main

gabrieldonadel · 2022-01-23T19:12:14Z

Libraries/Components/Touchable/TouchableOpacity.js

+    //function to check equality of two objects
+


I think this comment is no longer necessary

Suggested change

//function to check equality of two objects

@gabrieldonadel Done! Anything else I need to do?

facebook-github-bot · 2022-01-25T14:34:58Z

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

hetanthakkar · 2022-01-25T15:22:07Z

@cortinico Can you please tell me what’s failing the test

cortinico · 2022-01-25T15:47:19Z

@cortinico Can you please tell me what’s failing the test

I don't think it's related to your change. Let me take a closer look and get back to you

cortinico · 2022-01-28T14:00:13Z

@cortinico Can you please review!?

Sorry but we're having an internal discussion that is taking a bit longer than expected.
Also, please don't comment on other unrelated PRs. As a rule of thumb, please refrain from directly pinging other users more than say once a week.

@ryancat expressed some concerns. Let me get back to you

hetanthakkar · 2022-01-28T14:11:27Z

@cortinico Can you please review!?

Sorry but we're having an internal discussion that is taking a bit longer than expected. Also, please don't comment on other unrelated PRs. As a rule of thumb, please refrain from directly pinging other users more than say once a week.

@ryancat expressed some concerns. Let me get back to you

@cortinico Okay i get it will keep that in mind next time

Krisztiaan · 2022-01-28T15:07:46Z

Libraries/Components/Touchable/TouchableOpacity.js

-    if (this.props.disabled !== prevProps.disabled) {
+    if (
+      this.props.disabled !== prevProps.disabled ||
+      JSON.stringify(prevProps.style) !== JSON.stringify(this.props.style)


You could also just check if opacity is defined in style, and skip the change, without having to stringify for simple comparison

Suggested change

JSON.stringify(prevProps.style) !== JSON.stringify(this.props.style)

flattenStyle(this.props.style)?.opacity !== undefined

Or maybe even

Suggested change

JSON.stringify(prevProps.style) !== JSON.stringify(this.props.style)

flattenStyle(prevProps.style)?.opacity !== flattenStyle(this.props.style)?.opacity !== undefined

@Krisztiaan Thank you! I changed it. @cortinico You can also see the new changes and consider that for your discussion.

facebook-github-bot · 2022-01-31T17:09:01Z

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ryancat · 2022-01-31T17:38:54Z

The problem here is a timing issue. When pressout, we already tried to reset active state here. However, that wasn't working properly as the _getChildStyleOpacityWithDefault() method still returns old opacity value from prop (expect 1, saw 0.1). The style prop is not updated until later.

We could've done this via UNSAFE_componentWillReceiveProps, or getDerivedStateFromProps, but neither of them are proper here. In fact, I believe the previous PR which checks only the opacity is better. I failed to follow up with that PR (Sorry! The notification setup on github has some issues and I failed to receive them on time), but after some thoughts, I think what he did makes sense.

From React doc:
If you need to perform a side effect (for example, data fetching or an animation) in response to a change in props, use componentDidUpdate lifecycle instead.

The author was correct that no extra re-render is expected from the change as we are using native driver.

Unless there are something else I didn't notice that changed in this PR, I suggest we go with the old PR's change.

hetanthakkar · 2022-01-31T17:54:28Z

The problem here is a timing issue. When pressout, we already tried to reset active state here. However, that wasn't working properly as the _getChildStyleOpacityWithDefault() method still returns old opacity value from prop (expect 1, saw 0.1). The style prop is not updated until later.

We could've done this via UNSAFE_componentWillReceiveProps, or getDerivedStateFromProps, but neither of them are proper here. In fact, I believe the previous PR which checks only the opacity is better. I failed to follow up with that PR (Sorry! The notification setup on github has some issues and I failed to receive them on time), but after some thoughts, I think what he did makes sense.

From React doc: If you need to perform a side effect (for example, data fetching or an animation) in response to a change in props, use componentDidUpdate lifecycle instead.

The author was correct that no extra re-render is expected from the change as we are using native driver.

Unless there are something else I didn't notice that changed in this PR, I suggest we go with the old PR's change.

@ryancat That is what I have done in this PR, comparing the opacity values in componentDidUpdate

hetanthakkar · 2022-01-31T17:56:50Z

In my old PR doing this will fail tests: this.props.style.opacity !== prevProps.style.opacity because opacity is not accessible under styles So i have first flattened the styles and then compared the opacity

Krisztiaan · 2022-01-31T22:13:19Z

@ryancat this PR is actually a better version of the previous PR, by doing the same thing, except flattening the style too. What you seem to have replied to is it's state before this review, maybe?

Anyways, if you think that PR had the right intention, this one now has the right implementation.

Krisztiaan · 2022-01-31T22:15:11Z

@hetanthakkar1 The test seems to imply they (Meta employees) need to take some action in order to progress one of the tests, I don't think this means any issue on your part.

cortinico · 2022-02-01T11:36:21Z

I'll hand this over to @ryancat as I lack relevant context to do a full review here.

The test seems to imply they (Meta employees) need to take some action in order to progress one of the tests, I don't think this means any issue on your part.

The tests you see as failed are actually an internal warning. There is nothing to do on your end @hetanthakkar1

ryancat · 2022-02-04T17:33:45Z

@hetanthakkar1 The internal errors are some warnings that are not related to your change. I've accepted the change and this should be shipped soon.

react-native-bot · 2022-02-07T12:25:05Z

This pull request was successfully merged by @hetanthakkar1 in 3eddc9a.

^{When will my fix make it into a release? | Upcoming Releases}

mjmasn · 2022-11-09T15:38:22Z

FYI this appears to have caused #34376 and #34442

I suspect the !== undefined was an unintentional typo in the suggestion here: #32956 (comment)

…32956) Summary: This PR fixes the opacity bug where it fails to properly react to state change. This PR resolves the issue detailed in facebook#32476 ## Changelog  [General] [Fixed] - Fixed opacity value in TouchableOpacity Pull Request resolved: facebook#32956 Test Plan: The code I added in componentDidUpdate does solve the issue and passes all the test cases Reviewed By: ryancat Differential Revision: D33766718 Pulled By: cortinico fbshipit-source-id: 951bedf22619fc12e66156d0a6074cd8adf1d3eb

opacity in TouchableOpacity properly react to state change

949086d

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jan 23, 2022

react-native-bot added the Bug label Jan 23, 2022

Minor bug fix

ec62ba8

gabrieldonadel reviewed Jan 23, 2022

View reviewed changes

hetanthakkar added 4 commits January 23, 2022 19:55

implemented object comapre function

397182d

minor change

68a78ee

fixed bug of object compare function

3421ace

fix object compare bug

2c29337

gabrieldonadel reviewed Jan 23, 2022

View reviewed changes

hetanthakkar added 2 commits January 24, 2022 01:40

Removed unnecassary comment

02ed380

re-run circle ci tests

ed3b222

cortinico mentioned this pull request Jan 25, 2022

Value of opacity in TouchableOpacity properly react to state change #32477

Closed

hetanthakkar mentioned this pull request Jan 28, 2022

fix: cliPath should handle absolute paths #32983

Closed

Krisztiaan reviewed Jan 28, 2022

View reviewed changes

hetanthakkar added 2 commits January 28, 2022 22:27

Merge remote-tracking branch 'upstream/main' into opacity

d100533

compare opacity

5e55e1c

cortinico requested a review from ryancat February 1, 2022 11:36

hetanthakkar mentioned this pull request Feb 4, 2022

pointerEvents prop not working for IOS scroll views #33022

Closed

facebook-github-bot closed this in 3eddc9a Feb 7, 2022

react-native-bot added the Merged This PR has been merged. label Feb 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Opacity in TouchableOpacity properly react to state change #32956

Opacity in TouchableOpacity properly react to state change #32956

hetanthakkar commented Jan 23, 2022

hetanthakkar commented Jan 23, 2022

analysis-bot commented Jan 23, 2022 •

edited

Loading

gabrieldonadel Jan 23, 2022

hetanthakkar Jan 23, 2022

gabrieldonadel Jan 23, 2022

hetanthakkar Jan 23, 2022

analysis-bot commented Jan 23, 2022 •

edited

Loading

gabrieldonadel Jan 23, 2022

hetanthakkar Jan 23, 2022

facebook-github-bot commented Jan 25, 2022

hetanthakkar commented Jan 25, 2022

cortinico commented Jan 25, 2022

cortinico commented Jan 28, 2022

hetanthakkar commented Jan 28, 2022

Krisztiaan Jan 28, 2022 •

edited

Loading

Krisztiaan Jan 28, 2022

hetanthakkar Jan 28, 2022

facebook-github-bot commented Jan 31, 2022

ryancat commented Jan 31, 2022

hetanthakkar commented Jan 31, 2022 •

edited

Loading

hetanthakkar commented Jan 31, 2022

Krisztiaan commented Jan 31, 2022

Krisztiaan commented Jan 31, 2022

cortinico commented Feb 1, 2022

ryancat commented Feb 4, 2022

react-native-bot commented Feb 7, 2022

mjmasn commented Nov 9, 2022 •

edited

Loading

	JSON.stringify(prevProps.style) !== JSON.stringify(this.props.style)
	flattenStyle(this.props.style)?.opacity !== undefined

	JSON.stringify(prevProps.style) !== JSON.stringify(this.props.style)
	flattenStyle(prevProps.style)?.opacity !== flattenStyle(this.props.style)?.opacity !== undefined

Opacity in TouchableOpacity properly react to state change #32956

Opacity in TouchableOpacity properly react to state change #32956

Conversation

hetanthakkar commented Jan 23, 2022

Summary

Changelog

Test Plan

hetanthakkar commented Jan 23, 2022

analysis-bot commented Jan 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analysis-bot commented Jan 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 25, 2022

hetanthakkar commented Jan 25, 2022

cortinico commented Jan 25, 2022

cortinico commented Jan 28, 2022

hetanthakkar commented Jan 28, 2022

Krisztiaan Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 31, 2022

ryancat commented Jan 31, 2022

hetanthakkar commented Jan 31, 2022 • edited Loading

hetanthakkar commented Jan 31, 2022

Krisztiaan commented Jan 31, 2022

Krisztiaan commented Jan 31, 2022

cortinico commented Feb 1, 2022

ryancat commented Feb 4, 2022

react-native-bot commented Feb 7, 2022

mjmasn commented Nov 9, 2022 • edited Loading

analysis-bot commented Jan 23, 2022 •

edited

Loading

analysis-bot commented Jan 23, 2022 •

edited

Loading

Krisztiaan Jan 28, 2022 •

edited

Loading

hetanthakkar commented Jan 31, 2022 •

edited

Loading

mjmasn commented Nov 9, 2022 •

edited

Loading