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

Avoid scroll bleed when displaying modal UI on mobile #4398

Merged
merged 1 commit into from Mar 21, 2018

Conversation

Projects
None yet
5 participants
@brandonpayton
Member

brandonpayton commented Jan 11, 2018

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:

  1. Observing that I could scroll the editor body
  2. Opening block insertion modal and observing that I could only scroll the menu, not the body
  3. Closing the block insertion modal and observing I could scroll the body
  4. Opening the sidebar modal and observing that I could scroll the sidebar but not the body.
  5. Closing the sidebar and observing that I could scroll the body
  6. Opening the publish options modal, expanding sections so the modal could scroll, and observing that I could scroll the sidebar but not the body.
  7. 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

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Jan 15, 2018

Contributor

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?

Contributor

jasmussen commented Jan 15, 2018

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

This comment has been minimized.

Show comment
Hide comment
@brandonpayton

brandonpayton Jan 23, 2018

Member

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?

Member

brandonpayton commented Jan 23, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Jan 23, 2018

Contributor

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/

Contributor

jasmussen commented Jan 23, 2018

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

This comment has been minimized.

Show comment
Hide comment
@brandonpayton

brandonpayton Jan 23, 2018

Member

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.

Member

brandonpayton commented Jan 23, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Jan 24, 2018

Contributor

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 🏅

Contributor

jasmussen commented Jan 24, 2018

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

This comment has been minimized.

Show comment
Hide comment
@brandonpayton

brandonpayton Mar 7, 2018

Member

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

Member

brandonpayton commented Mar 7, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@brandonpayton

brandonpayton Mar 7, 2018

Member

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

Member

brandonpayton commented Mar 7, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@brandonpayton

brandonpayton Mar 7, 2018

Member

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.

Member

brandonpayton commented Mar 7, 2018

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

This comment has been minimized.

Show comment
Hide comment
@brandonpayton

brandonpayton Mar 8, 2018

Member

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.

Member

brandonpayton commented Mar 8, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Mar 8, 2018

Contributor

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!

Contributor

jasmussen commented Mar 8, 2018

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!

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Mar 8, 2018

@brandonpayton

This comment has been minimized.

Show comment
Hide comment
@brandonpayton

brandonpayton Mar 8, 2018

Member

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

Member

brandonpayton commented Mar 8, 2018

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

@brandonpayton

This comment has been minimized.

Show comment
Hide comment
@brandonpayton

brandonpayton Mar 8, 2018

Member

This has been updated in response to review comments.

Member

brandonpayton commented Mar 8, 2018

This has been updated in response to review comments.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Mar 12, 2018

Member

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 💯

Member

gziolo commented Mar 12, 2018

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

This comment has been minimized.

Show comment
Hide comment
@brandonpayton

brandonpayton Mar 12, 2018

Member

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.

Member

brandonpayton commented Mar 12, 2018

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.

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

This comment has been minimized.

@gziolo

gziolo Mar 13, 2018

Member

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

@gziolo

gziolo Mar 13, 2018

Member

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

This comment has been minimized.

@brandonpayton

brandonpayton Mar 13, 2018

Member

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

@brandonpayton

brandonpayton Mar 13, 2018

Member

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

This comment has been minimized.

@gziolo

gziolo Mar 14, 2018

Member

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

@gziolo

gziolo Mar 14, 2018

Member

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

This comment has been minimized.

@brandonpayton

brandonpayton Mar 14, 2018

Member

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.

@brandonpayton

brandonpayton Mar 14, 2018

Member

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

gziolo approved these changes Mar 13, 2018

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

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Mar 14, 2018

Contributor

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

Contributor

mtias commented Mar 14, 2018

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

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Mar 14, 2018

Contributor

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.

Contributor

afercia commented Mar 14, 2018

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

This comment has been minimized.

Show comment
Hide comment
@brandonpayton

brandonpayton Mar 14, 2018

Member

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?

Member

brandonpayton commented Mar 14, 2018

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

This comment has been minimized.

Show comment
Hide comment
@brandonpayton

brandonpayton Mar 21, 2018

Member

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.

Member

brandonpayton commented Mar 21, 2018

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 brandonpayton merged commit a62febf into WordPress:master Mar 21, 2018

2 checks passed

codecov/project 44.28% (+0.12%) compared to 030b562
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment