callstack / react-native-paper

Material Design for React Native (Android & iOS)
https://reactnativepaper.com
MIT License
12.75k stars 2.08k forks source link

Darkmode Card onPress error with 4.0.x #2063

Closed Keplyx closed 4 years ago

Keplyx commented 4 years ago

Current behaviour

With darkmode enabled, when clicking on a Card component with onPress prop set, I get the following error:

Error: Style property 'backgroundColor' is not supported by native animated module, js engine: hermes

Expected behaviour

The onPress function should be executed and the Card background should be animated to become lighter.

Code sample

Just create any project rendering a card with the following props:

<Card
   onPress={() => console.log("cc")}
>
   <Text>Hello</Text>
</Card>

Link to a Snack example

Screenshots (if applicable)

Screenshot_20200717-164154 Screenshot_20200717_164322

What have you tried

Tried switching to light mode, no issue. Tried to go back to 3.11.1, no issue.

Your Environment

software version
ios or android android (did not try iOS)
react-native 0.63.1
react-native-paper 4.0.1 and 4.0.0
react-native-vector-icons 7.0.0
node 12.18.2
npm or yarn npm 6.14.5
expo sdk N/A
JS engine Hermes
github-actions[bot] commented 4 years ago

Couldn't find version numbers for the following packages in the issue:

Can you update the issue to include version numbers for those packages? The version numbers must match the format 1.2.3.

The versions mentioned in the issue for the following packages differ from the latest versions on npm:

Can you verify that the issue still exists after upgrading to the latest versions of these packages?

ITLight commented 4 years ago

I have met same issue, but now I fixed by changing the theme from const theme = { ...DarkTheme, }; to const theme = { ...DefaultTheme, };

wortkotze commented 4 years ago

I have the same issue. when I toggle on darkmode > the card module from react native paper throws an error for animated backgrouncColor.

This issue is only available on darkmode not on the normal theme

github-actions[bot] commented 4 years ago

Hey! Thanks for opening the issue. Can you provide a minimal repro which demonstrates the issue? Posting a snippet of your code in the issue is useful, but it's not usually straightforward to run. A repro will help us debug the issue faster. Please try to keep the repro as small as possible. The easiest way to provide a repro is on snack.expo.io. If it's not possible to repro it on snack.expo.io, then you can also provide the repro in a GitHub repository.

Keplyx commented 4 years ago

I have met same issue, but now I fixed by changing the theme from const theme = { ...DarkTheme, }; to const theme = { ...DefaultTheme, };

Yes but this means disabling the built-in DarkTheme, which is not a fix.

Hey! Thanks for opening the issue. Can you provide a minimal repro which demonstrates the issue? Posting a snippet of your code in the issue is useful, but it's not usually straightforward to run. A repro will help us debug the issue faster. Please try to keep the repro as small as possible. The easiest way to provide a repro is on snack.expo.io. If it's not possible to repro it on snack.expo.io, then you can also provide the repro in a GitHub repository.

I updated the issue to include a link to a Snack example

raajnadar commented 4 years ago

You should use the useTheme() hook to get the current theme value.

https://callstack.github.io/react-native-paper/theming.html#using-the-theme-in-your-own-components

thisBrian commented 4 years ago

This is because using backgroundColor is not supported with the native driver, it's an unresolved issue from 3 years ago: https://github.com/facebook/react-native/issues/14178

The problem is handlePressIn / handlePressOut functions for the Card component have useNativeDriver: true

https://github.com/callstack/react-native-paper/blob/2ce47a149215bfe6c7c6cb8391a5096faf8a1266/src/components/Card/Card.tsx#L112

If you go and edit both of those functions to useNativeDriver: false, it will work fine.

Keplyx commented 4 years ago

You should use the useTheme() hook to get the current theme value.

https://callstack.github.io/react-native-paper/theming.html#using-the-theme-in-your-own-components

