Fullpage.js: setAllowScrolling not work with scrollBar:true

[grimcap]

When scrollBar set to "true" and we try to scroll fixed popUp, background is scroll anyway
setAllowScrolling(false) and other don't help
fullPage 2.9.7

[alvarotrigo]

Thanks for reporting it!
Although support is only provided for the latest fullPage.js version 3.0.2 at the moment.

Here's a reproduction of it using fullPage.js 3:
https://jsfiddle.net/alvarotrigo/ea17skjr/10/

I'll mark it as a bug and it should get fixed for fullpage.js 3.0.3.

[alvarotrigo]

This issue stills open for anyone who want to become a contributor to this project! 👍
I'm giving this way the chance for anyone to contribute to an open source project and practise their Javascript skills.

I propose the solution here. Now you have the chance to work on a few lines and make your contribution!

Proposed solution:

On the mouseWheel and touchMoveHandler, add a condition checking for the up and down values of the isScrollAllowed object.
Based on them, disallow the scroll by preventing the default behaviour and quitting the function.

[dotEthan]

I would love to do this and I think it's definitely in my skill set (I am pretty sure anyway). To be clear, I haven't used github beyond my own repos before, so if I ask stupid things or do stupid things, you'll have to forgive me. ;) I have forked the repo and cloned locally. I have it in front and found both the mousewheelhadler and touchmovehandler functions. I'll look further after supper! Thanks!

[alvarotrigo]

Great! I'll be waiting for it! :)

[dotEthan]

Hey, feeling a bit of an idiot. Where do I put "OPEN-SOURCE-GPLV3-LICENSE" for license?

[alvarotrigo]

@dotEthan don't worry about that. Not required for this issue.

[dotEthan]

Hey, week was a bit busy, but was looking at it all, I was hoping for the license because I can't console.log anything to test what I'm changing (dev-tools says I need a license).

So I need to add an if statement in both mouseWheelHandler and touchMoveHandler looking for both the "up" and (or?) "down" values to be false in the isScrollAllowed object? and if that's true (scroll not allowed) then turn off scrolling (through preventdefault(e)?)?

Or am I under/over thinking things?

[alvarotrigo]

I can't console.log anything to test what I'm changing (dev-tools says I need a license).

You'll get an error message. But you still can use it and output anything you want in the console. As I said, it is not a problem. Just keep playing with it.

So I need to add an if statement in both mouseWheelHandler and touchMoveHandler looking for both the "up" and (or?) "down" values to be false in the isScrollAllowed object?

Yes. Up or down.

and if that's true (scroll not allowed) then turn off scrolling (through preventdefault(e)?)?

Yeap exactly. Also, use return; so it will quit the function there instead of executing the rest of it. (as you'll place the condition on the very top of the function)

[alvarotrigo]

@dotEthan you think we could have this fixed by the end of the week?

Planning to release a new version and would like to do it with this fix.

[meceware]

Hi,

I would like to answer this but I didn't test it thoroughly. However, this should work fine.

Well, we can't remove touch handlers in setAllowScrolling so it should be

        function setAllowScrolling(value, directions){
            if(typeof directions !== 'undefined'){
                ...
            }
            else{
                setIsScrollAllowed(value, 'all', 'm');

                if(value){
                    setMouseWheelScrolling(true);
                    addTouchHandler();
                }else{
                    // setMouseWheelScrolling(false);
                    // removeTouchHandler();
                }
            }
        }

Then add the following inside touchMoveHandler right after isReallyTouch check:

                if (!isScrollAllowed.m.down && !isScrollAllowed.m.up) {
                    console.log("touch event forbidden now");
                    //preventing the easing on iOS devices
                    preventDefault(e);
                    return false;
                }

And almost same code in MouseWheelHandler

            if (!isScrollAllowed.m.down && !isScrollAllowed.m.up) {
                preventDefault(e);
                return false;
            }

Since touch handlers are not removed with setAllowScrolling, on destroy the call should be replaced as

            // setAllowScrolling(false);
            setMouseWheelScrolling(false);
            removeTouchHandler();

The only place left I didn't have time to figure out is the other call of setAllowScrolling in onMouseEnterOrLeave. I'm not sure if it could generate a problem on an specific browser.

I hope I'm not missing something here.

[alvarotrigo]

@meceware yeap looking good! Please feel free to provide the pull request for it and become a contributor! :) (Unless you want to leave some time for @dotEthan to do it instead :) )

[meceware]

@alvarotrigo I think there should be more changes there. Since the handlers are not removed anymore in setAllowScrolling, I think the adding part can be removed too. I didn't have time to write a test for it, so I merely was showing a way to @dotEthan. I'm not 100% sure if it affects anywhere else in the code.

[alvarotrigo]

