Updated with a new approach:
when the user is in edit mode the modals scrolling is always unlocked.

Hey, sorry for the miscommunication. Here are answers to your questions:

1. A video showing the bug you are fixing

It occurs when the user wants to open a modal in edit mode but decides to continue editing. In that situation, he can't scroll the chat, and he still can't scroll the chat when he discards edit by i.g. pressing the cross button.

2. A detailed explanation explaining how and why the bug is occurring

The bug on video occurs because the text-message.react.js sets the modal status to open before firing the navigate function:

overlayContext.setScrollBlockingModalStatus('open');

A similar situation is in:

• multimedia-message-multimedia.react.js,
• and probably in the robotext-message.react.js, but I haven't tested it.

3. An explanation for how you are choosing to "fix" the bug, and the other options you've considered

Unfortunately, when making the video, I found out that after exiting the edit mode the open modal status remains, and blocks the scrolling.

Solutions I tried:

• current one: appeared it doesn't unblock scrolling after leaving edit mode,
• the previous one - when the user decides to continue editing, we just set the modal status to closed, and unblock the scrolling as expected,
• I tried wrapping the setScrollBlockingModalStatus function, and preventing changing it in the edit state, but it didn't work for 2 reasons:
  • the InputStateContext was not always available here,
  • I can't determine, what should be status of the scrollBlockingModalStatus after leaving edit mode: it depends on what the user chooses to do, if the user opens the modal, we want to leave edit mode with open modal blocking state, otherwise it should be closed.

I believe, that the previous solution was the simplest one, and covered all cases.

It should work in all considered situations:

• the user decides to discard the edit by opening the modal -> scrolling blocked,
• the user decides to continue editing -> scrolling unblocked,
• the user decides to discard the edit by i.g. pressing the cross button (or simply sends the edited message) -> scrolling unblocked.

I still can't tell what makes this case "special", and what justifies this hack. In no other place do we have to manually set closed. Why is this case different? Why can't we continue to rely on the standard mechanism for setting closed?

Requesting review to

In D8271#248477, @ashoat wrote:

I still can't tell what makes this case "special", and what justifies this hack. In no other place do we have to manually set closed? Why is this case different? Why can't we continue to rely on the standard mechanism for setting closed

It is special because the i.g. text-message.react.js:

• sets scroll blocking modal to open before calling the navigate:

overlayContext.setScrollBlockingModalStatus('open');

• we cancel the navigate action, so the modal doesn't open,
• the scroll blocking modal status is normally set to closed after closing the modal,
• closing the alert doesn't trigger rerender -> the getScrollBlockingModalStatus is not called -> standard mechanism doesn't work.

In that case, we don't open the modal at all, so it can't close. But the open status remains.

Thanks for explaining! Your explanation gave me some helpful pointers, but I still had to do some investigation on my own to get the whole story.

I'll try to explain the whole story as to why we need a "hack" here:

1. In some places in the codebase, we preemptively call setScrollBlockingModalStatus('open'). OverlayNavigator would normally handle this via getScrollBlockingModalStatus, but this only gets called when the screen is being displayed, which leaves some time after measurement where the scroll position can change. By preemptively calling setScrollBlockingModalStatus('open') right before we measure, we guarantee that the measure location will be accurate.
2. By the time we call setScrollBlockingModalStatus('open'), we know we are going to display the modal. We know that when we dismiss the modal, OverlayNavigator will set it back to closed via getScrollBlockingModalStatus. So this is normally safe.
3. However, the changes in D8270 complicate this. There we add functionality to OverlayRouter to "cancel" the navigation action when !removeEditMode(action). In that case, there is nothing to undo the preemptive call to setScrollBlockingModalStatus('open'). So now we must handle this ourselves.

I believe, that the previous solution was the simplest one, and covered all cases.

That makes sense. Let's evaluate the previous solution.

1. Your approach of calling setScrollBlockingModalStatus('closed') when the user chooses to cancel the action is a good way to "undo" the preemptive setScrollBlockingModalStatus('open') call.
2. We might be worried that this will happen even if the modal is not in scrollBlockingModals. Before this change, we would never call setScrollBlockingModalStatus for modals that aren't in scrollBlockingModals. Is there a risk now that we are potentially calling setScrollBlockingModalStatus for other modals?
   • We could imagine a scroll blocking modal being displayed, and then a non-scroll blocking modal being displayed over it. If edit mode blocks the non-scrolling blocking modal from displaying, we would not want it to call setScrollBlockingModalStatus('closed'), since that would mess with the scroll blocking modal at the bottom.
   • The naive solution would be to check if the modal is not in scrollBlockingModals. But unfortunately, this is not enough, as we don't ALWAYS preemptively call setScrollBlockingModalStatus('open'): for instance, ThreadSettingsMediaGallery does not do this preemptive call when displaying ImageModal. We could imagine a case where a second scroll-blocking modal is blocked from displaying due to edit mode, but it doesn't do the preemptive call... in that case, our call setScrollBlockingModalStatus('closed') would mess with the first (already-displayed) scroll-blocking modal, which is not what we want.
   • getScrollBlockingModalStatus handles this better by checking the whole stack of modals and looking for any scroll-blocking modal. Setting open is safer than setting closed, since we want to set open if ANY scroll-blocking modal is present, but we only want to set closed if NO scroll-blocking modal is present.
   • Ultimately we could consider ourselves "safe" because of the details on when we expect the edit mode to appear. But this "hack" is still breaking some assumptions regarding the scroll-blocking modal status.

To summarize, the big concern is this one:

Setting open is safer than setting closed, since we want to set open if ANY scroll-blocking modal is present, but we only want to set closed if NO scroll-blocking modal is present.

I'd like to consider another solution: triggering getScrollBlockingModalStatus in this case.

1. getScrollBlockingModalStatus already has the logic to set the scroll-blocking modal status correctly. If it's called when there is no scroll-blocking modal in the stack, it will correctly clear it. But if there is still a scroll-blocking modal, it will not clear it.
   • In other words, calling getScrollBlockingModalStatus can be thought of as "resetting" the scroll-blocking modal status to match the React Navigation state.
   • However, getScrollBlockingModalStatus has the "negative" effect of clearing our preemptive override, so we only call it when something changes (eg. a modal appears or dismisses).
   • But "resetting" the scroll-blocking modal status to match React Navigation state is exactly what we want here!
2. My concrete proposal: can we expose a function called resetScrollBlockingModalStatus() in OverlayContext?
   • This is already where we expose setScrollBlockingModalStatus, so I think it's a good place.
   • We could define resetScrollBlockingModalStatus() as () => setScrollBlockingModalStatus(getScrollBlockingModalStatus(sceneList))
   • Then we can use @kuba's previous solution but call resetScrollBlockingModalStatus() instead of setScrollBlockingModalStatus('closed')

Does that makes sense?

Sorry, for the delay in the response, I had an exam session.

Thanks for the detailed response and for clarifying why setting the modal status to closed was unsafe. That was probably the part I was missing.

Does that makes sense?

Yes, it does. I've updated the diff.

Apologies for the continued churn, but I'm pretty sure this will be the last revision :)