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

ComponentWillUnmout not called for Modal in AppleTV #15313

Closed
rada opened this Issue Aug 1, 2017 · 10 comments

Comments

Projects
None yet
3 participants
@rada

rada commented Aug 1, 2017

Is this a bug report?

Yes

Have you read the Bugs section of the How to Contribute guide?

Yes

Environment

  1. react-native -v: 0.46.4
  2. node -v: 6.5.0
  3. npm -v: 3.10.3

Then, specify:

  • Target Platform: AppleTV

  • Development Operating System: macOS Sierra (10.12.5)

  • Build tools: Xcode (8.3.3)

Steps to Reproduce

Open Modal in Tab 1 by clicking on 'Open Modal' touchable.

(Write your steps here:)

  1. Focus touchable 'Open Modal' (in simulator press key Down)
  2. Press touchable 'Open Modal'
  3. Press 'Menu' button on Apple TV Remote (in simulator, press CMD + SHIFT + R to display TV Remote)

Expected Behavior

After the 'Menu' button is pressed on Apple TV Remote, Modal is closed and the method componentWillUnmout is called for component used in Modal.

Actual Behavior

Modal is closed but the componentWillUnmout method is not called if the modal is closed by pressing the 'Menu' button in the Apple TV Remote. If the modal is closed by pressing the touchable 'Close Modal', the componentWillUnmout method is called as expected.

Reproducible Demo

https://gitlab.com/radamich/tvTabBar/tree/tvosModal

index

/**
 * Sample React Native App
 * https://github.com/facebook/react-native
 * @flow
 */

import React, { Component } from 'react';
import {
  AppRegistry,
  Text,
  View,
  TabBarIOS,
  ListView,
  Modal,
  TouchableHighlight,
} from 'react-native';

import List from './List';

export default class tvTabBar extends Component {

	_tabItems: Array<{ key: string }> = [
		{ key: 'TAB 1' },
		{ key: 'TAB 2' },
		{ key: 'TAB 3' },
		{ key: 'TAB 4' },
	];

	constructor(props) {
		super(props);
		this.state = {
			activeTab: 'TAB 1',
			showModal: false,
		};
	}

	_getTabItem = (key: string) => {
		switch (key) {
			case 'TAB 1':
				return (
					<View style={{ flex: 1, top: 140, borderColor: 'red', borderWidth: 10, padding: 200 }}>
						<Modal 
							visible={this.state.showModal}
							transparent={true}
						>
							<List closeModal={this.closeModal} />

						</Modal>
						<TouchableHighlight onPress={() => {
								console.log('MODAL SHOW', this.state.showModal);
								this.setState({ showModal: true });
							}}
							underlayColor={'orange'}
						>
							<Text style={{ fontSize: 30 }}>Open Modal</Text>
						</TouchableHighlight>
					</View>
				);
			case 'TAB 2':
				return (
					<View style={{ flex: 1, top: 140, borderColor: 'green', borderWidth: 20 }}>
						<Text style={{ fontSize: 30 }}>TAB 2</Text>
					</View>
				);
			case 'TAB 3':
				return (
					<View style={{ flex: 1, top: 140, borderColor: 'darkblue', borderWidth: 30 }}>
						<Text style={{ fontSize: 30 }}>TAB 3</Text>
					</View>
				);
			case 'TAB 4':
				return (
					<View style={{ flex: 1, top: 140, borderColor: 'deeppink', borderWidth: 40 }}>
						<Text style={{ fontSize: 30 }}>TAB 4</Text>
					</View>
				);
			default:
				return null;
		}
	}

	closeModal = () => {
		this.setState({ showModal: false });
	}

	_setTab(key) {
		return () => {
			console.log('Selecting tab: ', key);
			if (this.state.activeTab !== key) {
				this.setState({ activeTab: key });
			}
		};
	}

	render() {
		return (
			<TabBarIOS>
				{
					this._tabItems.map((tabItem) => {
						return (
							<TabBarIOS.Item
								key={tabItem.key}
								onPress={this._setTab(tabItem.key)}
								title={tabItem.key}
								selected={tabItem.key === this.state.activeTab}
							>
								{ this._getTabItem(tabItem.key) }
							</TabBarIOS.Item>
						);
					})
				}
			</TabBarIOS>
		);
	}
}

AppRegistry.registerComponent('tvTabBar', () => { return tvTabBar; });

List.js

/**
 * Sample React Native App
 * https://github.com/facebook/react-native
 * @flow
 */

