vaadin / web-components

A set of high-quality standards based web components for enterprise web applications. Part of Vaadin 20+
https://vaadin.com/docs/latest/components
464 stars 83 forks source link

[overlay] Page scrolling doesn't work consistently when the overlay is open as a modal #3822

Open vursen opened 2 years ago

vursen commented 2 years ago

Description

By default, as long as modeless attribute is not set, the overlay opens as a modal. In this state, it adds pointer-events: none to the body element which blocks scrolling for all the scrollable elements except the elements inside the overlay (note, its [part=overlay] has pointer-events: auto) and the body element itself.

A finding from https://github.com/vaadin/web-components/issues/3540

Expected outcome

Either the body element should not be scrollable or other scrollable elements should be scrollable when the overlay is open.

Minimal reproducible example

Snippet 1 (with app-layout):

<style>
  html, body {
    height: 100%;
    min-height: 100%;
  }
</style>

<vaadin-app-layout>
  <div slot="navbar"><h1>Title</h1></div>
  <div style="display: flex; align-items: center; height: 150vh; padding: 10px;">
    <vaadin-context-menu>
      <button>Right click me!</button>
    </vaadin-context-menu>
  </div>
</vaadin-app-layout>

<script type="module">
  import '@vaadin/app-layout';
  import '@vaadin/context-menu';

  const menu = document.querySelector('vaadin-context-menu');
  menu.items = [
    { text: 'Menu Item 1' },
    { text: 'Menu Item 2' },
    { text: 'Menu Item 3' }
  ];
</script>

Snippet 2 (without app-layout):

<head>
  <meta name="viewport" content="width=device-width, initial-scale=1">
</head>

<div style="display: flex; align-items: center; height: 150vh; padding: 10px;">
  <vaadin-context-menu>
    <button>Click me!</button>
  </vaadin-context-menu>
</div>

<script type="module">
  import '@vaadin/context-menu';

  const menu = document.querySelector('vaadin-context-menu');
  menu.items = [
    { text: 'Menu Item 1' },
    { text: 'Menu Item 2' },

  ];
</script>

Steps to reproduce

Snippet 1 (with app-layout):

  1. Add the snippet to an HTML page.
  2. Click the button with the mouse right button to open the context menu
  3. You will see that the app-layout's content cannot be scrolled when the context menu is open.

Snippet 2 (without app-layout):

  1. Add the snippet to an HTML page.
  2. Click the button with the mouse right button to open the context menu
  3. You will see that the page can be scrolled when the context menu is open.

Video

With app-layout:

https://user-images.githubusercontent.com/5039436/167784314-9e5fa854-3506-4ace-ba65-58a347f31f4e.mov

Without app-layout:

https://user-images.githubusercontent.com/5039436/167784206-437be1f9-74ac-44fc-9552-120fd921d999.mov

Environment

Vaadin version(s): 23

Browsers

No response

tomivirkki commented 2 years ago

The modal overlays do make the body element non-scrollable. In the case of the provided example, it's the document element that scrolls.

But it seems the <html> can't be made non-scrollable using pointer-events: none and toggling overflow isn't an option either because it would dynamically change the layout for users that have the scrollbars always visible.

jouni commented 2 years ago

toggling overflow isn't an option either because it would dynamically change the layout for users that have the scrollbars always visible.

https://caniuse.com/mdn-css_properties_scrollbar-gutter

I wonder if this new scrollbar gutter property could be used somehow here.

vursen commented 2 years ago

The scrollbar-gutter property doesn't appear to make the scrollbar space stable when it is only added as long as the overlay is open. If not using scrollbar-gutter, adding overflow: hidden may affect the layout which is not good.

There is also an alternative approach based on adding margin-right when the document is scrollable: https://github.com/elix/elix/blob/ed09212f8b3ec2e8f652e2f318e7c9c9707b709e/src/base/DialogModalityMixin.js#L67-L77

However, based on the internal discussion, we agreed to deprioritize this issue as it was considered not that critical after all.