joelmukuthu / angular-snapscroll

Vertical scroll-and-snap functionality in angular
http://joelmukuthu.github.io/angular-snapscroll/
MIT License
56 stars 18 forks source link

Scroll window when before-snap returns false #42

Open ziaulrehman40 opened 7 years ago

ziaulrehman40 commented 7 years ago

Hi there,

I am using rails and angular, in there i have used snapscroll for some functionality, there are actually two issues, but both are linked in a way that it seems like single issue.

First issue is that when there are no more elements to scroll, snapscroll is supposed to scroll window than, as demos also have this same functionality, but instead for me snap-scroll just sticks in there and doesn't let window to scroll either.

Second issue is that when before-snap returns false, it should mean that we don't want to snap next item, but instead scroll should let window to scroll because snap-scroll has not used this scroll. So, it should be passed to window to let window scroll.

i am using something like:

%div(snapscroll="true" before-snap="b4snap(snapIndex)" prevent-double-snap-delay="200" snap-index="snapIndex" snap-animation="false")

Is there anything i can do about this? this issue has wasted about a weak, and i hope there is something i can do about it without changing the library or anything. Even if i need to edit the source i will be happy to make a pull-request, just guide me to the exact problem here...

EDIT: There seems to be much js on demo site that might be playing role, specially swipe up and swipe down functions

joelmukuthu commented 7 years ago

Hi Zia,

You're right that the issues are related. Snapscroll takes over the mousewheel events and thus prevents the browser's default scrolling. This happens here and here.

I think the solution will be to set the scrollTop of the element before returning false in your beforeSnap handler (see below for explanation). This will require passing the event object to the handler, which is currently not supported but I can add support for that.

var element = angular.element('#snapscrollElement');
scope.b4Snap = function (snapIndex, $event) {
   $event = $event.originalEvent ? $event.originalEvent : $event;
   element.scrollTop += $event.deltaY;
   return false;
};

You can make the scrolling smoother by using angular-scrollie, which snapscroll also uses to animate the scrolling.

The explanation:

This is how it works currently:

  1. Upon scrolling (with the mousewheel), prevent normal browser scrolling so we can do it with snapscroll.
  2. Calculate the new snapIndex.
  3. Call beforeSnap with the new snapIndex.
  4. If beforeSnap returns false, do not snap.

To fix the issue within snapscroll, at step 4 you would re-enable normal browser scrolling which unfortunately is not possible with the way browser events work. Also, triggering the mousewheel event with something like $(element).trigger(e) will not lead to the scrollTop of the element being updated, and it will also be caught by snapscroll again in step 1.

If we don't prevent normal scrolling at step 1, then it will interfere with snapscroll's scrolling (if beforeSnap will not be returning false). We can't even do step 1 after steps 2 and 3 because the browser scrolling starts so fast that it will end up interfering with snapscroll's scrolling.

joelmukuthu commented 7 years ago

Hi again,

I've released version 1.2.0, with support for:

So you can try the suggestion in my previous message, but with this updated code since keyboard events are also passed to the callback:

scope.b4Snap = function (snapIndex, $event) {
   $event = $event.originalEvent ? $event.originalEvent : $event;
   if ($event.type === 'wheel') {
     element.scrollTop += $event.deltaY;
   }
   return false;
};

Or disable the wheel binding altogether so you can have full control over when to preventDefault(), let the wheel events scroll the element (or the window) or snap the element.

Let me know if this helps.

ziaulrehman40 commented 7 years ago

This is so nice of you. So quick response and an update, hats off to you, i will try these suggestion(which i believe will work) and will update here appropriately.

But i think you should also make release here: https://github.com/joelmukuthu/angular-snapscroll/releases for now i have used tag to download latest build but since your readme points to releases section for latest release download, i suggest you also add a release tag so all users get the latest versions automatically.

joelmukuthu commented 7 years ago

You're welcome :) It's unfortunate that this issue cannot be fixed within snapscroll so I hope the changes will enable you to fix it. All in all let me know how it goes or if there's something I can do to help.

The releases just became too much work to maintain, in addition to maintaining the changelog. But you're right that the link in the readme is a bit misleading since someone might thing the 0.2.5 release is the latest release. I've decided to delete the releases so that github will list the tags by default.

ziaulrehman40 commented 7 years ago

I tried your suggestions and it does work, but this does not solves the first problem, because when all elements have been scrolled, than scrolling does not trigger before-snap, neither window get the scroll event so it can scroll it-self.

So, i think your second option of disabling wheel binding and than controlling snaping manually might be the way to go, that would be great if you can help me with some code for that. With which i will be disabling wheel binding and handling snaping manually, and when i snap, i preventDefaults, and when i dont snap i let the window scroll.

I am trying to get this functionality to work, but there were some issues thats why if you provide with some code for that, that will be great.

Once again, thankyou for your library, and great support for it (Y)

ziaulrehman40 commented 7 years ago

I think this library can be enhanced with a little feature/s, if you got some time for this, i hope these will be very helpful features to add...

The actual problem because of which i wanted window to scroll when i return false from before-snap was just to make sure that the whole element is visible on screen before it starts snapping elements in that div. I think if snapscroll lets us set some attribute to tell snapscroll that it should start snapping only if whole element is visible or this element is on top of the page(meaning it cannot fit in the screen, it is visible as maximum as it can be), otherwise snapscroll should not preventdefault and let the window scroll normally. It can be like snap-if-fully-visible or something like that. (to check if element is fully visible on screen i am using following condition:

      rect = element.getBoundingClientRect()
      isVisible = rect.top >= 0 and rect.left >= 0 and rect.bottom <= (window.innerHeight or document.documentElement.clientHeight) and rect.right <= (window.innerWidth or document.documentElement.clientWidth)

)

