Avoid scroll bleed when displaying modal UI on mobile #4398

brandonpayton · 2018-01-11T08:02:57Z

Description

This is a PR for avoiding scroll bleed when displaying modal UI on mobile devices. This is meant to address the first part of #4082.

How Has This Been Tested?

I successfully ran the unit tests. I also tried running the e2e tests but had an inconsistent experience with different failures with each run.

I manually tested using an iPhone 6s simulator, an iPhone X simulator, and a Galaxy S5 with Chrome:

Observing that I could scroll the editor body
Opening block insertion modal and observing that I could only scroll the menu, not the body
Closing the block insertion modal and observing I could scroll the body
Opening the sidebar modal and observing that I could scroll the sidebar but not the body.
Closing the sidebar and observing that I could scroll the body
Opening the publish options modal, expanding sections so the modal could scroll, and observing that I could scroll the sidebar but not the body.
Closing the publish options modal and observing that I could scroll the body.

Types of changes

This PR updates Popover to prevent body scrolling while a modal popover is open. It also adds a Popover.MobileScrollLock component so other modal-on-mobile UI like the default and publish options sidebars can declare scroll lock as well. Popover.MobileScrollLock is used in the editor layout to declare scroll lock when sidebars are displayed on mobile.

Scroll locking is accomplished by adding a lockscroll class to the <html> and <body> elements. Unlocking restores the pre-lock scroll position.

This solution is body-specific, so scroll bleed to other ancestor elements is still possible. We can prevent scroll bleed on mobile by preventing the default action on touchmove events, but I'd like to learn more first to be sure we don't break other mobile interactions involving touchmove.

Additional comments

I'd prefer to encapsulate what makes a mobile screen in Popover.MobileScrollLock but did not because that knowledge is currently duplicated between editor modules and components/popover.
Scroll bleed only appears to be an issue in portrait orientation because overflow: hidden is applied to the body at a min-width of 600px.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation.

jasmussen · 2018-01-15T09:20:00Z

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?

brandonpayton · 2018-01-23T07:44:25Z

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?

jasmussen · 2018-01-23T08:30:20Z

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/

brandonpayton · 2018-01-23T19:16:40Z

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.

jasmussen · 2018-01-24T07:34:34Z

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 🏅

brandonpayton · 2018-03-07T19:43:23Z

@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.

brandonpayton · 2018-03-07T19:52:34Z

@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.

brandonpayton · 2018-03-07T20:58:36Z

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.

brandonpayton · 2018-03-08T00:02:21Z

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.

jasmussen · 2018-03-08T08:36:55Z

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!

gziolo · 2018-03-08T08:43:19Z

components/popover/mobile-scroll-lock.js