Not in this case, I simply want to set the default theme to the dark one, the hook is only useful for custom components (which I am using when needed).

This is because using backgroundColor is not supported with the native driver, it's an unresolved issue from 3 years ago: facebook/react-native#14178

The problem is handlePressIn / handlePressOut functions for the Card component have useNativeDriver: true

https://github.com/callstack/react-native-paper/blob/2ce47a149215bfe6c7c6cb8391a5096faf8a1266/src/components/Card/Card.tsx#L112

If you go and edit both of those functions to useNativeDriver: false, it will work fine.

But I didn't have any problem with v3.x.x, did they enable nativeDriver in v4 ? I am not going to edit the lib because it makes unmaintainable code, instead I just add a TouchableRipple inside the card until it is fixed.

raajnadar commented 4 years ago

@Keplyx you should not set the theme directly to state you should use context API to store the selected theme.

I am using Context Api to switch theme and it works perfectly.

wortkotze commented 4 years ago

If you use ...DarkTheme, mode: "exact",

then also everything is fine but not without exact mode

Keplyx commented 4 years ago

@Keplyx you should not set the theme directly to state you should use context API to store the selected theme.

I am using Context Api to switch theme and it works perfectly.

The problem happens even if the theme is statically set to Dark mode, so switching themes is not the issue (until now it worked perfectly with state as I do not use Context API).

If you use ...DarkTheme, mode: "exact",

then also everything is fine but not without exact mode

You're right, didn't know about that ! It does not crash but the click animation does not play

wortkotze commented 4 years ago

yes thats right > I just use this now as a quick fix so that the application works again and I hope we find a long lasting solution :)

thisBrian commented 4 years ago

But I didn't have any problem with v3.x.x, did they enable nativeDriver in v4 ? I am not going to edit the lib because it makes unmaintainable code, instead I just add a TouchableRipple inside the card until it is fixed.

Yes it was changed for v4 with this PR #1787

As for editing the lib, I use patch-package to make small temporary patches that are relatively easy to maintain without needing to make a fork or track upstream.

SleeplessByte commented 4 years ago

@Keplyx if you're still interested in a patch-package solution, the following preserves the native driver on light mode / exact mode and only uses the JS driver on dark adaptive mode.

diff --git a/node_modules/react-native-paper/lib/commonjs/components/Card/Card.js b/node_modules/react-native-paper/lib/commonjs/components/Card/Card.js
index 2dd6d71..aa70977 100644
--- a/node_modules/react-native-paper/lib/commonjs/components/Card/Card.js
+++ b/node_modules/react-native-paper/lib/commonjs/components/Card/Card.js
@@ -78,27 +78,23 @@ class Card extends React.Component {
     });

     _defineProperty(this, "handlePressIn", () => {
-      const {
-        scale
-      } = this.props.theme.animation;
+      const { dark, mode, animation: { scale } } = this.props.theme

       _reactNative.Animated.timing(this.state.elevation, {
         toValue: 8,
         duration: 150 * scale,
-        useNativeDriver: true
+        useNativeDriver: !dark || mode === 'exact',
       }).start();
     });

     _defineProperty(this, "handlePressOut", () => {
-      const {
-        scale
-      } = this.props.theme.animation;
+      const { dark, mode, animation: { scale } } = this.props.theme

       _reactNative.Animated.timing(this.state.elevation, {
         // @ts-ignore
         toValue: this.props.elevation,
         duration: 150 * scale,
-        useNativeDriver: true
+        useNativeDriver: !dark || mode === 'exact',
       }).start();
     });
   }
