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

Scroll behaviour not working on body when no explicit height is set #131

Open
rredel opened this issue May 7, 2018 · 16 comments
Open

Scroll behaviour not working on body when no explicit height is set #131

rredel opened this issue May 7, 2018 · 16 comments

Comments

@rredel
Copy link

rredel commented May 7, 2018

For some reason, I can't get the scroll behaviour to work. Even in an absolute minimalist setup like this:
HTML:
<div style="height: 500px;"></div>
<div style="padding: 50px; background: aqua;" draggable="true">drag me</div>
<div style="height: 1500px;"></div>

JS:
import {polyfill} from "mobile-drag-drop";

// optional import of scroll behaviour
import {scrollBehaviourDragImageTranslateOverride} from "mobile-drag-drop/scroll-behaviour";

polyfill( {
// use this to make use of the scroll behaviour
dragImageTranslateOverride: scrollBehaviourDragImageTranslateOverride
} );

window.addEventListener( "touchmove", function() {
// workaround to make scroll prevent work in iOS Safari > 10
} );

The dragging itself works fine though.
How can I get scrolling to work?

Thanks in advance!

@reppners
Copy link
Collaborator

reppners commented May 7, 2018

Welcome and thanks for providing a small example.

I put up a stackblitz with your code and indeed - it's not scrolling 😞
After adding explicit overflow styling for the body element it started working.

You can play with it here https://stackblitz.com/edit/typescript-mobile-drag-drop-playground?file=style.css

Related source code is

function isScrollable( el:HTMLElement ):boolean {
const cs = getComputedStyle( el );
if( el.scrollHeight > el.clientHeight && (cs.overflowY === "scroll" || cs.overflowY === "auto") ) {
return true;
}
if( el.scrollWidth > el.clientWidth && (cs.overflowX === "scroll" || cs.overflowX === "auto") ) {
return true;
}
return false;
}

Maybe there is a better way to determine if an element is scrollable. Any hints and suggestions are greatly appreciated!

@rredel
Copy link
Author

rredel commented May 7, 2018

Unfortunately I can't get your example to work either. Even with the overflow set, which would work for my current needs as a workaround, it is not scrolling on mobile (tested in Chrome on desktop simulated via dev tools and Chrome 66 on Android 7.1.1).

@reppners
Copy link
Collaborator

reppners commented May 7, 2018

bildschirmfoto 2018-05-07 um 20 24 31

Well.. yeah the body element does not report the values I expected it to report. isScrollable check thus is failing.

@rredel
Copy link
Author

rredel commented May 7, 2018

So maybe findScrollableParent() should fall back to document.documentElement in case the detected parent is body. Since I didn't dig that deep into the polyfill code today, I can't tell if that would have any impact on other functionalities.

@reppners
Copy link
Collaborator

reppners commented May 7, 2018

The main issue is that the body element does not have a height restriction which causes it to not really be overflowing. It works when limiting the max-height of the body.

https://stackblitz.com/edit/typescript-mobile-drag-drop-playground?file=style.css

So maybe findScrollableParent() should fall back to document.documentElement in case the detected parent is body. Since I didn't dig that deep into the polyfill code today, I can't tell if that would have any impact on other functionalities.

Yes, there needs to be some special handling for the body to make it "just work".

@rredel
Copy link
Author

rredel commented May 7, 2018

Oh boy, this special treatment of <html> and <body> has driven me nuts so many times, and today I can add another one to the list... :D
As a workaround I tend to set <html> and <body> to height: 100%; and <body> to overflow-x: hidden; overflow-y: auto;
With some CSS trickery, that will also workaround the problem the polyfill causes in my fixed header which includes a relative positioned div with margin auto that keeps jumping to the right when the polyfill kicks in.

@reppners reppners changed the title Scroll behaviour not working Scroll behaviour not working on body when no explicit height is set May 7, 2018
@reppners
Copy link
Collaborator

reppners commented May 7, 2018

I hear you!

The polyfill should enable it to just work so I flag this issue as a bug.

The workaround of setting height to 100% or 100vh comes with a drawback especially on mobile because of dynamic browser navbars that hide only on scroll of document - which can't happen when its limited to viewport size - effectively stealing screen estate from the user.

@rredel
Copy link
Author

rredel commented May 7, 2018

What about something like this in isScrollable():
if(el === document.body && document.body.scrollHeight > document.documentElement.clientHeight && cs.overflowY !== "hidden") { return true;}
Will test that tomorrow. If it works, I'll send you a pull request ;)

@reppners
Copy link
Collaborator

reppners commented May 7, 2018

Maybe we can avoid any special workarounds on body and handle the documentElement differently as it's the root element! I've checked the current implementation and it falls short detecting the documentElement as scrollable.

bildschirmfoto 2018-05-07 um 21 34 52

Basically the documentElement should be detected as scrollable as soon as it's scrollHeight is larger than clientHeight

function findScrollableParent( el:HTMLElement ):HTMLElement {
    do {
        if( !el ) {
            return null;
        }
        if( isScrollable( el ) ) {
            return el;
        }
    } while( el = <HTMLElement>el.parentElement );
    return null;
}
function isScrollable( el:HTMLElement ):boolean {
    
    if( el.scrollHeight > el.clientHeight ) {
      
        if( el === document.documentElement ) {
            return true;
        }
  
        const cs = getComputedStyle( el );
        return cs.overflowY === "scroll" || cs.overflowY === "auto";
    }

    if( el.scrollWidth > el.clientWidth ) {

        if( el === document.documentElement ) {
            return true;
        }

        const cs = getComputedStyle( el );
        return cs.overflowX === "scroll" || cs.overflowX === "auto";
    }

    return false;
}

@rredel
Copy link
Author

rredel commented May 7, 2018

Looks like a neat solution. I'll give it a shot tomorrow and will report the result.

@rredel
Copy link
Author

rredel commented May 8, 2018

I just gave it a test and it works kind of. Scrolling to the top works like a charm but to the bottom doesn't. It seems like it doesn't detect that the edge of the scrollable element is reached and therefore doesn't set scrollIntensions.vertical to 1. I'll do some deeper digging.

@rredel
Copy link
Author

rredel commented May 8, 2018

So, the problem is in getElementViewportSize. I don't know why topLevelElements should be treated differently here. I just replaced the whole content within the function with just:
´return (axis === 0) ? el.clientWidth : el.clientHeight;` and it works like a charm.

@rredel
Copy link
Author

rredel commented May 8, 2018

After some further investigation, my solution above works for scrolling to the buttom. But this also leads to another problem. Since the "ghost" element is being placed via translate3d, it adds up to the scrollHeight of the body when it gets close to the edge which leads to the issue that isScrollEndReached() is always false and it keeps scrolling on and on. I don't have a solid suggestion as of yet on how to resolve this. One solution could be to store the scrollHeight and width of scrollableParentBounds only on dragStart instead of updating it on every move. But that could be an issue for applications that change the DOM via service workers or such (e.g. a chat).
Any ideas?

@rredel
Copy link
Author

rredel commented May 9, 2018

So finally after lots of testing and pulling my hair out on this, I got a solution for this which is working in my minimal example aswell as in the standard demo. Take a look at the last commit in my fork to see the changes.

@reppners
Copy link
Collaborator

Thx for your effort 👍

Can you do a pull request containing your changes?

@rredel
Copy link
Author

rredel commented May 10, 2018

Thank you for the help and the awesome polyfix!
I've created a pull request which you can find here:
#132

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

No branches or pull requests

2 participants