Skip to content
This repository has been archived by the owner on Dec 19, 2024. It is now read-only.

keep overflow, prevent events that cause scrolling #93

Merged
merged 17 commits into from
Jul 19, 2016
Merged

Conversation

valdrinkoshi
Copy link
Member

@valdrinkoshi valdrinkoshi commented Jul 13, 2016

(started in #64, which was reverted because it introduced a breaking change)

Fixes #54 , fixes #43.

For #54, the goal is to keep the overflow on document.body, and prevent the scrolling from happening if:

  • the scroll happens outside the locked element
  • the scroll happens inside the locked element, but the target is contained in scrollable parents that cannot scroll (because they're at their boundaries already)

For #43, the scroll event listener on document will restore scrolling if it happens because of a user text selection or if the user scrolls through the side scroll bars. If allowScrollOutside, it will call refit.

I've profiled the performance of the change using this file https://gist.github.com/valdrinkoshi/5fb1c67fddd31782508b9e39c8ac25a3, and I'm seeing that overall fps is around 60fps, except some longer frames which go down to 25-30fps, and I believe it is mainly because of the touchmove listener which interrupts the scrolling to execute the javascript (in the timeline I see only the Painting and Rendering activities during this longer frames).

Interactions:
heavy-test-mobile

Longer frame example:
heavy-test-longer-frame

@valdrinkoshi
Copy link
Member Author

FYI for #54, I came up with another way to solve it in #95

@@ -57,7 +64,7 @@

scrollLocked = !!currentLockingElement &&
currentLockingElement !== element &&
!this._composedTreeContains(currentLockingElement, element);
!Polymer.dom(currentLockingElement).deepContains(element);
Copy link
Contributor

@e111077 e111077 Jul 15, 2016

Choose a reason for hiding this comment

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

Does this check the composed tree as well? 🌴

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, let me revert this part as it has nothing to do with the fix ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@valdrinkoshi valdrinkoshi force-pushed the issue-54 branch 2 times, most recently from 982afbf to 0080469 Compare July 18, 2016 18:34
@e111077
Copy link
Contributor

e111077 commented Jul 19, 2016

Testing on nexus 6p and pixel c; CPU usage while scrolling on unscrollable section is very low. Two cores used, and mostly less than half. No frame drops; all idle frames!!

Slower when scrolling in a scrollable section. CPU usage seems to be relatively high, but no higher than the animation of opening the dropdown. No frame drops except when flinging, but that happens on the main page too with no open dropdowns (damn android). I think that the memoization is a success.

@e111077
Copy link
Contributor

e111077 commented Jul 19, 2016

LGTM

@e111077
Copy link
Contributor

e111077 commented Jul 19, 2016

fixes #91

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
6 participants