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

Fix: Modal and screen closing with single esacpe keypress #5889

Merged
merged 5 commits into from
Oct 20, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Oct 15, 2021

Details

Fixed Issues

$ #5769

Tests | QA Steps

  1. Navigate to Workspace settings on the web.
  2. Navigate to members
  3. Check one from the list of members and click on the remove button.
  4. Once the remove modal appears, press the esc button.
  5. Only Confirm modal should close.
  6. Press Esc again.
  7. Now the Settings Modal should close.

Tested On

  • Web
  • Mobile Web [No Escape key]
  • Desktop
  • iOS [No Escape key]
  • Android [No Escape key]

Screenshots

Web | Desktop

output_file.mp4

Mobile Web

Desktop

iOS

Android

@parasharrajat parasharrajat requested a review from a team as a code owner October 15, 2021 15:52
@parasharrajat parasharrajat marked this pull request as draft October 15, 2021 15:53
@MelvinBot MelvinBot requested review from deetergp and removed request for a team October 15, 2021 15:53
@parasharrajat parasharrajat marked this pull request as ready for review October 15, 2021 15:53
@botify botify requested a review from Julesssss October 15, 2021 15:53
@parasharrajat parasharrajat changed the title Fix: Modal and screen closing with single esacpe keypress [N6 HOLD] Fix: Modal and screen closing with single esacpe keypress Oct 15, 2021

/** Details about any modals being used */
modal: PropTypes.shape({
/** Indicates if there is a modal currently visible or not */
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: Your property names are pretty self-describing, which is great, so the doc comment is unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comments are necessary for props based on the styleGuide.

deetergp
deetergp previously approved these changes Oct 15, 2021
Copy link
Contributor

@deetergp deetergp left a comment

Choose a reason for hiding this comment

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

Looks good and tests pass 👍

Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

I'm not sure the exact reproduction steps, but I am still able to trigger the Esc bug occasionally. It's shown at the end of this clip, I close the confirmation dialog a couple of times, then hit escape which closes both the modal and screen.

EscapeStillClosing.mov

@parasharrajat
Copy link
Member Author

Will check...

@parasharrajat
Copy link
Member Author

@Julesssss I have found the issue. Thanks for reporting.
I want to ask you the correct approach.

we set Modal: {isvisible} on Onyx for each type of modal in our app. ScreenWrapper is also behaving like a modal

const modalScreenListeners = {
focus: () => {
setModalVisibility(true);
},
beforeRemove: () => {
setModalVisibility(false);
},
. This is necessary for focus management on the composer.

Now, I need a way to differentiate the Alert modals and Modal screens (they are not really a modal). Apart from this, there is currently no supported way to override the escape handler.

What I am planning is

  1. I will create separate property under ONYXKEYS.MODAL then the state will become like:
Onyx.merge(ONYXKEYS.MODAL, {
 isVisible : Boolean
 isAlertModalVisible: Boolean,
});

To keep the backward compatibility I will set the isVisible:true whenever isAlertModalVisible is true

Do you think this is fine? I don't see any other way right now.

@parasharrajat
Copy link
Member Author

OK, I have pushed the suggested code above. Now there is another issue.

  1. isVisible or isAlertModalVisible takes some time before becoming true. Which creates a short time span between ConfirmModal is shown and the button is pressed. If the user presses Esc in this time span, Screen will close first then the ConfirmModal will be shown and it will be unmounted. It does not look good.

Why?
we set this to true on onModalShow which runs after the animation is finished.

Fix:

Create another key something like isImmediateActive which will be set to true as soon as user-set isVisible to true on the BaseModal and then false after modal is completely hidden(I don't see an application where we should set this flag to true as soon as isVisible on BaseModal goes false. It makes sense to do some operation only after the modal is completely hidden.) So when modal hides isImmediateActive and isVisible will be set to false simultaneously.

@parasharrajat
Copy link
Member Author

@Julesssss Thoughts?

@Julesssss
Copy link
Contributor

Hmm, that's unfortunate. I think you're on the right track with the solution, I just have a few suggestions for naming:

  • To align the Modal Onyx key state, I suggest we change isVisible > isModalVisible or isModalScreenVisible to match isAlertModalVisible
  • isImmediateActive isn't that clear to me, what do you think about willBecomeActive, willBecomeVisibile or isLaunching, to better suggest that the modal is about to be opened?

@parasharrajat
Copy link
Member Author

To align the Modal Onyx key state, I suggest we change isVisible > isModalVisible or isModalScreenVisible to match isAlertModalVisible

I don't think we should do that yet. We need a common prop to keep the app behavior as it is. Otherwise, we will have to use both props where isVisible is used.

isImmediateActive isn't that clear to me, what do you think about willBecomeActive, willBecomeVisibile or isLaunching, to better suggest that the modal is about to be opened?

Really a good suggestion.

Now, I think that we only need willAlertModalBecomeActive. On screenWrapper we have to check if the modal opened is alertModal and we need a flag that is active as soon as the user triggers it and we are sure it will be visible.

So willAlertModalBecomeActive will be enough. and I can skip adding others for now as they are no active usages for those keys.

@Julesssss
Copy link
Contributor

Cool, sounds good. But would you mind explaining why isVisible shouldn't be renamed yet? I only see one usage prior to this PR and it's not obvious to me why this is the case:

Otherwise, we will have to use both props where isVisible is used

* @param {Boolean} isVisible
*/
function willAlertModalBecomeVisible(isVisible) {
Onyx.merge(ONYXKEYS.MODAL, {willAlertModalBecomeVisible: isVisible});
Copy link
Member Author

Choose a reason for hiding this comment

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

OK, @Julesssss I did this change. Now I think we don't need to rename the isVisible as this is still valid for both alert and screenModal context.

@parasharrajat
Copy link
Member Author

This is ready for review @deetergp @Julesssss .

@parasharrajat parasharrajat changed the title [N6 HOLD] Fix: Modal and screen closing with single esacpe keypress Fix: Modal and screen closing with single esacpe keypress Oct 19, 2021

/** Details about any modals being used */
modal: PropTypes.shape({
/** Indicates if there is a Alert modal currently visible or not */
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: The idea here is that we know the modal will be made visible, right? I was going to suggest that we add '...currently visible or about to become visible'. But I'm not sure that it's necessary TBH.

Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

Tests well 👍

@Julesssss Julesssss merged commit 1ff4397 into Expensify:main Oct 20, 2021
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Julesssss in version: 1.1.8-10 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@s77rt
Copy link
Contributor

s77rt commented May 18, 2023

This PR could have prevented this bug #18558. The willAlertModalBecomeVisible call is only made on componentDidUpdate. We should have made this call on componentDidMount as well to cover the cases where the modal is initially rendered visible.

@parasharrajat
Copy link
Member Author

parasharrajat commented May 18, 2023

I guess that was not a use case when this PR was created (2 years back).

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

Successfully merging this pull request may close these issues.

5 participants