import React, { Component } from 'react';
import {
  Text,
  View,
  ListView,
  FlatList,
  TouchableHighlight,
} from 'react-native';

export default class List extends Component {

	
	constructor(props) {
		super(props);

		this.ds = new ListView.DataSource({ rowHasChanged: (r1: Object, r2: Object) => { return r1 !== r2; } });

		this.state = {
			dataSource: this.ds.cloneWithRows([{ value: 1 }]),
			data: [{ value: 1 }, { value: 2 }],
		}
	};

	_renderRow(rowData) {
		console.log('RenderRow: ', rowData.value);
		return (
			<View>
				<Text>{rowData.value}</Text>
			</View>
		)
	}
	
	_renderFlatListItem = ({ item }) => {
		return this._renderRow(item);
	}
	
	componentDidMount() {
		setTimeout(() => {
			const data = Array.from({ length: 2000 }).map((_, index) => {
				return { value: index };
			})
			this.setState({
				dataSource: this.ds.cloneWithRows(data),
				// data: data,
			})
		}, 3000)
	}

	componentWillUnmount() {
		console.log('List.js: componentWillUnmount');
	}
	
	render() {
		console.log('Render List');
		return (
			<View style={{ backgroundColor: 'blue', padding: 500 }}>
				<FlatList
					data={this.state.data}
					renderItem={this._renderFlatListItem}
					keyExtractor={(item) => { return item.value }}
				/>
				<TouchableHighlight onPress={() => {
						this.props.closeModal();
					}}
					underlayColor={'orange'}
				>
					<Text style={{ fontSize: 30 }}>Close Modal</Text>
				</TouchableHighlight>
			</View>
		);
	}
}
@rada

This comment has been minimized.

Show comment
Hide comment
@rada

rada commented Aug 1, 2017

@dlowder-salesforce

This comment has been minimized.

Show comment
Hide comment
@dlowder-salesforce

dlowder-salesforce Aug 1, 2017

Collaborator

Thanks for the bug report @rada , I'll have a look

Collaborator

dlowder-salesforce commented Aug 1, 2017

Thanks for the bug report @rada , I'll have a look

@dlowder-salesforce

This comment has been minimized.

Show comment
Hide comment
@dlowder-salesforce

dlowder-salesforce Aug 1, 2017

Collaborator

Hmmm this does not look like something that will be easy to fix. The Modal component isn't really a modal in the usual iOS sense -- it just renders null if visible is set to false, and renders normally otherwise. The problem is that the tvOS menu button bypasses the JS and just dismisses the view, getting the app into a bad state where JS thinks the Modal component is still there, but there is no native view for it any more. I think this component needs some work maybe....

Collaborator

dlowder-salesforce commented Aug 1, 2017

Hmmm this does not look like something that will be easy to fix. The Modal component isn't really a modal in the usual iOS sense -- it just renders null if visible is set to false, and renders normally otherwise. The problem is that the tvOS menu button bypasses the JS and just dismisses the view, getting the app into a bad state where JS thinks the Modal component is still there, but there is no native view for it any more. I think this component needs some work maybe....

@dlowder-salesforce

This comment has been minimized.

Show comment
Hide comment
@dlowder-salesforce

dlowder-salesforce Aug 2, 2017

Collaborator

Found a solution -- will be submitting a pull request shortly.

Collaborator

dlowder-salesforce commented Aug 2, 2017

Found a solution -- will be submitting a pull request shortly.

@dlowder-salesforce

This comment has been minimized.

Show comment
Hide comment
@dlowder-salesforce

dlowder-salesforce Aug 2, 2017

Collaborator

@rada you can test out my fix by using https://github.com/dlowder-salesforce/react-native/tree/tvos-combined-fixes (includes previous fixes to TabBarIOS).

You will need to add one line of code to your test case: in index.ios.js, the Modal component should look as shown below:

						<Modal 
							visible={this.state.showModal}
                                                        onMenuPress={this.closeModal}    // <------
							transparent={true}
						>
							<List closeModal={this.closeModal} />

						</Modal>
Collaborator

dlowder-salesforce commented Aug 2, 2017

@rada you can test out my fix by using https://github.com/dlowder-salesforce/react-native/tree/tvos-combined-fixes (includes previous fixes to TabBarIOS).

You will need to add one line of code to your test case: in index.ios.js, the Modal component should look as shown below:

						<Modal 
							visible={this.state.showModal}
                                                        onMenuPress={this.closeModal}    // <------
							transparent={true}
						>
							<List closeModal={this.closeModal} />

						</Modal>
