Fix __addScrollListeners for polyfilled browsers

[limonte]

The comment above the line I changed says:

Listen for scroll events in all shadowRoots hosting this overlay only when in native ShadowDOM

It's not possible to detect the native ShadowDOM by checking only 'attachShadow' in Element.prototype && 'getRootNode' in Element.prototype:

Firefox	IE11

[limonte]

@valdrinkoshi I would recommend you to set up your editor to auto-trim trailing whitespaces, e.g. the setting for VS Code: https://stackoverflow.com/a/30884298/1331425

For now, it'd be easier to do CR with whitespaces diff disabled: https://github.com/PolymerElements/iron-overlay-behavior/pull/263/files?w=1

[valdrinkoshi]

Hi @limonte thanks for the contribution!

It's not clear what issue the current implementation is creating, mind filing it so we can discuss it? I tried skimming through predixdesignsystem/px-data-grid#331, but couldn't understand the scenario: looks like the problem is that with ShadyDOM we trigger twice the __onCaptureScroll, but not clear to me why is that a problem.

Also, this PR needs a test to avoid future regressions.
I left some comments to the current implementation in the meantime 👌

[valdrinkoshi]

This won't be compatible with polymer 1.x (as it depends on webcomponents v0 which is not implemented with ShadyDOM). This whole check could be replaced with:

if (Polymer.Settings.useShadow) {

[limonte]

Done 👍

[valdrinkoshi]

Instead of having this if wrapping the whole suite, you can use suiteSetup and skip the whole suit:

suite('scroll actions in shadow root', function() {
  suiteSetup(function() {
    if (!Polymer.Settings.useShadow) {
      this.skip();
    }
  });

[limonte]

Done 👍

[valdrinkoshi]

Same as above, you can use suiteSetup

suite('scroll actions in shadow root, overlay distributed', function() {
  suiteSetup(function() {
    if (!Polymer.Settings.useShadow) {
      this.skip();
    }
  });

[limonte]

Done 👍

[limonte]

It's not clear what issue the current implementation is creating, mind filing it so we can discuss it? I tried skimming through predixdesignsystem/px-data-grid#331, but couldn't understand the scenario: looks like the problem is that with ShadyDOM we trigger twice the __onCaptureScroll, but not clear to me why is that a problem.

In px-data-grid component we have px-dropdown which extends iron-dropdown which extends iron-overlay-behavior. The issue was that once px-dropdown is opened in Firefox or IE11 the scrolling in px-data-grid stops working. Here's the gif:

What happens here is basically that scroller component stops firing 'scroll' events. After a lot of digging, I found this issue in checking for native Shadow DOM which fixes the px-data-grid's issue and I guess a lot of scrolling issues.

Also, this PR needs a test to avoid future regressions.

I'm not sure what should I test in this case, could you please help me with the idea how to perform the test?

Thanks a lot for the quick and great feedback!

[valdrinkoshi]

I tried install the version in master and play with it locally:

I noticed that the scroll is not blocked when I scroll with the trackpad (not with the scroll handler, through the pointer). I think that should be locked no matter what, so there seems to be something going on with scroll handling there. It would be helpful to have a smaller repro, e.g. this doesn't happen in the demo page of iron-overlay-behavior https://www.webcomponents.org/element/PolymerElements/iron-overlay-behavior/demo/demo/index.html

[limonte]

I noticed that the scroll is not blocked when I scroll with the trackpad (not with the scroll handler, through the pointer). I think that should be locked no matter what, so there seems to be something going on with scroll handling there.

There's actually no handling there, because there's nothing to handle, 'scroll' events are stolen by iron-overlay-behavior:

[valdrinkoshi]

iron-overlay-behavior doesn't stop scroll events propagation, that seems to be done in vaadin-grid components. I noticed that vaadin-grid-scroll-mixin stops scroll propagation, while vaadin-grid-outer-scroller does restore the scroll position on scroll.

My understanding is that you want <iron-dropdown> to lock scrolling when is vertical, and allow it when it's horizontal, is this correct?

Currently, <iron-dropdown> blocks scrolling. You can either set the allowOutsideScroll=true or scrollAction="refit" to have the <iron-dropdown> refit when scrolls happens, or set the property scrollAction="cancel" to have the dropdown cancel (aka close) on scroll.

For any other custom behavior, consider toggling allowOutsideScroll or scrollAction at appropriate times.

[limonte]

Even after adding allowOutsideScroll to all iron-dropdowns (predixdesignsystem/px-data-grid#635), the issue is still the same.

[valdrinkoshi]

oh! This is a bug of ShadyDOM, where shadowRoot.removeEventListener doesn't actually remove the listener - webcomponents/shadydom#190
See

[valdrinkoshi]

Even when webcomponents/shadydom#190 will be fixed, I still think this PR would make iron-overlay-behavior more performant by avoiding to add scroll event listeners when those are not needed.

I don't think there is an easy way to write a test for this, so I'll merge it as is.

Thanks @limonte again for guiding me through the scenario and your contribution 👍

[limonte]

Thanks for your prompt review @valdrinkoshi!

Can we expect the patch release of iron-overlay-behavior soon so we can close predixdesignsystem/px-data-grid#331?

[valdrinkoshi]

right, 2.3.2 released 👌