SteffeyDev / react-native-popover-view

A well-tested, adaptable, lightweight <Popover> component for react-native
MIT License
613 stars 92 forks source link

Bugfix/rct keyboard observer bug #84

Closed ugrky closed 4 years ago

ugrky commented 4 years ago

Issue This pull request fixes the following issue: #74 .

Cause of Issue In two distinct places, listeners assigned to keyboardDidShowListener and keyboardDidHideListener are attempted to be removed. These two logic are triggered before unmount right after each other, and defined in following places:

  componentWillUnmount() {
    this._isMounted = false;
    Dimensions.removeEventListener('change', this.handleResizeEvent)
    this.keyboardDidShowListener && this.keyboardDidShowListener.remove();
    this.keyboardDidHideListener && this.keyboardDidHideListener.remove();
}
<BasePopover
  ...
  onCloseStart={() => {
    onCloseStart && onCloseStart();
    this.debug("Tearing down keyboard listeners");
    this.keyboardDidShowListener && this.keyboardDidShowListener.remove();
    this.keyboardDidHideListener && this.keyboardDidHideListener.remove();
    this.keyboardDidShowListener = null;
    this.keyboardDidHideListener = null;
    if (this._isMounted) this.setState({ shiftedDisplayArea: null });
  }}
  ...
/>

In remove logic in componentWillUnmount is triggered before onCloseStart. However, in componentWillUnmount, the listeners are not set back to null, so this.keyboardDidShowListener && check in onCloseStart returns true, and we attempt to remove a non-existing listener.

Fix

Added a similar logic that sets the listeners to null in componentWillUnmount:

  componentWillUnmount() {
    this._isMounted = false;
    Dimensions.removeEventListener('change', this.handleResizeEvent)
    this.keyboardDidShowListener && this.keyboardDidShowListener.remove();
    this.keyboardDidHideListener && this.keyboardDidHideListener.remove();
    this.keyboardDidShowListener = null;
    this.keyboardDidHideListener = null;
}

Additionally, typed the listeners properly:

  private keyboardDidShowListener: EmitterSubscription | null = null;
  private keyboardDidHideListener: EmitterSubscription | null = null;
SteffeyDev commented 4 years ago

I actually had a fix for #74 that I made a while ago, but forgot to push, sorry! Just deployed as 3.0.4. I'll let you test and either close this pull request or amend it if you find it necessary.

ugrky commented 4 years ago

Hey Steffey, would be great if you do the new release. It's a neat code, and super useful! Thanks in advance! 🙂🙂🙂

SteffeyDev commented 4 years ago

I already released, should be up on NPM now

Ajmal0197 commented 2 years ago

Hi, I am getting this issue on going back of screen instantly after closing the popover. Using latest version.

Simulator Screen Shot - iPhone 12 - 2022-09-14 at 14 24 01

const popoverVisibility = bool => {
    InteractionManager.runAfterInteractions(() => {
      setShowPopover(bool);
    });
  };

 <Popover
          placement={PopoverPlacement.BOTTOM}
          isVisible={showPopover}
          onRequestClose={() => popoverVisibility(false)}
          from={
            <TouchableOpacity
              onPress={() => popoverVisibility(true)}
              style={styles.chipWrapper(true)}>
            </TouchableOpacity>
          }>
          <View style={{padding: 8}}>
            {propertyStatusData.map((v, i) => (
              <TouchableOpacity
                onPress={() => onPressPropertyStatus(v)}

              </TouchableOpacity>
            ))}
          </View>
        </Popover>`
SteffeyDev commented 2 years ago

@Ajmal0197 Please open a new issue with the same information, and we can discuss it there