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

rredel commented 6 years ago

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 commented 6 years ago

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

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

rredel commented 6 years ago

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 commented 6 years ago

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

rredel commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

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 commented 6 years ago

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

rredel commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Thx for your effort 👍

Can you do a pull request containing your changes?

rredel commented 6 years ago

Thank you for the help and the awesome polyfix! I've created a pull request which you can find here: https://github.com/timruffles/mobile-drag-drop/pull/132

timruffles / mobile-drag-drop

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