Modal | JS | Add page scrolling option

[danielamorse]

Jira

http://vjira2:8080/browse/BDS-1681

Summary

Allow page scroll behind modal to prevent content from shifting.

Details

Until we have a solid solution for content shifting / page scrolling when modal is open, just let the page scroll. The previous work to prevent the page from scrolling is now conditional based on the preventBodyScrolling prop.

Code changes:

• Hide the modal "custom events" page that demos how to set padding on fixed elements when modal opens/closes. Will bring this back later if necessary.
• Move scrollbar functions into Core (per feedback in Modal | JS | Visible shift of content when opening/closing video modal  #1296).
• Move scrollbar calculation functions to BoltModal class, so that they are only called once (per feedback in Modal | JS | Visible shift of content when opening/closing video modal  #1296).
• Add preventBodyScroll as prop, default to false.

How to test

• Run locally in Chrome, Safari, and IE.
• Verify that there is no page content shift when modal opens.
• Verify that the page scrolls behind the modal (when it is long enough).
• Flag any issues.

[sghoweri]

Tested and working as expected. Nice job @danielamorse.

As we chatted about, I'd recommend we make a two small tweaks to this:

1. Move the two helper functions into @bolt/core/utils for easier maintainability
2. Try setting the data we're storing for later on the HTML elements as a property vs data attribute (double check to keep me honest on this one 😉)

// If the browser body has scrollbar, set padding on the element the width of the scrollbar
export const setScrollbarPadding = function(element, hasScrollbar, scrollbarWidth) {
  if (!element) {
    return;
  }

  if (hasScrollbar) {
    const originalPaddingRight = element.style.paddingRight;
    const calculatedPadding = window.getComputedStyle(element)[
      'padding-right'
    ];

    // Save original padding value for later
    element.originalPaddingRight = originalPaddingRight;
    element.style.paddingRight = parseFloat(calculatedPadding) + scrollbarWidth + 'px';
  }
}

// Reset the padding on that element after modal is hidden
export const resetScrollbarPadding = function(element) {
  if (!element) {
    return;
  }

  if (typeof element.originalPaddingRight === 'undefined') {
    element.style.paddingRight = '';
  } else {
    element.style.paddingRight = element.originalPaddingRight; // Restore original padding value
  }
}

3. Make sure our example shows how to account for selecting multiple elements + uses a js- selector

// Arbitrary fixed element, could be .c-page-header
const fixedElements = Array.from(document.querySelectorAll('.js-fixed-position'));

// Listen for 'modal:show' on the body, event will bubble up from the modal
document.body.addEventListener('modal:show', function(e) {
  fixedElements.map(element => {
    setScrollbarPadding(element, e.detail.hasScrollbar, e.detail.scrollbarWidth);
  });
});

// Listen for 'modal:hidden', fires after the modal has animated out
document.body.addEventListener('modal:hidden', function(e) {
  fixedElements.map(element => {
    resetScrollbarPadding(element);
  });
});

[sghoweri]

@danielamorse would no_body_scroll make more sense here?

[sghoweri]

Make sure to remove this ;)

[sghoweri]

👏 we need to start doing this for all net-new JS coming in. Nice!!

[sghoweri]

+1 for commenting on this

[sghoweri]

Nice work @danielamorse!

Make sure to nix that one modal-specific class on the new utility function + rename the new modal body scroll prop if you like the proposed name change. Other than that, this looks good to go. 👍

[remydenton]

Sort of a nitpick, but can we make the default value (false) explicit the schema?

[remydenton]

Although, now that I think about it, do we ultimately want it to actually default to true? That's the ideal UX after all once we can get it working...

I suppose the way you have it is the path of least resistance though. Don't let me derail it.

[danielamorse]

@sghoweri :

Make sure to nix that one modal-specific class on the new utility function

Done.

@remydenton @sghoweri:

Although, now that I think about it, do we ultimately want it to actually default to true? That's the ideal UX after all once we can get it working...

You both mentioned this point, and I say, if we're going to be strict about breaking changes, the default should be true. Even better, let's not make it a prop until there's actually a need for it. The main use of the prop at this point is just for local development of the future 'no-body-scroll' feature. In light of that, I changed the hidden but publicly available noBodyScroll prop to a private variable (see commit). Everything else works the same. You all ok with that decision?

[sghoweri]

I’m good with this. @remydenton, any thoughts?

[remydenton]

Yes! I think that's an excellent choice @danielamorse. The behavior may change in the future (e.g. the background will stop scrolling), but that's not a breaking change if we never had a prop that would let you pick one or the other. And frankly, I don't think you should be able to pick one or the other-- we know which one's the better UX.