ilyashubin / scrollbooster

Enjoyable content drag-to-scroll library
https://ilyashubin.github.io/scrollbooster
MIT License
986 stars 81 forks source link

'isDragging' is incorrectly read as true when querying via 'getState()' #54

Open emmettdn opened 4 years ago

emmettdn commented 4 years ago

Hi, first of all thanks for the great library.

I'm currently having an issue where isDragging state is set to true but shouldn't be. This is reproducible when completing a mouse drag in the scrollable area then querying the state without clicking in the scrollable area again.

It seems to be to do with how we determine isDragging state in getState().

isDragging: !!(this.dragOffset.x || this.dragOffset.y),

Instead of using this.isDragging which is set to false on mouseup it relys on dragOffset which doesn't seem to be reset until another pointer event occurs.

Couldn't find any other issues relating to this so I am wondering if I am missing something?

Thanks in advance :)

mlshv commented 3 years ago

I have the same problem! This issue definitely needs a fix

Upd Just looked into the code and it seems like I have a problem of another kind but it looks similar. I'm trying to use drag-and-drop handler together with scrollbooster. I have shouldScroll function defined like this:

new ScrollBooster({
    viewport: containerRef.current,
    content: contentRef.current,
    scrollMode: 'native',
    direction: 'horizontal',
    textSelection: true,
    shouldScroll: (state, event) => {
        const isDragHandleToched = event.path.some(checkIsDragHandle)

        return !isDragHandleToched
    },
})

But it disables click handlers on a draggable element after a first scroll move with scrollbooster. And state.isDragging is always true on single click because of this line in scrollboster:

this.events.pointerdown = (event) => {
     // ....
     this.isDragging = true
}

I'm going to investigate further

Upd2 I think I've found the problem:

this.events.click = (event) => {
    const state = this.getState()
    const dragOffsetX = this.props.direction !== 'vertical' ? state.dragOffset.x : 0
    const dragOffsetY = this.props.direction !== 'horizontal' ? state.dragOffset.y : 0
    if (Math.max(Math.abs(dragOffsetX), Math.abs(dragOffsetY)) > CLICK_EVENT_THRESHOLD_PX) {
        // this is always true if you have scrolled to any amount of pixels more than 5
        // ...even if this.isDragging equals false
        event.preventDefault()
        event.stopPropagation()
    }
    this.props.onClick(state, event, isTouch)
}

My solution is to add this.isDragging to the if statement:

    if (this.isDragging && Math.max(Math.abs(dragOffsetX), Math.abs(dragOffsetY)) > CLICK_EVENT_THRESHOLD_PX) {
        event.preventDefault()
        event.stopPropagation()
    }

I didn't dig in enough to say it should be patched on the main branch or if it going to work in your case, but it worked for me

asmedberg commented 2 years ago

Upd2 I think I've found the problem:

this.events.click = (event) => {
    const state = this.getState()
    const dragOffsetX = this.props.direction !== 'vertical' ? state.dragOffset.x : 0
    const dragOffsetY = this.props.direction !== 'horizontal' ? state.dragOffset.y : 0
    if (Math.max(Math.abs(dragOffsetX), Math.abs(dragOffsetY)) > CLICK_EVENT_THRESHOLD_PX) {
        // this is always true if you have scrolled to any amount of pixels more than 5
        // ...even if this.isDragging equals false
        event.preventDefault()
        event.stopPropagation()
    }
    this.props.onClick(state, event, isTouch)
}

My solution is to add this.isDragging to the if statement:

    if (this.isDragging && Math.max(Math.abs(dragOffsetX), Math.abs(dragOffsetY)) > CLICK_EVENT_THRESHOLD_PX) {
        event.preventDefault()
        event.stopPropagation()
    }

I didn't dig in enough to say it should be patched on the main branch or if it going to work in your case, but it worked for me

Not sure this project is getting updated but this solution worked for my project as well.