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

changed scrollIntoView to target all scrollable parents #55

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dawaltconley
Copy link

@dawaltconley dawaltconley commented Feb 24, 2017

I ran into an issue working with this polyfill's scrollIntoView on a site (this one) that has a body set to height: 100% with all of the page's content existing in a scrollable child div. Any direct children of that div would scroll as expected, but if the target of the scroll was nested within another scrollable div (think the 'hello!' section of your example) the main-page div wouldn't scroll down to it. This was different behavior from the default scrollIntoView method, which would scroll to the element in Firefox.

Changing the if statement in the polyfill to a while loop fixed this for me, and improves how it works in other cases of nested scrollable divs.

For what it's worth, this doesn't seem to to be quite how the default method works. On the modified home page, Firefox will 1) scroll each div until the target element is at the top of its parent---unless the target element is already at the top of it's parent, in which case do the same to the next scrollable parent, etc.---and 2) also scroll the body down so the top of the viewport is aligned with the element. This is almost what the polyfill does, but not quite, as multiple clicks in Firefox will eventually bring the element to where it wants to be, whereas multiple clicks with the polyfill will not.

I feel like the while loop method is the most flexible option, as it works no matter which element is functioning as the page body, and brings the element into view with one click. But I don't know what the word is on having polyfills deviate from the default behavior, even if they end up performing slightly better. It's also is a pretty unlikely scenario where it makes a real difference (who nests multiple scrollable divs?) but thought I'd share this sorta-solution rather than opening an issue. :)

});

el = scrollableParent;
scrollableParent = findScrollableParent(el);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not overriding scrollableParent directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants