WICG / virtual-scroller

Other
2k stars 83 forks source link

Add demo: list with dismissable items #96

Closed kenchris closed 6 years ago

kenchris commented 6 years ago

Add an example of how you can create a list with dismissable/deleteable items.

kenchris commented 6 years ago

Video of how it works: https://www.youtube.com/watch?v=NLkt1t2gWNI

kenchris commented 6 years ago

Lots of great comments, thank you! I will have a look at them all tomorrow morning.

would be nice to get some details/comments on how these parts work together.

Yes, I am still trying to find out the best and most seamless way to make it all work together and simplify it, but I thought it was better to upload my first fully working implementation that I was reasonable satisfied with :-)

How's the behavior on the actual device?

I tested an earlier version yesterday night and it worked fine on my Pixel 2. I tried to match the feeling from other native Android apps.

Do you think we can enable these gestures also on desktop through mouseup/down/move? Maybe not worth it?

It shouldn't be that hard to implement, and I could potentially use pointer events. Currently it is not a very common user interaction model on desktop, but many desktop apps need to support both keyboard and touch (Windows 10, Pixelbook etc) so that could of course change over time, as users become used to the pattern.

kenchris commented 6 years ago

OK, fixed the review comments and it looks much nicer. I don't have an iPhone so couldn't test on iOS, but Web Animations is behind a flag there so either a polyfill is needed or I need to just animate using CSS.

I did try that, but for some reason it doesn't animate to the end and I never got the transitionend event. It is also quite annoying as I only want animations in one direction (collapse) so I need to add and remove CSS classes to remove the transitions - so in that sense Web Animations is much nicer.

kenchris commented 6 years ago

I didn't include my changes to the deployment script, because it doesn't work by default for me - I am on Linux so it might be due to sed differences between Linux and Mac

My changes are:

+mv node_modules/web-animations-js components/
 # replace node_modules/ with components/
-find node_modules/$repo -name '*.js' -exec sed -i '' 's/node_modules\//components\//g' {} \;
+find node_modules/$repo -name '*.js' -exec sed -i 's/node_modules\//components\//g' {} \;
+find -name '*.html' -exec sed -i 's/node_modules\//components\//g' {} \;

I have it deployed here: https://kenchris.github.io/virtual-scroller/demo/dismissable/

kenchris commented 6 years ago

So moving to pointer events, made the list not scroll on mobile as it relies on regular touch events. Maybe we could consider moving to pointer events for the list and that would solve this issue.

The reason is that in order to use pointer events for touch, you must set touch-action: none to block regular touch events, and regular touch events cancels touch based pointer events :-(

My last patch uses only pointer events for mouse.

domenic commented 6 years ago

So moving to pointer events, made the list not scroll on mobile as it relies on regular touch events. Maybe we could consider moving to pointer events for the list and that would solve this issue.

This is interesting, and might be worth filing as a separate issue. I hadn't realized we were using touch events...

(But to be clear, we shouldn't necessarily block on figuring this out before landing this cool demo.)

valdrinkoshi commented 6 years ago

The virtual-scroller implementation doesn't directly rely on these events to scroll, but the scrolling on mobile gets prevented because the demo calls preventDefault() on these events. If you do that on touchstart or touchmove, you won't receive next events as you'd expect. Is the demo invoking e.preventDefault() to disable vertical scrolling while the user swipes horizontally?

kenchris commented 6 years ago

Actually, the current code works fine.

It wasn't working when I just used pointer events, because touch events cancel pointer events (ie. pointercancel gets called after like 5-10 pointermove events). The only way to avoid that, as far as I researched, is to set touch-action: none, but that means that no touch events are forwarded to virtual-scroller, and only pointer move events, which virtual-scroller doesn't seem to handle.

kenchris commented 6 years ago

From: https://developer.mozilla.org/en-US/docs/Web/CSS/touch-action

An application using Pointer_events will receive a pointercancel event when the browser starts handling a touch gesture.

This seems to be what I am running into.

Adding touch-action: none as the previous commit, makes the dismissal code work fine, but it makes the list unscrollable.

kenchris commented 6 years ago

@valdrinkoshi Did you want to fix the deploy script yourself? I need to change node_modules to components in the .html file and the first '' in the sed command makes the sed fail for me on Chrome OS (Termina VM Container)

valdrinkoshi commented 6 years ago

Uhm, I don't think we need the polyfill for Web Animations, as the demo won't work in iOs anyways, as VirtualScroller depends on ResizeObserver. I opened this issue to track what features we're using https://github.com/valdrinkoshi/virtual-scroller/issues/100

kenchris commented 6 years ago

OK, I can remove it...

OK, I removed it and pushed without that commit

kenchris commented 6 years ago

Putting here as GitHub collapses old comments:

Actually if the list starts scrolling, that should cancel the dismissal handling.

If I start a drag horizontally, then I cannot scroll until I lift my finger/mouse.. even if I pan my finger all the way down vertically and then to the right. That works like Android - just try panning away one of the notifications on Android - press, move a bit left or right, then while finger down, move all the way down and then you can still dismiss by moving left and right.

I don't want to break that behavior. So basically just knowing when the list is being scrolled and then dismiss would be fine!

valdrinkoshi commented 6 years ago

Agreed on the design, I think adding the state = 'cancel' in the right spot should fix the last issue you mentioned \o/

kenchris commented 6 years ago

Interesting observation: You can dismiss multiple contacts using multiple fingers. It actually seem to work, but I see some jank.

I checked Android and apps only seem to allow one dismissable item at the time. In Google Inbox... multiple touch points means that the dismissal gets ignored outright.

For notifications, though, one of the fingers will be used, so you can dismiss one item - Google Tasks and GMail works the same way. - that seems to be the default.

UPDATE: Actually, with the e.preventDefault() - the multi dismissal isn't working and no item is being dismissed (ie. it works like Google Inbox on Android, and not like most Android apps)

valdrinkoshi commented 6 years ago

I've played with this a bit, sent a PR on your repo https://github.com/kenchris/virtual-scroller/pull/1 I think some of the problems might have been related to not resetting _tracker when gesture finished. anyways, on my local tests i don't face the issues you mentioned

kenchris commented 6 years ago

I can still trigger the problem though... just move more in x direction than y:

https://drive.google.com/file/d/1UVg8LCaqQ93vHmGm57PCeEq72KgEG2Zu/view

The DevTools shows that it is running the newest code

Only the preventDefaults seems to solve this for me. May I ask why you don't like it? It is only being called in the panning code.

kenchris commented 6 years ago

Yay, thanks for merging \o/

valdrinkoshi commented 6 years ago

Interesting...i see in your console that content-script.js is causing a TypeError right before you see that problem...that error in particular seems to be doing something with ShadowRoots - should be some extension you have active. Can you try it on a clean chrome profile, or by disabling extensions?

kenchris commented 6 years ago

Yes, that is when I use the recording extension... that is also the one that breaks the overflow-x:hidden.

I can easily reproduce with that extension disabled - but then I just cannot record on Chrome OS :-)

kenchris commented 6 years ago

What I do is: Start a mini scroll vertically, then do a mini swipe up-right... that is more to the right than up.

As you see with the below image, I am scrolling (the scroll indicator is visible)

image

Same in incognito (no extensions);

image

kenchris commented 6 years ago

When I am in dragging/panning mode, I really don't want anyone else acting on the same events, to do scrolling and whatnot. I tried event.stopPropagation() but only event.preventDefault() is working for that.

kenchris commented 6 years ago

I guess that it is not the worst in the world that you can scroll while panning, if it wasn't for the fact that the draggable state switches between 0 and the drag position and the virtual list resets the transform during scroll.

valdrinkoshi commented 6 years ago

oh...what about updating the transform of the distributed <content-element> instead of the <dismissable-item>? That way we could scope all the things within <dismissable-item>, also the overflow-x: hidden

kenchris commented 6 years ago

hmm, that is one option. But then I could only have one child or would need to apply transform to them all, or is there a smarter way. I could add the overflow-x: hidden inside the shadow root.

kenchris commented 6 years ago

I really don't know how this is happening.. for a second I though it was the deltaX ** 2 + deltaY ** 2 < kTouchSlopValue ** 2 code that allow a scroll to start, but even removing that and I can still trigger it

valdrinkoshi commented 6 years ago

Uhm, if we want to support multiple children, then the shadowRoot of <dismissable-item> could look like this:

<style>
  #contentWrapper {
    overflow-x: hidden;
   }
</style>
<div id="contentWrapper">
  <slot></slot>
</div>

And you could then translate #contentWrapper.

Also, probably the default value for state should not be initial but null or something else, otherwise all items will do things on their touchmove (even if they didn't have a touchstart) :x

valdrinkoshi commented 6 years ago

Oh forgot to answer to your question

May I ask why you don't like it? It is only being called in the panning code.

In general, I'm afraid of preventDefault() on scroll-related events, as it might prevent the browser from performing smooth scrolls. e.g. chrome made certain event listeners passive by default to improve scroll performance on mobile https://developers.google.com/web/tools/lighthouse/audits/passive-event-listeners

In this case, I'd like to understand what is causing that scroll that we're trying to prevent, and see if we can get around it with css. If we can do that, the next step would be to just listen for those events with {passive: true} \~o~

kenchris commented 6 years ago

OK that works nicer: https://github.com/valdrinkoshi/virtual-scroller/pull/107

kenchris commented 6 years ago

Yes, I know about the passive listeners and the interventions. cc @rbyers

Here in this case we don't want a scroll because we are in panning/draggable mode - so yes, we are preventing a scroll here. Also, this preventDefault() is only called while panning/dragging.

I assume that wouldn't affect any future scroll, for instance started with a touch flick (touchstart, touchmove, touchend without any call to preventDetault(). Maybe Rick can enlighten us here :-)

Ah, getting late here, I think I understand you now. You want to add {passive: true} to these events, which could make preventDefault useless as it is ignored. Got ya!

valdrinkoshi commented 6 years ago

Thanks for the hard work @kenchris - this little demo is becoming a great custom element for swiping stuff away \o/

kenchris commented 6 years ago

Indeed :-) I really want our nice web platform to have these nice primitives available, working well and easy to use and re-use.

I still hope we can find and fix this last bug in a nice manner though, but I first need to figure out what is going on.