Since the handlers are not removed anymore in setAllowScrolling, I think the adding part can be removed too.

An easy solution is to only remove then under certain conditions to make sure we won't break anything else:

    }else if(!options.scrollBar && options.autoScrolling){
          setMouseWheelScrolling(false);
          removeTouchHandler();
    }

This way we keep the old functionality but fix the specific scenario.
I'll have to check it with more time on my own, as it might interfere with some fullpage.js extensions. So leaving it this way makes sure it won't break anything.

Also, no need to add the check on the touchMoveHandler.

Then add the following inside touchMoveHandler right after isReallyTouch check:

The touch function already checks for it:

if(isScrollAllowed.m.right){
    moveSlideRight(activeSection); //next
}
...
if(isScrollAllowed.m.left){
    moveSlideLeft(activeSection); //prev
}

As well as the scrolling function to scroll vertically used by moveTouchHandler:

        function scrolling(type){
            if (!isScrollAllowed.m[type]){
                return;
            }

[dotEthan]

Feel free to get @meceware to update, I don't want to slow release or anything and I'm finding I over estimated how simple of a thing it would be. I will work on it, if I can get it working and @meceware hasn't updated it already, I'll post and let you know.

[dotEthan]

Ok, after actually looking back at the code, what you guys were saying became much clearer. I have it working locally with the same setup as the jsFiddle above, when I click "Allow" it scrolls page to page. when I click "Not Allow" it doesn't scroll at all. Also tested on my phone with touch and also stays put when "Not Allow" is clicked.

Before I update my github repo and then look into how exactly to do a pull request, is there any other testing I should do on this before hand (Not sure if there is a procedure or not)?

[dotEthan]

And just to be clear, I just added the

    }else if(!options.scrollBar && options.autoScrolling){
          setMouseWheelScrolling(false);
          removeTouchHandler();
    }

if I'm understanding your discussion above, that seemed to be all that was needed.

[alvarotrigo]

@dotEthan awesome!
Now all you have to do is clone the repository on your side. Use the dev branch and modify the file src/fullpage.js.

Then commit it to the dev branch again.

Regarding the tests, you could actually run the ones in the tests folder. But do not worry about that, I did that myself.

[dotEthan]

Just commit (hopefully) to dev branch (console says dev). Any problems let me know.

[dotEthan]

Oh, wait, do I just commit, or do I commit and then do a pull request?

[alvarotrigo]

You can create a pull request through the website. By going here. Clicking on "New pull request" and choosing your forked branch to compare with the dev branch.
That's probably the easiest way. More info here.

[dotEthan]

Quick question before I do that, just so I'm clear in the order. (thanks for your patience!)

I cloned the dev branch to my local hard drive. made the changes then 'git commit -a -m "message"' in the console. but I haven't pushed because everything keeps saying to only commit to dev branch. Should I push it or just leave it at the commit stage?

I ask because I don't see anything in the "compare" list (on the pull request page) that is my branch. So I'm a bit worried I've done something off.

[alvarotrigo]

I ask because I don't see anything in the "compare" list (on the pull request page) that is my branch. So I'm a bit worried I've done something off.

That's probably because you didn't fork the repository into your own github account. You worked locally on your computer.

But if you prefer to do it your way, you'll just have to make sure to commit it to the dev branch.

You can use git or use any software for it, such as SourceTree or Github Desktop if you do not want to deal with commands and the console.

[dotEthan]

I actually did originally fork it, but then I thought that was not the way to do it, so cloned your repo directly to my local computer. sigh It's been a learning experience anyway.

Anyway, did it on github, yeah, much easier. ;) Thanks

[alvarotrigo]

@dotEthan nothing to panic. You can not break a repository so easily :)

You created the pull request in your own repository as you can see here.

You should have done it in fullPage.js repository instead.
Just merge it to your repository, so your repo has the fix. Then create the pull request from this repository choosing your repository and branch as base to merge into the dev branch.

[dotEthan]

Oh damn.... I think I had it right, then I saw it was listed on my forked repo and thought "Oh no, I did it wrong!" so I canceled it. This has been enlightening. Will redo. hold on.

[dotEthan]

Ok, got confused as to which was base, but done. pull request in your list now. Murphy's Law in full effect here haha Anyway, good learning experience...

[alvarotrigo]

Yeah its a bit hidden on Github. You have to click on "compare across forks" when on the "new pull request" screen.
I noticed you didn't apply the changes on the MouseWheelHandler to prevent the default behaviour as we detailed before on this topic.

I'll keep the conversation on your pull request topic.

[alvarotrigo]

@meceware I ended up going for your suggested approach. Separating the "allow scrolling" behaviour from the "mouseWheel hijack" seemed the right way to go for it.
That was an old peace of code that evolved into a little ugly function :)

Thanks for seeing it!