@@ -0,0 +1,129 @@
+/**


There is a folder with all Higher-Order Components: https://github.com/WordPress/gutenberg/tree/master/components/higher-order. We also try to come up with a pattern for naming such helpers. It seems like in this case withScrollLock would be a good pick. In general, we prefix all HOCs with with in the majority of cases. The only exception is when a component might not be rendered when a given condition occurs. In such case if is the preferred prefix. @aduth might even update our docs somewhere, but I'm sure he commented about it at least twice.

Okey, I now see that it is an independent component. Skip my previous comment in this context :)

gziolo · 2018-03-08T09:46:14Z

edit-post/components/layout/index.js

@@ -112,26 +115,31 @@ function Layout( {
 				openedGeneralSidebar !== null && <GeneralSidebar
 					openedGeneralSidebar={ openedGeneralSidebar } />
 			}
+			<Popover.MobileScrollLock enabled={ layoutHasOpenSidebar && isMobileViewport } />


There is also ifViewportMatch HOC which might be used to wrap this component. In that case, it would never mount when the viewport is larger than small (mobile). It leaves layoutHasOpenSidebar part in here. My question is if we can convert this component into HOC that would be applied to the popover instead? In that case we would naturally combine two related concepts. This MobileScrollLock would be a sibling component that renders only on mobile and when any popover is actually rendered.

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). We can probably get most of the way there though:

Update MobileScrollLock to be enabled if present.

Make users of MobileScrollLock responsible for only adding for mobile viewports. I believe we only use it in two places right now.

Once we can use ifViewportMatch, we can push that requirement into MobileScrollLock.

gziolo · 2018-03-08T09:51:32Z

components/popover/index.js

@@ -331,4 +335,7 @@ Popover.contextTypes = {

 Popover.Slot = () => <Slot bubblesVirtually name={ SLOT_NAME } />;

+// Make this available for use by other modal UI that is not based on Popover
+Popover.MobileScrollLock = MobileScrollLock;


If we want to make it reusable for other use cases it should be exposed as its own component to avoid confusion. I was sure it is specific to the Popover component.

I think you're right and will make the change. The original thinking was that it was ideally only for popover but could be used for Popover-like components.

brandonpayton · 2018-03-08T14:50:17Z

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

brandonpayton · 2018-03-08T17:42:49Z

This has been updated in response to review comments.

gziolo · 2018-03-12T08:08:50Z

components/mobile-scroll-lock/README.md

+	return (
+		<div>
+			Sidebar Content!
+			{ isMobile && <Popover.MobileScrollLock /> }


<Popover.MobileScrollLock /> wasn't updated to reflect the latest changes.

gziolo · 2018-03-12T08:12:45Z

components/mobile-scroll-lock/README.md

+```jsx
+import { MobileScrollLock } from '@wordpress/components';
+
+function Sidebar( { isMobile } ) {


I still think that using HOC would be the cleanest solution because the scroll lock behavior would be applied on top of the component that needs it.

function Sidebar() { return ( <div> Sidebar Content! </div> ); } export default withMobileScrollLock( Sidebar );

gziolo · 2018-03-12T08:20:13Z

components/popover/index.js

-		return <span ref={ this.bindNode( 'anchor' ) }>{ content }</span>;
+		return <span ref={ this.bindNode( 'anchor' ) }>
+			{ content }
+			{ this.state.isMobile && expandOnMobile && <MobileScrollLock /> }


This component would benefit from using this new HOC withViewportMatch, too.

It's still tricky because it depends on another prop. I think we could add additional checks to HOC:

export default withMobileScrollLock( ( props ) => Boolean( props.expandOnMobile ), )( Popover );

This assume isMobile is always handled by the internal check based on withViewportMatch.

gziolo · 2018-03-12T08:22:46Z

edit-post/components/layout/index.js

@@ -112,26 +115,31 @@ function Layout( {
 				openedGeneralSidebar !== null && <GeneralSidebar
 					openedGeneralSidebar={ openedGeneralSidebar } />
 			}
+			{ layoutHasOpenSidebar && isMobileViewport && <MobileScrollLock /> }


When using HOC withMobileScrollLock we wouldn't have to put this logic in here at all, which is quite disconnected from the sidebar component that needs them.

gziolo · 2018-03-12T08:25:34Z

components/mobile-scroll-lock/README.md

@@ -0,0 +1,21 @@
+MobileScrollLock


I think that it can be renamed to ScrollLock as it is no longer dependent on any mobile related logic :)

gziolo · 2018-03-12T08:26:24Z

components/mobile-scroll-lock/index.js

+		--lockCounter;
+	}
+
+	return class MobileScrollLock extends Component {


Yes, it's now a general purpose scroll lock component :)

gziolo · 2018-03-12T08:41:38Z

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 💯

gziolo

See above.

brandonpayton · 2018-03-12T19:24:11Z

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.

gziolo · 2018-03-13T09:35:45Z

components/scroll-lock/test/index.js

+describe( 'scroll-lock', () => {
+	const lockingClassName = 'test-lock-scroll';
+
+	// Use a separate document to reduce the risk of test side-effects.


There is no risk, all test suites are isolated when you use Jest :)

I was very excited by this, but in my testing, it seems like the global document persists between tests and suites.

Interesting, I will check it once in master, out of curiosity :)

I added a class to the body in one test, checked for its presence in beforeEach, and found the class on the body in the second test. Are you up for letting me know if you find different behavior? I'd love to take advantage of isolated environment.

gziolo · 2018-03-13T09:37:59Z

edit-post/components/layout/index.js

@@ -82,6 +84,11 @@ function Layout( {
 			) }
 			{ editorSidebarOpened && <Sidebar /> }
 			{ pluginSidebarOpened && <PluginsPanel /> }
+			{
+				isMobileViewport &&
+				( publishSidebarOpened || editorSidebarOpened || pluginSidebarOpened ) &&


Nit: can we extract inline variable for those 3 flags? We use it twice.

Thanks for catching that. I pushed a fix.

gziolo

This looks great after a few iterations. Thanks for addressing all feedback 👍
I noticed that @jasmussen has tested it already, so I'll assume it work as advertised :)

mtias · 2018-03-14T13:48:19Z

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

afercia · 2018-03-14T15:14:34Z

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.

brandonpayton · 2018-03-14T21:07:50Z

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?

brandonpayton · 2018-03-21T14:04:19Z

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.

brandonpayton force-pushed the add/mobile-scroll-lock-for-modals branch from 00da4b7 to de1fca4 Compare January 13, 2018 04:05

brandonpayton mentioned this pull request Mar 7, 2018

Components: Port Popover to use withViewportMatch #5316

Closed

brandonpayton force-pushed the add/mobile-scroll-lock-for-modals branch 2 times, most recently from d8fae1d to 2bda1c6 Compare March 7, 2018 18:43

jasmussen requested a review from a team March 8, 2018 08:37

gziolo reviewed Mar 8, 2018

View reviewed changes

gziolo reviewed Mar 12, 2018

View reviewed changes

gziolo requested changes Mar 12, 2018

View reviewed changes

brandonpayton force-pushed the add/mobile-scroll-lock-for-modals branch from 732f150 to 444461a Compare March 12, 2018 19:14

gziolo reviewed Mar 13, 2018

View reviewed changes

gziolo approved these changes Mar 13, 2018

View reviewed changes

gziolo added Mobile Web Viewport sizes for mobile and tablet devices [Type] Enhancement A suggestion for improvement. labels Mar 14, 2018

Lock body scroll for modal UI on mobile

044a556

brandonpayton force-pushed the add/mobile-scroll-lock-for-modals branch from 5c3fe3e to 044a556 Compare March 21, 2018 19:00

brandonpayton merged commit a62febf into WordPress:master Mar 21, 2018

Avoid scroll bleed when displaying modal UI on mobile #4398

Avoid scroll bleed when displaying modal UI on mobile #4398

Conversation

brandonpayton commented Jan 11, 2018 • edited Loading

Description

How Has This Been Tested?

Types of changes

Additional comments

Checklist:

jasmussen commented Jan 15, 2018

brandonpayton commented Jan 23, 2018

jasmussen commented Jan 23, 2018

brandonpayton commented Jan 23, 2018 • edited Loading

jasmussen commented Jan 24, 2018

brandonpayton commented Mar 7, 2018 • edited Loading

brandonpayton commented Mar 7, 2018

brandonpayton commented Mar 7, 2018

brandonpayton commented Mar 8, 2018

jasmussen commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonpayton Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonpayton commented Mar 8, 2018 • edited Loading

brandonpayton commented Mar 8, 2018

gziolo Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Mar 12, 2018

gziolo left a comment

Choose a reason for hiding this comment

brandonpayton commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

mtias commented Mar 14, 2018

afercia commented Mar 14, 2018

brandonpayton commented Mar 14, 2018 • edited Loading

brandonpayton commented Mar 21, 2018

brandonpayton commented Jan 11, 2018 •

edited

Loading

brandonpayton commented Jan 23, 2018 •

edited

Loading

brandonpayton commented Mar 7, 2018 •

edited

Loading

brandonpayton Mar 8, 2018 •

edited

Loading

brandonpayton commented Mar 8, 2018 •

edited

Loading

gziolo Mar 12, 2018 •

edited

Loading

gziolo Mar 12, 2018 •

edited

Loading

brandonpayton commented Mar 14, 2018 •

edited

Loading