Skip to content
This repository has been archived by the owner on Feb 25, 2020. It is now read-only.

[Fix RN TextInput focus bug with react-navigation] Add a condition to blur the currently focused textinput #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gazoudoudou
Copy link

@gazoudoudou gazoudoudou commented Nov 11, 2018

I noticed a bug while using react-navigation + the react-native TextInput component.

HOW TO REPRODUCE THE BUG :

Let’s consider the following app with only 2 pages :

Page1.js :

import React, { PureComponent } from 'react';
import { TouchableOpacity, View, Text } from 'react-native';

export default class Page1 extends PureComponent {
  render() {
    return (
      <View>
        <TouchableOpacity
          style={{ backgroundColor: 'red', height: 30, justifyContent: 'center', alignItems: 'center' }}
          onPress={() => this.props.navigation.navigate('Page2')}
        >
          <Text>Go to page 2</Text>
        </TouchableOpacity>
      </View>
    );
  }
}

Page2.js :

import React, { PureComponent } from 'react';
import { TouchableOpacity, TextInput, Keyboard, View, Text } from 'react-native';

export default class Page2 extends PureComponent {
  componentDidMount() {
    this.textInput && this.textInput.focus();
  }

  render() {
    return (
      <View>
        <TextInput ref={ref => (this.textInput = ref)} placeholder={'Write something here'} />
        <TouchableOpacity
          style={{ backgroundColor: 'blue', height: 30, marginTop: 20, justifyContent: 'center', alignItems: 'center' }}
          onPress={Keyboard.dismiss}
        >
          <Text>Dismiss keyboard</Text>
        </TouchableOpacity>
      </View>
    );
  }
}

App.js :

import React, { Component } from 'react';
import { createStackNavigator, createAppContainer } from 'react-navigation';
import * as Pages from './pages';

const RootNavigator = createStackNavigator(
  {
    Page1: {
      screen: Pages.Page1,
    },
    Page2: {
      screen: Pages.Page2,
    },
  },
  {
    initialRouteName: 'Page1',
  }
);

const AppContainer = createAppContainer(RootNavigator);

class App extends Component {
  render() {
    return <AppContainer />;
  }
}

export default App;

On Page1 you have a single button to navigate to Page2. On Page2, you’d like that a TextInput is focused when you arrive on this page. You also have a « dismiss » button to close the keyboard when the textinput is focused.

You could use the TextInput props « autoFocus » so that it is focused at the beginning. There is absolutely no bug in this case.
But instead, I tried to create a ref « textinput », and called this.textinput.focus() in the componentDidMount method to focus it at the beginning. It works, but when I press the dismiss button, the keyboard won’t close.

After digging, I found that a blur call is made when you arrive on Page2, coming from the @react-navigation/native/src/KeyboardAwareNavigator.js file. In _handleTransitionStart, transitionProps.index !== prevTransitionProps.index is true because you are switching pages, and !!currentField too in the « this.textinput.focus() » case. As a consequence, it blurs the textinput, the TextInput.State.currentlyFocusedField() becomes null, and you cannot dismiss the keyboard anymore.

MY FIX :

I tried to add another condition in addition to the if(currentField) one, to decide when we should blur the currently focused textinput. According to me, it should be something like if(currentField && previousPage includes currentField) or if(currentField && currentPage does not include currentField).

To do so, I used the viewIsDescendantOf method from UIManager, react-native (to check if the currently focused field is a child of the previous scene). In addition to the tag of the currently focused field (which is precisely currentField), I needed the tag of the previous scene.

In the PR react-navigation/core#19, I tried to add the node tag of a sceneview in the nav params.
In this one, I use the two tags to decide when we should blur the textinput.

@horacioh
Copy link
Member

I create this snack with the code you added here and I did not see any errors with the keyboard.

can you edit the code and replicate the error?
thanks!

@gazoudoudou
Copy link
Author

You have to remove the "autoFocus" props on the textinput in Page2 (which is not supposed to be necessary since "this.textInput.focus()" is called in the componentDidMount).

@horacioh
Copy link
Member

oh yeah I added there after it. but its also working without that prop being set. :S

am I missing something?

@gazoudoudou
Copy link
Author

The problem is that the keyboard won't close when you click on the "dismiss keyboard" button. I tried in the snack and I see well the error :).

@horacioh
Copy link
Member

horacioh commented Dec 19, 2018

aaah ok!

I thought the problem was that the keyboard will be always visible after clicking the "dismiss keyboard" button, even if you go back to Page1, and that was not happening :)

cool! now we have the example running :)

thanks!

Copy link
Contributor

@ericvicenti ericvicenti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a solution that doesn't require calling into UIManager

prevTransitionProps.scene.route.params &&
prevTransitionProps.scene.route.params.nodeTag;
if (previousSceneTag) {
UIManager.viewIsDescendantOf(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this native call will be slow, and we shouldn't need to call into native APIs for this use case, because JS should already have awareness of what is focused

@@ -43,9 +43,26 @@ export default (Navigator, navigatorConfig) =>
// in the case where the index did not change, I believe. We
// should revisit this after 2.0 release.
if (transitionProps.index !== prevTransitionProps.index) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also looks like a real bug to me. Instead of comparing the index, we should see if the active key has changed. Something like this:

const lastActiveKey = prevTransitionProps.state.routes[prevTransitionProps.state.index].key;
const activeKey = transitionProps.state.routes[transitionProps.state.index].key;
if (lastActiveKey !== activeKey) {
  ...

Theoretically this could be an issue if a screen index changes but it stays active

@ericvicenti
Copy link
Contributor

Specifically, I think we need a context API that allows text inputs to negotiate focus with navigators. I welcome any proposals surrounding that, and will help with the implementation and code review.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants