Restore TouchableHighlight and TouchableOpacity behaviour on TV platforms

[gtebbutt]

Since #18470, the default focus behaviour of TouchableHighlight and TouchableOpacity has been missing on tvOS. This uses the new touchableHandleFocus and touchableHandleBlur functions to restore the behaviour. Fixes #21295.

Test Plan:

Minimal example below works with expected colour/opacity changes on tvOS on React Native 0.55, but not on 0.57. This PR restores the documented behaviour as in 0.55.

import React, { Component } from 'react';
import {
  Platform,
  StyleSheet,
  Text,
  View,
  TouchableHighlight,
  TouchableOpacity
} from 'react-native';

export default class App extends Component {
  render() {
    return (
      <View style={styles.container}>
        <Text style={styles.welcome}>
          Welcome to React Native!
        </Text>
        <TouchableHighlight style={styles.button} onPress={() => { console.log('Pressed') }} underlayColor="red"><Text style={{color: 'white'}}>Button</Text></TouchableHighlight>
        <TouchableOpacity style={styles.button} onPress={() => { console.log('Pressed') }}><Text style={{color: 'white'}}>Button</Text></TouchableOpacity>
      </View>
    );
  }
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'center',
    alignItems: 'center',
    backgroundColor: '#F5FCFF',
  },
  welcome: {
    fontSize: 20,
    textAlign: 'center',
    margin: 10,
  },
  button: {
    padding: 10,
    borderColor: 'black',
    backgroundColor: 'blue',
    borderWidth: 1,
    borderRadius: 5
  }
});

Release Notes:

[GENERAL] [BUGFIX] [Touchable] - Restore focus behaviour for TouchableHighlight and TouchableOpacity on tvOS

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[craigrbruce]

Excellent work @gtebbutt. Merging this pull request will unblock our project.

[Justimiano]

Thank you @gtebbutt and everyone involved. I can't wait to try it.

[Weetbix]

Thank you for the fix @gtebbutt This is blocking a project of mine 👍

[pull-bot]

	Warnings
⚠️	📋 Changelog - This PR appears to be missing Changelog.

Generated by 🚫 dangerJS

[gtebbutt]

@kelset @dlowder-salesforce No doubt you guys are super busy, but I wanted to check if there's any chance of getting this merged?

Looks like this is blocking several TV projects (my own included), so it would be very much appreciated if this could get in for the next release.

[davidwico]

Any update on this ? Really need that to continue work

[cpojer]

I'm worried that this is a breaking change that removes functionality that is part of Touchable, and that it would break code that consumes Touchable directly. Could we instead change this to pass a prop to Touchable that disables the onFocus and onBlur, indicating that it is dealt with at a higher level instead? I would feel much better about landing it then.

[gtebbutt]

@cpojer Thank you for following up - I very much appreciate the feedback. Apologies in advance if I've misunderstood at all; this is my first jump into the RN source!

I'd look at it slightly differently, in that the actual breaking change was #18470 - it changed the interface in a way that removed functionality on all non-touch platforms, both for the default components and for third-parties that consume Touchable directly.

It does so silently, making the issue difficult to trace, and it does so in a way that's not consistent between touch and non-touch platforms. It's also comparatively difficult to fix, as the end-user needs to dig inside the library to find and duplicate the touch functionality in onFocus, rather than the component author being able to do so consistently across platforms.

What I'd say this PR does is takes the existing silent breaking change, and makes it visible. Implementers of Touchable now get a clear error rather than a silent failure.

I'd also worry that a flag would create an inconsistency between touch and non-touch UIs that would be somewhat frustrating for cross-platform developers.

All that said, I'm happy to make changes if you do still think they would be necessary!

[cpojer]

@gtebbutt yes, you are right that the other change was a an unintended breaking change but that has nothing to do with this PR. The point I'm trying to make is that you are removing functionality from <Touchable />. This means that if there are external components that directly build on top of Touchable instead of TouchableBounce etc., they will lose the onFocus and onBlur behavior after we land your diff. That's not good, and would be hard to detect by people.

What I am proposing instead is to keep the focus and blur functionality in Touchable and adding a prop to disable the default focus and blur functionality. This prop would be passed by all the components you are modifying in this PR that are wrapping Touchable. That way you'd fix the bug in question without breaking Touchable's focus and blur behavior. The downside is that maintainers of external components will have to make the same adjustment if they are supporting non-touch platforms, but that should be ok.

[gtebbutt]

Understood. I was under the impression that a missing abstract method implementation on a component using a mixin would throw an error, but it looks like I was mistaken on that one.

Since Touchable is a mixin rather than a HOC, and mixins apparently don't allow default implementations of abstract methods, I will admit that I'm not entirely sure how to go about this in a way that would still allow users of TouchableHighlight, for example, to define onFocus & onBlur props (even with a flag) while retaining the onShowUnderlay function.

I'm definitely the newbie on this codebase though, so happy to do some digging and figure it out assuming you're confident that it's the better option, though.

[cpojer]

Oh wow, I have to admit I thought it was a higher order component but now I realize Touchable is a module from another era. I’ll give this another look tomorrow as well and will reply then unless you figure out a safe solution until then :)

[gtebbutt]

All good, thanks for taking the time! I'll do a little more digging this afternoon, but unless there's a flash of inspiration I'll wait for your follow up.

[cpojer]

Since mixins don't allow overwriting of methods, the only safe way I can think of is using a pattern similar to this: https://codesandbox.io/s/qzmx555mz6

Basically the idea is not to change Touchable at all, but instead also export a second mixin on Touchable that does not come with onFocus and onBlur which is then used in the other components you are changing in your PR. It's not the cleanest solution but it would ensure we don't break anyone.

[analysis-bot]

Code analysis results:

• eslint found some issues.

[gtebbutt]

Good thinking - I've updated the PR to follow that pattern.

[gtebbutt]

Looks like the change is causing some flow errors - looking into it.

[gtebbutt]

OK, should be good to go now - AppVeyor issue looks unrelated.

[facebook-github-bot]

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[cpojer]

Thank you!! This solution looks reasonable, I'm gonna land it. :)

[react-native-bot]

@gtebbutt merged commit cc4211c into facebook:master.