Nice work, thanks for working on this!

🚨 This is currently a non-starter because setting overflow: hidden on the body resets the scroll position to zero. It's surprising when you're scrolled halfway down a post, open and close the block inserter, and find you're now scrolled to the top of the post. I am considering a solution for this.

Is this actually the case, though? I know the following is just testing in the inspector, but for me it works fine to set overflow: hidden; on the body element:

https://cloudup.com/cYbErRWbcOC

In my experience, the only thing that resets the scroll position is if for whatever reason the main scrolling canvas changes height. Is it when it lands on a real device that it stops working?

Hi @jasmussen, I'm sorry for the delay in response. Here is what I'm seeing when I the lockscroll class to the body and html elements.
https://cloudup.com/cb-hZYMuoaw

The scroll unfortunately resets.

I am thinking saving and restoring the body's scrollTop is a reasonable workaround, but it might interfere with other logic that does things like scroll elements into view. An example of that is the url-input block.

One idea is to lock and unlock scroll in componentWillUpdate since things like scroll-into-view likely need to wait for componentDidUpdate. This is getting too clever IMO, but I don't currently have a cleaner idea for working around this issue.

What do you think?

It seems I was mistaken indeed. On Android and in the Chrome simulator, applying overflow: hidden; only to the body element achieves the desired effect, however you're right, it has to be applied also to the html element for this to work on iOS. And incidentally the latter is what resets the scroll position. That's unfortunate.

It does seem like, perhaps, your approach is what we'll have to do — store the scroll position and then restore it on modal-close. This one seemed promising: https://benfrain.com/preventing-body-scroll-for-modals-in-ios/

Regarding your link, I love the idea of preventing the default action for touchmove, especially because we'd only need to worry about touches that start within the modal and we wouldn't have to meddle with the scroll.

I've wondered whether it would be too clever though. I am not yet very familiar with touch events. Here are a couple of questions I need to answer:

• Are touchmove events emitted when dragging an element? If so, we could interfere with drag and drop.
• Is it possible to scroll diagonally on mobile? If so, we can interfere with horizontal scrolling if we prevent default on a diagonal scroll because it would cause vertical scroll bleed.

In the meantime, I'm thinking it probably makes sense to save and restore body scroll position.

Thanks for keeping on this Brandon.

Just to clarify — you should decide, based on your knowledge of this, what is the best programmatic approach to solving this problem. While I might sometimes suggest something to explore, it's merely a casual suggestion, and being mostly a designer I'd always defer to you and others on the code implementation. So go forth with what you feel will work best 🏅

@jasmussen I think this is ready for consideration.

I'm not 100% comfortable with saving and restoring scroll position because I'm afraid it will interfere with other components that scroll-into-view, but in my testing, scrolling after inserting a block is working as I'd expect. But if we land #5316, I believe I can update Popover.MobileScrollLock similarly and restore scroll at an earlier point in the React lifecycle to reduce the likelihood of this issue.

@jasmussen I take that back. I had previously tested on an Android device but am seeing issues restoring scroll position on the latest. It seems to restore but not actually update the display until I interact with something. I'll reply again once I know more.

I committed a fix. I thought this was working previously but don't see how. The scroll position needed to be restored on document.scrollingElement. It appears to be the same as document.body on iOS, at least in the editor, but it is the same as document.documentElement on Android.

I noticed I don't have a test for restoring scroll position, so I'm adding one.

I am not sure how to reasonably test restoring scroll position, but I may be able to add an e2e test that resizes the window to a mobile viewport width to trigger scroll locking.

It might be worth landing this as-is though.

In my testing, this is absolutely working, and working really really well. Because it's so good, I strongly agree it would be good to get in asap. Adding a quick code review but experience wise this seems great. Thank you!

Thanks for testing, @jasmussen, and for the review, @gziolo!

This has been updated in response to review comments.

Thanks adding your changes to the PR 👍

I would love to use ifViewportMatch here, but my understanding is that it depends on resolving a circular dependency between @wordpress/components and @wordpress/viewport (see #5316).

Yes, this needs to be resolved separately first. Then we would be able to refactor what we have in this PR and introduce withMobileScrollLock HOC, which could look like:

const withMobileScrollLock = ( predicate) => ( WrappedComponent ) => { 
    const MobileScrollLock = compose(
        ifViewportMatches( '< small' ),
        ifCondition( predicate ),
    )( ScrollLock );
    const EnhancedComponent = ( props ) => (
        <Fragment>
            <MobileScrollLock { ...props } />
            <WrappedComponent { ...props } />
        </Fragment>
    );

    EnhancedComponent.displayName = getWrapperDisplayName( WrappedComponent, 'withMobileScrollLock' );

    return EnhancedComponent;
};

I don't want to block this PR until we find a viable solution for ifViewportMatch. You can skip temporarily my comments related to withMobileScrollLock HOC. Let's change the name MobileScrollLock to ScrollLock and update all corresponding docs and we should be good to go 💯

Hi @gziolo, I believe I've address all your review comments.

I like your idea of a withMobileScrollLock HOC. Thanks for taking time to talk about it and work through how it would be applied to various <ScrollLock> consumers.

Let's get this for 2.5 to make sure there are no unintended issues.

Scroll locking is accomplished by adding a lockscroll class to the and elements.

Just to note that core uses a CSS class modal-open for this, and only on the body.

Thanks, that's good to know, @afercia. In my testing, it was necessary to also add the class to the document element effectively prevent scroll bleed on iOS devices. It's possible there is something I missed. Any thoughts here?

I need to rebase this and relocate the rules to component CSS. I'm planning to do that this morning. Then this should be ready to go.