diff --git a/node_modules/react-native-paper/lib/module/components/Card/Card.js b/node_modules/react-native-paper/lib/module/components/Card/Card.js
index a080816..53a37ee 100644
--- a/node_modules/react-native-paper/lib/module/components/Card/Card.js
+++ b/node_modules/react-native-paper/lib/module/components/Card/Card.js
@@ -60,25 +60,23 @@ class Card extends React.Component {
     });

     _defineProperty(this, "handlePressIn", () => {
-      const {
-        scale
-      } = this.props.theme.animation;
+      const { dark, mode, animation: { scale } } = this.props.theme
+
       Animated.timing(this.state.elevation, {
         toValue: 8,
         duration: 150 * scale,
-        useNativeDriver: true
+        useNativeDriver: !dark || mode === 'exact',
       }).start();
     });

     _defineProperty(this, "handlePressOut", () => {
-      const {
-        scale
-      } = this.props.theme.animation;
+      const { dark, mode, animation: { scale } } = this.props.theme
+
       Animated.timing(this.state.elevation, {
         // @ts-ignore
         toValue: this.props.elevation,
         duration: 150 * scale,
-        useNativeDriver: true
+        useNativeDriver: !dark || mode === 'exact',
       }).start();
     });
   }
diff --git a/node_modules/react-native-paper/src/components/Card/Card.tsx b/node_modules/react-native-paper/src/components/Card/Card.tsx
index a61cfee..28f4966 100644
--- a/node_modules/react-native-paper/src/components/Card/Card.tsx
+++ b/node_modules/react-native-paper/src/components/Card/Card.tsx
@@ -105,21 +105,23 @@ class Card extends React.Component<Props, State> {
   };

   private handlePressIn = () => {
-    const { scale } = this.props.theme.animation;
+    const { dark, mode, animation: { scale } } = this.props.theme
+
     Animated.timing(this.state.elevation, {
       toValue: 8,
       duration: 150 * scale,
-      useNativeDriver: true,
+      useNativeDriver: !dark || mode === 'exact',
     }).start();
   };

   private handlePressOut = () => {
-    const { scale } = this.props.theme.animation;
+    const { dark, mode, animation: { scale } } = this.props.theme
+
     Animated.timing(this.state.elevation, {
       // @ts-ignore
       toValue: this.props.elevation,
       duration: 150 * scale,
-      useNativeDriver: true,
+      useNativeDriver: !dark || mode === 'exact',
     }).start();
   };
PedroBern commented 4 years ago

Need to reopen this

2068 looks like solving the issue, but it still breaks if:

1: Currently on light theme 2: Press a card, for example, that opens a menu 3: The user quickly change mind and click a "toggle dark theme switch with adaptive mode"

What is happening is:

The Card component is still with the elevation animation running with dark: false, but suddenly the dark: true and the animation breaks. Just as if we tried to start the animation with the dark: true in the first place (partial solution by #2068).

The problem is here in the Surface. At this line the elevation is using native driver with the dark theme (in this very specific case of running animation while toggling the theme).

@Trancever can you reopen?

SleeplessByte commented 4 years ago

I don't feel that that is this issue. Theme shouldn't be switched whilst animations are running (it leads to more issues than just this).

2020-08-08 01:38, pedrobern wrote:

NEED TO REOPEN THIS

2068 [1] looks like solving the issue, but it still breaks if:

1: Currently on light theme 2: Press a card, for example, that opens a menu 3: The user quickly change mind and click a "toggle dark theme switch with adaptive mode"

What is happening is:

The Card component is still with the elevation animation running with dark: false, but suddenly the dark: true and the animation breaks. Just as if we tried to start the animation with the dark: true in the first place (partial solution by #2068 [1]).

-- You are receiving this because you commented. Reply to this email directly, view it on GitHub [2], or unsubscribe [3].

Links:

[1] https://github.com/callstack/react-native-paper/pull/2068 [2] https://github.com/callstack/react-native-paper/issues/2063#issuecomment-670785227 [3] https://github.com/notifications/unsubscribe-auth/AAO7SWBEMRQUGWDX44SDIFLR7SF7TANCNFSM4O6MDRFQ

PedroBern commented 4 years ago

@SleeplessByte already solved at #2113