@rada

This comment has been minimized.

Show comment
Hide comment
@rada

rada Aug 2, 2017

Thanks @dlowder-salesforce, it's working now with your fix. Thanks.

rada commented Aug 2, 2017

Thanks @dlowder-salesforce, it's working now with your fix. Thanks.

@dlowder-salesforce

This comment has been minimized.

Show comment
Hide comment
@dlowder-salesforce

dlowder-salesforce Aug 2, 2017

Collaborator

@rada in further work on this issue, I find that I need to change the onMenuPress property to onRequestClose instead, for consistency with Android (where the back button does the same thing). My repo has been updated with this change.

Collaborator

dlowder-salesforce commented Aug 2, 2017

@rada in further work on this issue, I find that I need to change the onMenuPress property to onRequestClose instead, for consistency with Android (where the back button does the same thing). My repo has been updated with this change.

facebook-github-bot added a commit that referenced this issue Aug 17, 2017

Fix for Modal behavior when menu button pressed on Apple TV (Issue #1…
…5313)

Summary:
**Motivation**

On Apple TV, pressing the menu button destroys the native view that backs the `Modal` component, causing an app using this component to get into a broken state.  This fix implements `onRequestClose` for tvOS to have the same behavior as it does for the Android back button.

**Test plan**

Manually tested this with the `ModalExample` in the `RNTester` app.  See also the test code in issue #15313.
Closes #15341

Differential Revision: D5651035

Pulled By: shergin

fbshipit-source-id: 54bf66887bbe85940567e63e90b437ac4a8daf9a
@dlowder-salesforce

This comment has been minimized.

Show comment
Hide comment
@dlowder-salesforce

dlowder-salesforce Aug 19, 2017

Collaborator

The fix for this is now merged, so we can close this issue.

Collaborator

dlowder-salesforce commented Aug 19, 2017

The fix for this is now merged, so we can close this issue.

@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot Oct 18, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

stale bot commented Oct 18, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

@stale stale bot added the Stale label Oct 18, 2017

@stale stale bot closed this Oct 25, 2017

@psegalen

This comment has been minimized.

Show comment
Hide comment
@psegalen

psegalen Mar 8, 2018

Hi all, sorry for bringing bad news but it seems that @dlowder-salesforce's onRequestClose gets called on tvOS only for "Debug" scheme, in "Release" it just doesn't fire!

Repo to reproduce: https://github.com/psegalen/rn-tvos-modal-bug
Steps:

  • Get the code, run it in Apple TV simulator with XCode (btw, to get an app running in Release scheme for tvOS you have to modify a react-native init config twice, one to add libReact.a to the test project, another to add missing "Other Linker Flag" -lc++) (Posted a new issue (#18288))
  • If you press the touchable, the modal shows, if you press the remote's menu button, it hides, it's ok but onRequestClose isn't called (you can see that because the state hasn't changed, isModalOpen remains true)
  • If you do the same test after switching the "Run" scheme to "Debug", you'll see that the behaviour is correct since the state is correctly changed (isModalOpen become false after closing the modal)

Can someone check why this is happening please?
My app is behaving as expected when I'm in debug mode but not on an actual device installed via TestFlight. :(

Posted a new issue (#18289)

psegalen commented Mar 8, 2018

Hi all, sorry for bringing bad news but it seems that @dlowder-salesforce's onRequestClose gets called on tvOS only for "Debug" scheme, in "Release" it just doesn't fire!

Repo to reproduce: https://github.com/psegalen/rn-tvos-modal-bug
Steps:

  • Get the code, run it in Apple TV simulator with XCode (btw, to get an app running in Release scheme for tvOS you have to modify a react-native init config twice, one to add libReact.a to the test project, another to add missing "Other Linker Flag" -lc++) (Posted a new issue (#18288))
  • If you press the touchable, the modal shows, if you press the remote's menu button, it hides, it's ok but onRequestClose isn't called (you can see that because the state hasn't changed, isModalOpen remains true)
  • If you do the same test after switching the "Run" scheme to "Debug", you'll see that the behaviour is correct since the state is correctly changed (isModalOpen become false after closing the modal)

Can someone check why this is happening please?
My app is behaving as expected when I'm in debug mode but not on an actual device installed via TestFlight. :(

Posted a new issue (#18289)

@facebook facebook locked and limited conversation to collaborators May 15, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.