body overflow:scroll issue on new Portal.js HOC Overlay

[joevbruno]

In the new HOC (react-toolbox/components/hoc/Portal.js), the unrenderOverlay method causes a horizontal scrollbar to appear unnecessarily in at least Google Chrome. Furthermore, because it appears as an inline style, it's specificy wins. In the code below, document.body.style.overflow needs to be set to null instead of 'scroll'. I am happy to submit this fix as a pull request. Am I missing something that requires the body style to be set to scroll?

 _unrenderOverlay () {
    if (this._overlayTarget) {
      if (this.props.lockBody) document.body.style.overflow = 'scroll';
      ReactDOM.unmountComponentAtNode(this._overlayTarget);
      this._overlayInstance = null;
    }
  }

[javivelasco]

I missed that, both the x-overflow and the property value. It should be at auto instead of scroll when it's re-enabled. I'll be happy to accept a PR about it and release a patch :)

[dlebedynskyi]

have same issue. fix would be awesome )

[jasonkylefrank]

I work with @joevbruno and have discussed this with him... along with the overflow = 'null' PR that he submitted, we think we know of a way to let users get the behavior that they need.

I checked Joe's PR and although it partially addresses our need (which is for React Toolbox to not mess with our body styles), it has some unwanted side effects such as delayed removal of the scrollbar - making the opening / closing of the Drawer not as smooth as it was (because the space for the vertical scrollbar that appears/disappears on Windows causes the Drawer to "jump" during that entry/exit because of the setTimeout - which is not synched to the Drawer opening/closing).

Therefore, along with his PR, a better option would be to let users "opt-out" of your control messing with the body style... and the great thing is that you already have most of the hooks/infrastructure in place to provide this!

Just provide a prop for Drawer for lockBody and pass that down to Portal. Portal already checks for this prop, but we have no way to pass it in...

Thoughts?

[dlebedynskyi]

@jasonkylefrank idea look good. expect part that you will need to update all components that use overlay/ portal like date time picker.

[javivelasco]

Hi and thanks for your comments! I think that locking the body can be definitively optional but truth is that I don't see it as a desired behavior because in that case the Overlay would be placed there but scrollable what makes not too much sense I think. Of course it's better to provide an option anyway!

To avoid the priority of an inline style we can add a class instead or writing directly the style. I did not tried that yet but I suppose it would work in the same way. We are always free to go back to the previous implementation but this way is easier. If you want to try this change along the day and provide feedback it would be great, I plan to release it this afternoon.

Thanks! :)

[jasonkylefrank]

Hi @javivelasco, thanks for looking into this! So you're planning to provide the lockBody prop to Drawer then (which would get passed down to Overlay and then Portal)?

Regarding your statement:

I don't see it as a desired behavior because in that case the Overlay would be placed there but scrollable what makes not too much sense I think.

When we used the Drawer before version 16 we did not find the Overlay itself to be scrollable (even when the Drawer content itself required scrolling), or show scrollbars on the page. So I may not understand what you mean there. Can you enlighten me?

[javivelasco]

Of course! In previous versions we provided an App element which was wrapping the whole tree structure. This element was absolute positioned over the body and had an scroll overflow meaning that when you used a React Toolbox application the scrolled element never was the body but this app wrapper. It was needed because the Overlay was set as a fixed element taking the whole screen. As it was placed right in front of the container element the scroll was "blocked". Also, components reading position like the Ripple were easier because they didn't need to count on the scroll. Without the wrapper, even adding the fixed element, the body height would still be there and the content would be scrollable of course.

The problem with this, apart the "uglyness" of adding a container for the app was that browsers can't provide the usual animations on scrolling because they are applied over the body. The body never should be styled thought IMO.

With the latest release I wanted to get rid of the App element and still block the body for overlays. The best way I've found to do so is to set overflow hidden to the body. This way if the content was scrolled it would stay at it's position while the scrolling is not possible. Then, at hiding, it would restore the overflow making the body usable again.

I think we can provide a prop for components that use the Overlay and block the body to make it optional. It's just to pass props through as you said. Anyway I think it's not very useful because opening a drawer with a backdrop that does not block the height sounds quite weird. Maybe the best option would be just to add a class to block the body to avoid priority and not overriding body props in case you are using them.

[jasonkylefrank]

@javivelasco It may be that we didn't experience some of the things you are describing because we did not use the React Toolbox App wrapper...

In previous versions we provided an App element which was wrapping the whole tree structure. This element was absolute positioned over the body and had an scroll overflow meaning that when you used a React Toolbox application the scrolled element never was the body but this app wrapper.

But what I think you're describing is that when the Overlay is applied, you want the scrollbars to disappear so that the user cannot scroll the content of the page that is partially visible underneath the Overlay scrim. In that case, I understand the situation, because I actually had to specifically handle that when I was using the Drawer on the right-hand-side because I needed the body scrollbar to disappear so I could introduce a scrollbar for the right-hand-side Drawer and not have the user have "double-scrolling" (one for the body and one for the right-hand-side Drawer).

So it sounds like we want the same general thing. Maybe the main difference is how it is implemented. The way it is handled in v16+ via the setTimeout is probably what is causing the UX to not feel as good as how we did it internally (which removed the scrollbar immediately as the drawer was about to open). See my earlier comments about things "jumping" for what I mean here.

I think you may be able to execute the scrollbar-handling code without a delay though...

I think you could get the scrollbar-handling code to execute immediately by utilizing a new prop called isTransitioningToActive (and you might also need a isTransitioningToInactive):

Then Portal could react to isTransitioningToActive and isTransitioningToInactive in its componentWillReceiveProps. It might look something like:

componentWillReceiveProps (nextProps) {
  if (nextProps.isTransitioningToActive && !this.props.isTransitioningToActive)  {
    // add a css class to <body> which handles removing scrollbar
  } else if (nextProps.isTransitioningToInactive && !this.props.isTransitioningToInactive) {
    // remove the css class from <body>
  }
}

Thoughts?

[javivelasco]

I see the issue now. Portal component removes the lock when it is removed from the document. setTimeout is used in ActivableRenderer to keep a window for the animation before removing the inactive DOM elements. The issue is that this is also delaying the unlocking.

It should be easier than what you propose in my opinion because probably this is a symptom showing that the component responsible from locking the body is not the Portal but the Overlay. In my opinion we should remove the locking from Portal and add this in the Overlay:

  componentDidMount () {
    if (this.props.active) {
      this.escKeyListener = document.body.addEventListener('keydown', this.handleEscKey.bind(this));
      document.body.style.overflow = 'hidden';
    }
  }

  componentWillUpdate (nextProps) {
    if (nextProps.active && !this.props.active) document.body.style.overflow = 'hidden';
    if (!nextProps.active && this.props.active) document.body.style.overflow = null;
  }

What it's weird to me is that without using the App wrapper the overlayed components locked your scroll 😕 Maybe you had styles that allowed to do so. I've tested it and it's working. Maybe you'd want to give a try before publishing

[jasonkylefrank]

@javivelasco Yeah, moving the scrollbar-handling to Overlay might be a good idea. It probably would be simpler, as you say. I'd have to try it out to see if the "jumping" gets corrected.

Regarding:

What it's weird to me is that without using the App wrapper the overlayed components locked your scroll

We were handling the body overflow css ourselves when the drawer opened/closed (i.e., becameactive). We toggled a CSS class on the body element.

Your new proposed solution looks like its worth a try... except that it should toggle a CSS class instead of setting the style inline. 😀 And I'm still in favor of providing a prop so users can opt-out of the CSS class being applied in case they want to handle the transition in a specific way themselves.