Skip to content
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

fix(modals): prevent html overflow #894

Merged
merged 3 commits into from
Oct 9, 2020
Merged

fix(modals): prevent html overflow #894

merged 3 commits into from
Oct 9, 2020

Conversation

hzhu
Copy link
Contributor

@hzhu hzhu commented Oct 8, 2020

Description

When Bedrock CSS is turned on and the Modal and DrawerModal components are opened the main window should be inert, but the user can scroll the main window.

Steps to reproduce:

  1. Navigate to https://zendeskgarden.github.io/react-components/modals/
  2. Turn on Bedrock CSS
  3. Open a Modal or DrawerModal
  4. Trigger some scroll events; the main window behind the backdrop scrolls, but it shouldn't

Detail

The Bedrock CSS that normalizes CSS in the Garden ecosystem applies an overflow-y:scroll; to the html element. In some cases like modal components, the overflow-y:scroll; should be overwritten so that inert windows behind an open modal dialog cannot be scrolled.

This PR proposes to modify overflow CSS for the html element instead of the body element. By doing so, the overflow from Bedrock CSS is correctly overwritten when a modal is opened.

Screenshots

Before: when Bedrock CSS is turned on, the user can scroll an inert window behind open modals
2020-10-08 12 40 22


After: when Bedrock CSS is turned on, the user cannot scroll an inert window behind open modals
2020-10-08 12 41 42

Checklist

  • 🌐 Styleguidist demo is up-to-date (yarn start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 🀘 renders as expected with Bedrock CSS (?bedrock)
  • β™Ώ analyzed via axe and evaluated using VoiceOver
  • πŸ’‚β€β™‚οΈ includes new unit tests
  • πŸ“ tested in Chrome, Firefox, Safari, Edge, and IE11

@hzhu hzhu self-assigned this Oct 8, 2020
@zendesk-garden zendesk-garden temporarily deployed to staging October 8, 2020 20:27 Inactive
@hzhu hzhu force-pushed the hzhu/modal-bedrock-scroll branch 2 times, most recently from fcb063c to 7fef3f2 Compare October 8, 2020 21:03
@hzhu hzhu force-pushed the hzhu/modal-bedrock-scroll branch from 7fef3f2 to f2d3b64 Compare October 8, 2020 21:59
@hzhu hzhu marked this pull request as ready for review October 8, 2020 22:18
@hzhu hzhu requested a review from a team as a code owner October 8, 2020 22:18
@zendesk-garden zendesk-garden temporarily deployed to staging October 8, 2020 22:37 Inactive
}

if (htmlElement) {
previousHtmlOverflowY = htmlElement.style.overflowY;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overflow needs to be hidden on both axes, not just y.

@@ -138,17 +141,32 @@ export const Modal = React.forwardRef<HTMLDivElement, IModalProps>(
bodyElement.style.paddingRight = `${bodyPaddingRight + scrollbarSize}px`;
}

const previousBodyOverflow = bodyElement.style.overflow;
previousBodyOverflow = bodyElement.style.overflow;

bodyElement.style.overflow = 'hidden';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is body overflow management being retained?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, it doesn't and the solution can be simplified a lot. Thanks!

@zendesk-garden zendesk-garden temporarily deployed to staging October 9, 2020 00:43 Inactive
@hzhu hzhu requested review from jzempel and a team October 9, 2020 16:07
Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This maps to how the 1.0 version of modals was developed:

  componentDidUpdate(prevProps) {
    const { hidden } = this.props;
    const { hidden: prevHidden } = prevProps;


    if (!hidden && prevHidden) {
      document.querySelector("html").style.overflow = "hidden";
    } else if (hidden && !prevHidden) {
      document.querySelector("html").style.overflow = "";
      this.tabJail = null;
    }
  }

...not sure why it was changed in version 2.0.

Copy link
Contributor

@austingreendev austingreendev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jzempel I think the original html -> body overflow change was due to iframed areas that were using Modal. Before we allowed document to be provided by the ThemeProvider this was a bigger issue.

@zendesk-garden zendesk-garden temporarily deployed to staging October 9, 2020 19:07 Inactive
@hzhu hzhu merged commit d692abd into main Oct 9, 2020
@hzhu hzhu deleted the hzhu/modal-bedrock-scroll branch October 9, 2020 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants