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

setNativeProps ignores previous style changes #1935

Closed
piaskowyk opened this issue Mar 12, 2021 · 10 comments
Closed

setNativeProps ignores previous style changes #1935

piaskowyk opened this issue Mar 12, 2021 · 10 comments
Assignees
Labels
has: pr Subject of a pull request resolution: wontfix

Comments

@piaskowyk
Copy link
Contributor

The problem

Update react-native-web from 0.12.3 to 0.13.0 broke setNativeProps function, and next call setNativeProps override previously applayed style.

For example. I have component:

<View
  ref={viewRef}
  style={{ width: 100, height: 100, background: "#333" }}
/>

I want to change the style of the component.
Operation:

viewRef.setNativeProps({ style: { width: 200 } });
viewRef.setNativeProps({ style: { height: 200 } })

Result in version 0.12.3:
view style = { width: 200, height: 200 }

Result in version 0.13.0 (and newer):
view style = { width: 100, height: 200 }

How to reproduce

Just run both apps

Example in version 0.12.3: https://codesandbox.io/s/rnw-12-y11uy
Example in version 0.13.0: https://codesandbox.io/s/rnw-13-sbuto

Steps to reproduce:

  1. Run both examples
  2. Click a couple of times on the button
  3. Observe differences

Expected behavior

I expect behavior like was in version 0.12.3

Environment (include versions). Did this work in previous versions?

  • React Native for Web (version): 0.13.0
  • React (version): 17.01
  • Browser: 17.01

Additional context

In react-native-reanimated we use setNativeProps to apply changed style during an animation. But after upgrade react-native-web, we have strange issues on the web version if one animation is longer than another component back to the initial style.

Example issue:

I made a little investigation and confirmed that setNativeProps doesn't change the initial styles of component:

const nextDomStyle = domProps.style;
if (previousStyleRef.current != null) {
if (domProps.style == null) {
domProps.style = {};
}
for (const styleName in previousStyleRef.current) {
if (domProps.style[styleName] == null) {
domProps.style[styleName] = '';
}
}
}

I can prepare PR to apply changes to the initial style, but I'm not sure how this can affect other components.

Thanks for any help!

@necolas
Copy link
Owner

necolas commented Mar 12, 2021

Sure, you can make a PR. Just be aware that the behavior of setNativeProps is undefined in RN and complicated to implement on web.

@necolas necolas added this to the 0.15.* milestone Mar 25, 2021
necolas pushed a commit that referenced this issue Mar 29, 2021
setNativeProps method was ignoring previously changed style.

Close #1939
Fix ##1935
@necolas
Copy link
Owner

necolas commented Mar 29, 2021

This patch is available in 0.15.2

@necolas
Copy link
Owner

necolas commented Mar 29, 2021

c0abdbf

@necolas necolas closed this as completed Mar 29, 2021
@nandorojo
Copy link
Contributor

Thank you both for the quick fix here.

@necolas
Copy link
Owner

necolas commented Apr 13, 2021

Re-opening as the patch caused a regression and has been reverted in 0.15.7

@necolas necolas reopened this Apr 13, 2021
@necolas necolas modified the milestones: 0.15.*, 0.17 Apr 13, 2021
@necolas
Copy link
Owner

necolas commented Apr 16, 2021

This will be very hard if not impossible to match react native. The good news is that this API will eventually be deprecated and removed from RN. The bad news is that the way React DOM updates, combined with the way RN style is converted to RDOM className + style, means that React DOM updates won't always clear away styles set using setNativeProps. Ideally an API like this would be integrated with the platform renderer. Same is true for touching the DOM outside of render, e.g., during style injection. I've brought this up many times with the React team but there is no progress because they're all focused on trying to get concurrent mode out.

Ultimately, I think React DOM is not the ideal renderer, but we need to use it for compat with the existing ecosystem. One day we might be able to build a new renderer with no backwards compat for React DOM and it would be a lot more efficient and powerful

