Skip to content
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

Focus compose field when modal dismissed #1780

Merged
merged 3 commits into from
Mar 16, 2021

Conversation

Beamanator
Copy link
Contributor

@Beamanator Beamanator commented Mar 15, 2021

Details

Re-implemented compose field being focused when a page-level modal gets dismissed (this broke due to a small regression that was introduced by the recent React-navigation overhaul).

Before the React-Navigation upgrade, closing a page-modal (like /search) would trigger BaseModal's onModalHide callback, which is where we handled a few important things (mainly unsubscribing from keyboard events and setting modal visibility).

After the React-navigation overhaul, when a user navigates away from a page with a modal, BaseModal gets taken out of the DOM before it has time to trigger its onModalHide callback.

To fix, I've added a new function hideModalAndRemoveEventListeners which contains the functions to hide the modal and unregister key event listeners. This function is now called in componentWillUnmount (for page-modals that unmount immediately upon navigation) and in onModalHide (for FAB, which isn't unmounted after its modal hides).

Other notes can be found here: https://github.com/Expensify/Expensify/issues/154180#issuecomment-799676117

Note: Original PR which focused the compose field when closing modals (cc @tgolen): #1699

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/154180

Tests

  1. Go to a chat and start typing a comment
  2. Open up the LHN search
  3. Close the search modal (or select any chat)
  4. Verify that the compose field gets focus

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS (n/a because the field does not get focus by design)
  • Android (n/a because the field does not get focus by design)

Screenshots

Web

Not really any screenshots necessary

Mobile Web

Not really any screenshots necessary

Desktop

Not really any screenshots necessary

iOS

Not really any screenshots necessary

Android

Not really any screenshots necessary

@Beamanator Beamanator requested a review from a team as a code owner March 15, 2021 20:44
@Beamanator Beamanator self-assigned this Mar 15, 2021
@botify botify requested review from timszot and removed request for a team March 15, 2021 20:44
@Beamanator Beamanator changed the title Beaman focus compose when modal dismissed Focus compose when modal dismissed Mar 15, 2021
@Beamanator Beamanator changed the title Focus compose when modal dismissed Focus compose field when modal dismissed Mar 15, 2021
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

Do we need to adjust the onModalHide() callback in the render method below? If those aren't firing at all now, then there is maybe a little more we should do when unmounting the component.

@Beamanator
Copy link
Contributor Author

Beamanator commented Mar 15, 2021

@tgolen yeah I believe onModalHide() is only still being triggered when the FAB is opened & closed

The only things happening here other than setModalVisibility(false) are:

  1. unsubscribeFromKeyEvents (which was already previously added to componentWillUnmount
  2. this.props.onModalHide()
    • onModalHide is only being passed in CreateMenu (which doesn't get unmounted)

So I think we're good to go - but obviously I defer to you :D

@github-actions
Copy link
Contributor

github-actions bot commented Mar 15, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@tgolen
Copy link
Contributor

tgolen commented Mar 15, 2021

OK, thanks. Can unsubscribeFromKeyEvents and setModalVisibility be removed from that callback now?

@Beamanator
Copy link
Contributor Author

Beamanator commented Mar 15, 2021

@tgolen as per our discussion, I've moved the repetitive code into a new function hideModalAndRemoveEventListeners, and called that in componentWillUnmount and onModalHide 👍🏽

I also added the updates in the OP so it remains accurate

Copy link
Contributor

@stitesExpensify stitesExpensify left a comment

Choose a reason for hiding this comment

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

Nice!

@stitesExpensify stitesExpensify merged commit a5e4cf9 into master Mar 16, 2021
@stitesExpensify stitesExpensify deleted the beaman-focusComposeWhenModalDismissed branch March 16, 2021 18:19
@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2021
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.

4 participants