Similarly, an option/attribute which if set will cause snapscroll to 'scroll window' when all items in the snapscroll have been scrolled(on either side) and user is still scrolling, in that case should snapscroll let window scroll than or not, option like scroll-window-after-snap-ends or something like that can be added.

PS: Take this comment purely as suggestions. For now, if you just help me with my previous comment that would be more than enough. These suggestions are just to enhance the usability and features in this library.

joelmukuthu commented 7 years ago

Sorry for the delay (I'm currently on holiday).

Here's some sample code (using https://github.com/joelmukuthu/angular-wheelie to bind to the wheel events):

<div id='snapscroll' snapscroll disable-wheel-binding snap-index='snapIndex'></div>
angular.module('myapp', ['snapscroll', 'wheelie']);
angular.controller('Ctrl', ['wheelie', '$scope', function (wheelie, $scope) {
  $scope.snapIndex = 0; // initial index

  var element = angular.element('#snapscroll');

  wheelie.bind(element, {
     up: function (e) {
        if (allowSnapping) {
           e.preventDefault();
           $scope.snapIndex--;
        }
     },
     down: function (e) {
        if (allowSnapping) {
           e.preventDefault();
           $scope.snapIndex++;
        }
     }
  });

  $scope.on('$destroy', function () {
    wheelie.unbind(element);
  });
}]);

I haven't tested it but that should do it, or at least point you towards the right direction.

joelmukuthu commented 7 years ago

Regarding the problem you are trying to solve, snapscroll actually supports having elements that are bigger than the window, but they have to be children elements in the snapscroll element (for example this page of the demo http://joelmukuthu.github.io/angular-snapscroll/#3). So if a child element is bigger than the snapscroll element itself, it will show the rest of the element before snapping to the next child.

As for allowing the window to scroll when the snapscroll element has been scrolled to the end, that's a bit hard to solve. That's because once you preventDefault(), you can't later allow it. With events like click and submit events, you can later dispatch the old event and the browser will do the right thing (i.e. behave as if it was the original click or submit) but with wheel events, if you dispatch the old event (or even create a new one), it doesn't actually scroll the window, which is unfortunate and rather weird (maybe even a bug). Snapscroll makes an attempt at solving this by letting the wheel events propagate upwards so if you had another div wrapping the snapscroll div that listens to wheel events it'll get the wheel event (if the snapscroll is scrolled to the end) but that still doesn't make it scroll.

Sorry if I'm not making much sense here but the conclusion is that snapscroll works best if the whole page is wrapped in a 'snapscroll' element. If you only want a section of the page to be the snapscroll element, then you can add a wheel listener on the parent of that element and you'll get wheel events when the snapscroll has been scrolled to the end. Then with that you can then scroll the window with something like body.scrollTop += wheelEvent.deltaY. FYI, that's how nested snapscroll elements work. When they are scrolled to the end, they let the wheel event propagate to the parent snapscroll element, which then snaps to the next page.

ziaulrehman40 commented 7 years ago

Well, thankyou for your code and time on this issue. I believe your above provided code would work. And I can understand what you are saying, and i believe you can solve this problem soon. I am really busy on some projects, i will try to have a look and make a PR if i get time any time soon. Until than i will use your suggested code. Thankyou for the support and i really appreciate your help.

joelmukuthu commented 7 years ago

You're very welcome! I hope it works out and I'm looking forward to see what you come up with :)

ziaulrehman40 commented 7 years ago

I tried this technique and wheelie was successfully bound to the element but snapping is happening even when the if in up or down of wheelie is false, it still snaps to next element, and does not let window scroll at all. Maybe i am missing something here, i used these options:

snapscroll
disable-wheel-binding
 prevent-snapping-after-manual-scroll
prevent-double-snap-delay="400"
snap-index="snapIndex"
snap-animation="false"

Even tried only these options, and also some other combinations:

snapscroll
disable-wheel-binding
snap-index="snapIndex"

With no success, snapscroll is somehow still acting on wheel events and is not being disabled on wheel events. Please let me know what i might be missing here.

joelmukuthu commented 7 years ago

Perhaps there's a bug with the implementation of disable-wheel-binding, I'll take a look

joelmukuthu commented 7 years ago

I've updated an old fiddle I had lying around (https://jsfiddle.net/w08fbyq3/10/) and it works as I would expect. PTAL and let me know if that's the kind of functionality you need.

ziaulrehman40 commented 7 years ago

Yes, this fiddle is actually transferring the scroll event to window, i tried by adding a wrapper div (

) and after inner scroll finished, it actually scrolled the window. So, do you think is this some problem specific to me? Or is this kind of bug in disbale-wheel-binding?

Also check this fiddle: https://jsfiddle.net/9p1keq6p/3/ try to scroll up and down for a few times, you might notice some strange behaviour/lag. Anyway, i am not going to give-up on this :sunglasses: I will make it work for me, specially after all the help you have provided, only difference in your code and mine(in which i have problem) is that you have set class and id to snapscroll, nothing else seems to be different. I will take another look into my code, until than please have a play with this fiddle, there is yet another issue/bug which is kind of re-producible(at-least in our code) but we will work on that issue in a separate thread after this one is solved.