@necolas necolas removed this from the 0.17 milestone Apr 22, 2021
hyochan pushed a commit to hyochan/hackatalk that referenced this issue Jul 28, 2021
`transform` animation value is not preserved between render on mobile. This is the reason that `scale` is return to origin when writing a text on `textInput` which invokes `re-render`.  So I changed animation value from `scale` to `width` and `height`.

But there is a bug about `withSpring` function on `web`, then I splits an animation value according to platform. 
This bug seems to have related with `react-native-web`.  You can check relate issue with [here](software-mansion/react-native-reanimated#1804) or [here](necolas/react-native-web#1935).
jmysliv added a commit to software-mansion/react-native-reanimated that referenced this issue Aug 12, 2021
setNativeProps method was ignoring previously changed style, so before the method call, I merge the current style with the previous one to overcome this issue. Detailed description: necolas/react-native-web#1935
imjlk pushed a commit to imjlk/hackatalk that referenced this issue Aug 19, 2021
`transform` animation value is not preserved between render on mobile. This is the reason that `scale` is return to origin when writing a text on `textInput` which invokes `re-render`.  So I changed animation value from `scale` to `width` and `height`.

But there is a bug about `withSpring` function on `web`, then I splits an animation value according to platform. 
This bug seems to have related with `react-native-web`.  You can check relate issue with [here](software-mansion/react-native-reanimated#1804) or [here](necolas/react-native-web#1935).
@nandorojo
Copy link
Contributor

@piaskowyk can you think of an alternate solution for Reanimated, since it sounds unlikely that this patch will be in RNW?

Currently, Reanimated only functions properly with RNW 0.15.2 - 0.15.6. This is difficult for web adoption.

Could reanimated internally keep track of the previous value it passed to setNativeProps?

@nandorojo
Copy link
Contributor

Reanimated now has an updated version that sort of solves this internally. So you could probably close this issue.

However, Reanimated on Web still has its occasional bugs, namely for springs or styles that update after a component gets unmounted/remounted.

It's possible these are bugs I should open on the Reanimated repo. But they feel more like race conditions that come about from the use of setNativeProps.

Given the limitations of setNativeProps (and the plans to deprecate it), would it be a bad idea to directly manipulate the DOM node's style inside of Reanimated, rather than use setNativeProps?

RNW exposes the DOM ref, so I figured this might be a better option. Reanimated is hugely a popular library whose internals are very low-level, so it seems like an appropriate use-case to go around RNW.

I'm asking a bit naïvely, since there may be some obvious reasons not to do that.

@elan
Copy link

elan commented Dec 6, 2021

I'm on Reanimated v2.2.4, react-native-web v0.17.5 and I still get an error:

TypeError
Cannot read property 'previousStyle' of undefined

Screen Shot 2021-12-06 at 9 23 31 AM

Anything obvious I'm missing? 😅

@nandorojo
Copy link
Contributor

Yeah, use 2.2.3. 2.2.4 is broken on web I think.

@necolas necolas added this to the 0.20: React 18 milestone Jul 8, 2022
@necolas necolas self-assigned this Aug 26, 2022
necolas added a commit that referenced this issue Aug 27, 2022
Previously deprecated, and not supported in the React Native Fabric
architecture.

Close #1935
@necolas necolas added the has: pr Subject of a pull request label Aug 27, 2022
necolas added a commit that referenced this issue Aug 27, 2022
Previously deprecated, and not supported in the React Native Fabric
architecture.

Close #1935
necolas added a commit that referenced this issue Aug 30, 2022
Previously deprecated, and not supported in the React Native Fabric
architecture.

Close #1935
necolas added a commit that referenced this issue Sep 1, 2022
Previously deprecated, and not supported in the React Native Fabric
architecture.

Close #1935
rnike pushed a commit to VeryBuy/react-native-web that referenced this issue Sep 13, 2022
setNativeProps method was ignoring previously changed style.

Close necolas#1939
Fix #necolas#1935
necolas added a commit that referenced this issue Nov 28, 2022
Previously deprecated, and not supported in the React Native Fabric
architecture.

Close #1935
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 resolution: wontfix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants