Skip to main content

Search and Top Navigation

#9121 closed bug (duplicate)

Opened February 25, 2013 04:42PM UTC

Closed March 12, 2013 01:22AM UTC

Auto scrolling (i.e. scroll:true) with Sortable/Draggable hasn't worked since jquery-ui 1.8.24 powered by jQuery 1.8.2

Reported by: dcarrith Owned by: dcarrith
Priority: minor Milestone: none
Component: ui.sortable Version: 1.10.1
Keywords: Cc:
Blocked by: Blocking:
Description

The difference between jQuery UI 1.8.24 and jQuery UI 1.10.1 seems to be how the helper is determining the this.scrollParent. In jQuery UI 1.8.24, it's correctly using either window or html. However, in jQuery UI 1.10.1, this.scrollParent seems to be the parent of the element on which the sortable was instantiated (which doesn't work because the div isn't the container in which it needs to scroll, the document and/or html element is). In my case, it's setting this.scrollParent to the div that wraps the ul element on which I instantiated the sortable (so the li elements can be sortable).

I fixed it in my application by commenting out the if block "if(this.scrollParent[0] !== document && this.scrollParent[0].tagName !== "HTML") {" so that it always treats it as if the this.scrollParent[0] is document or html. This works for my needs, but I'm guessing there's a better way to fix it.

//Do scrolling

if(this.options.scroll) {

/*if(this.scrollParent[0] !== document && this.scrollParent[0].tagName !== "HTML") {

if((this.overflowOffset.top + this.scrollParent[0].offsetHeight) - event.pageY < o.scrollSensitivity) {

alert("scrolling");

alert("this.scrollParent[0].id: "+this.scrollParent[0].id+"\\nthis.scrollParent[0].tagName: "+this.scrollParent[0].tagName);

this.scrollParent[0].scrollTop = scrolled = this.scrollParent[0].scrollTop + o.scrollSpeed;

alert("this.scrollParent[0].scrollTop: "+this.scrollParent[0].scrollTop);

} else if(event.pageY - this.overflowOffset.top < o.scrollSensitivity) {

this.scrollParent[0].scrollTop = scrolled = this.scrollParent[0].scrollTop - o.scrollSpeed;

alert("scrolling");

alert("this.scrollParent[0].id: "+this.scrollParent[0].id+"\\nthis.scrollParent[0].tagName: "+this.scrollParent[0].tagName);

alert("this.scrollParent[0].scrollTop: "+this.scrollParent[0].scrollTop);

}

if((this.overflowOffset.left + this.scrollParent[0].offsetWidth) - event.pageX < o.scrollSensitivity) {

this.scrollParent[0].scrollLeft = scrolled = this.scrollParent[0].scrollLeft + o.scrollSpeed;

} else if(event.pageX - this.overflowOffset.left < o.scrollSensitivity) {

this.scrollParent[0].scrollLeft = scrolled = this.scrollParent[0].scrollLeft - o.scrollSpeed;

}

} else {*/

if(event.pageY - $(document).scrollTop() < o.scrollSensitivity) {

alert("scrolling");

alert("$(window).height(): "+$(window).height()+"\\nevent.pageY: "+event.pageY+"\\n$(document).scrollTop(): "+$(document).scrollTop()+"\\no.scrollSensitivity: "+o.scrollSensitivity);

scrolled = $(document).scrollTop($(document).scrollTop() - o.scrollSpeed);

} else if($(window).height() - (event.pageY - $(document).scrollTop()) < o.scrollSensitivity) {

alert("scrolling");

alert("$(window).height(): "+$(window).height()+"\\nevent.pageY: "+event.pageY+"\\n$(document).scrollTop(): "+$(document).scrollTop()+"\\no.scrollSensitivity: "+o.scrollSensitivity);

scrolled = $(document).scrollTop($(document).scrollTop() + o.scrollSpeed);

}

if(event.pageX - $(document).scrollLeft() < o.scrollSensitivity) {

scrolled = $(document).scrollLeft($(document).scrollLeft() - o.scrollSpeed);

} else if($(window).width() - (event.pageX - $(document).scrollLeft()) < o.scrollSensitivity) {

scrolled = $(document).scrollLeft($(document).scrollLeft() + o.scrollSpeed);

}

//}

if(scrolled !== false && $.ui.ddmanager && !o.dropBehaviour) {

$.ui.ddmanager.prepareOffsets(this, event);

}

}

Attachments (0)
Change History (7)

Changed February 26, 2013 02:37AM UTC by tj.vantoll comment:1

owner: → dcarrith
status: newpending

Hi dcarrith,

Thanks for taking the time to contribute to the jQuery UI project. Could you please provide a reduced test case that shows the issue that you're experiencing. You can use this as a starting point - http://jsfiddle.net/tj_vantoll/wjxe4/.

If you're having these issues in IE this might be related to #9057.

Thanks!

Changed March 11, 2013 02:05PM UTC by onereason comment:2

_comment0: Ran into the same problem. \ \ Used your starting point and created a simple version that breaks in > IE9: http://jsfiddle.net/vtYGg/2/ \ Try dragging any elements down and you will see that it never scrolls in IE. Works in all other browsers I tested.1363012747751277

Ran into the same problem.

Used your starting point and created a simple version that breaks in > IE9: http://jsfiddle.net/vtYGg/2/

Try dragging any elements down and you will see that it never scrolls in IE. Works in all other browsers I tested.

Changed March 11, 2013 03:30PM UTC by dcarrith comment:3

_comment0: I did some more testing this morning after I received the email notification when @onereason commented on the issue. I noticed a difference in how "scrolled" was being set. So, I modified the section I had originally commented out (see above in original ticket write-up). This actually fixes the scrolling issue, but it causes another weird issue where the sorted item gets overlayed on top of the item below where it's dropped (i.e. the items don't get shifted up or down appropriately). So, I'm wondering why in the if block, the scrolled variable is getting set to an integer. But, in the else block it gets set to an object. So, to "fix" it, I changed it so the scrolled variable would get set to an object or false, to be in line with the else block (which I know works fine). \ \ //Do scrolling \ if(this.options.scroll) { \ \ if(this.scrollParent[0] !== document && this.scrollParent[0].tagName !== "HTML") { \ \ if((this.overflowOffset.top + this.scrollParent[0].offsetHeight) - event.pageY < o.scrollSensitivity) { \ \ // The original line results in scrolled being set to an integer \ //this.scrollParent[0].scrollTop = scrolled = this.scrollParent[0].scrollTop + o.scrollSpeed; \ \ // This modified line results in scrolled being set to an object \ scrolled = this.scrollParent[0].scrollTop( this.scrollParent[0].scrollTop + o.scrollSpeed ); \ \ } else if(event.pageY - this.overflowOffset.top < o.scrollSensitivity) { \ \ // The original line results in scrolled being set to an integer \ //this.scrollParent[0].scrollTop = scrolled = this.scrollParent[0].scrollTop - o.scrollSpeed; \ \ // This modified line results in scrolled being set to an object \ scrolled = this.scrollParent[0].scrollTop( this.scrollParent[0].scrollTop - o.scrollSpeed ); \ } \ \ if((this.overflowOffset.left + this.scrollParent[0].offsetWidth) - event.pageX < o.scrollSensitivity) { \ this.scrollParent[0].scrollLeft = scrolled = this.scrollParent[0].scrollLeft + o.scrollSpeed; \ } else if(event.pageX - this.overflowOffset.left < o.scrollSensitivity) { \ this.scrollParent[0].scrollLeft = scrolled = this.scrollParent[0].scrollLeft - o.scrollSpeed; \ } \ \ } else { \ \ if(event.pageY - $(document).scrollTop() < o.scrollSensitivity) { \ scrolled = $(document).scrollTop($(document).scrollTop() - o.scrollSpeed); \ } else if($(window).height() - (event.pageY - $(document).scrollTop()) < o.scrollSensitivity) { \ scrolled = $(document).scrollTop($(document).scrollTop() + o.scrollSpeed); \ } \ \ if(event.pageX - $(document).scrollLeft() < o.scrollSensitivity) { \ scrolled = $(document).scrollLeft($(document).scrollLeft() - o.scrollSpeed); \ } else if($(window).width() - (event.pageX - $(document).scrollLeft()) < o.scrollSensitivity) { \ scrolled = $(document).scrollLeft($(document).scrollLeft() + o.scrollSpeed); \ } \ \ } \ \ if(scrolled !== false && $.ui.ddmanager && !o.dropBehaviour) { \ $.ui.ddmanager.prepareOffsets(this, event); \ } \ }1363015941427153
status: pendingnew

I did some more testing this morning after I received the email notification when @onereason commented on the issue. I noticed a difference in how "scrolled" was being set. So, I modified the section I had originally commented out (see above in original ticket write-up). This actually fixes the scrolling issue, but it causes another weird issue where the sorted item gets overlayed on top of the item below where it's dropped (i.e. the items don't get shifted up or down appropriately). So, I'm wondering why in the if block, the scrolled variable is getting set to an integer. But, in the else block it gets set to an object. So, to "fix" it, I changed it so the scrolled variable would get set to an object or false, to be in line with the else block (which I know works fine).

		//Do scrolling
		if(this.options.scroll) {

			if(this.scrollParent[0] !== document && this.scrollParent[0].tagName !== "HTML") {

				if((this.overflowOffset.top + this.scrollParent[0].offsetHeight) - event.pageY < o.scrollSensitivity) {
					
					// The original line results in scrolled being set to an integer 
					//this.scrollParent[0].scrollTop = scrolled = this.scrollParent[0].scrollTop + o.scrollSpeed;
					
					// This modified line results in scrolled being set to an object
					scrolled = this.scrollParent[0].scrollTop( this.scrollParent[0].scrollTop + o.scrollSpeed );
				
				} else if(event.pageY - this.overflowOffset.top < o.scrollSensitivity) {
					
					// The original line results in scrolled being set to an integer 
					//this.scrollParent[0].scrollTop = scrolled = this.scrollParent[0].scrollTop - o.scrollSpeed;

					// This modified line results in scrolled being set to an object
					scrolled = this.scrollParent[0].scrollTop( this.scrollParent[0].scrollTop - o.scrollSpeed );
				}

				if((this.overflowOffset.left + this.scrollParent[0].offsetWidth) - event.pageX < o.scrollSensitivity) {
					this.scrollParent[0].scrollLeft = scrolled = this.scrollParent[0].scrollLeft + o.scrollSpeed;
				} else if(event.pageX - this.overflowOffset.left < o.scrollSensitivity) {
					this.scrollParent[0].scrollLeft = scrolled = this.scrollParent[0].scrollLeft - o.scrollSpeed;
				}

			} else {

				if(event.pageY - $(document).scrollTop() < o.scrollSensitivity) {
					scrolled = $(document).scrollTop($(document).scrollTop() - o.scrollSpeed);
				} else if($(window).height() - (event.pageY - $(document).scrollTop()) < o.scrollSensitivity) {
					scrolled = $(document).scrollTop($(document).scrollTop() + o.scrollSpeed);
				}

				if(event.pageX - $(document).scrollLeft() < o.scrollSensitivity) {
					scrolled = $(document).scrollLeft($(document).scrollLeft() - o.scrollSpeed);
				} else if($(window).width() - (event.pageX - $(document).scrollLeft()) < o.scrollSensitivity) {
					scrolled = $(document).scrollLeft($(document).scrollLeft() + o.scrollSpeed);
				}

			}

			if(scrolled !== false && $.ui.ddmanager && !o.dropBehaviour) {
				$.ui.ddmanager.prepareOffsets(this, event);
			}
		}

Changed March 11, 2013 03:31PM UTC by scottgonzalez comment:4

@dcarrith: Would you be interested in sending a pull request with your proposed fix?

Changed March 11, 2013 03:37PM UTC by dcarrith comment:5

_comment0: Replying to [comment:4 scott.gonzalez]: \ > @dcarrith: Would you be interested in sending a pull request with your proposed fix? \ \ Sure, but with this fix in place, there is still an issue where the items aren't being shifted up or down. I can try to fix that, but won't be able to look at that until tonight. I would also want to make sure it still works in a simple jsfiddle. 1363016844283324
_comment1: Replying to [comment:4 scott.gonzalez]: \ > @dcarrith: Would you be interested in sending a pull request with your proposed fix? \ \ Sure, but with this fix in place, there is still an issue where the items aren't being shifted up or down. I can try to fix that, but won't be able to look at that until tonight. I would also want to make sure it still works in a simple jsfiddle. \ \ Update: scratch that. My "fix" isn't much of a fix. I just noticed the js error caused by calling scrollTop on an HTML DIV element, which is not a function. I'll look at it some more tonight. 1363016875826212
_comment2: Replying to [comment:4 scott.gonzalez]: \ > @dcarrith: Would you be interested in sending a pull request with your proposed fix? \ \ Sure, but with this fix in place, there is still an issue where the items aren't being shifted up or down. I can try to fix that, but won't be able to look at that until tonight. I would also want to make sure it still works in a simple jsfiddle. \ \ Update: scratch that. My "fix" isn't much of a fix. I just noticed the js error caused by calling scrollTop on an HTML DIV element, which is not a function. I'll look at it some more tonight. It's kind of strange how that caused it to work. 1363019957665505

Replying to [comment:4 scott.gonzalez]:

@dcarrith: Would you be interested in sending a pull request with your proposed fix?

Sure, but with this fix in place, there is still an issue where the items aren't being shifted up or down. I can try to fix that, but won't be able to look at that until tonight. I would also want to make sure it still works in a simple jsfiddle.

Update: scratch that. My "fix" wasn't much of a fix. I noticed the js error caused by calling scrollTop on an HTML DIV element, which is not a function. I'll look at it some more tonight. It's kind of strange how that caused it to work though.

Changed March 11, 2013 04:37PM UTC by dcarrith comment:6

Okay, this seems to fix it. Notice the additional check to make sure this.scrollParent[0].offsetHeight is less than $(window).height() in the 2nd line of code:

		//Do scrolling
		if(this.options.scroll) {

			if( ( this.scrollParent[0] !== document && this.scrollParent[0].tagName !== "HTML" ) && ( this.scrollParent[0].offsetHeight < $(window).height() ) ) {

				if((this.overflowOffset.top + this.scrollParent[0].offsetHeight) - event.pageY < o.scrollSensitivity) {
					this.scrollParent[0].scrollTop = scrolled = this.scrollParent[0].scrollTop + o.scrollSpeed;
				} else if(event.pageY - this.overflowOffset.top < o.scrollSensitivity) {
					this.scrollParent[0].scrollTop = scrolled = this.scrollParent[0].scrollTop - o.scrollSpeed;
				}

				if((this.overflowOffset.left + this.scrollParent[0].offsetWidth) - event.pageX < o.scrollSensitivity) {
					this.scrollParent[0].scrollLeft = scrolled = this.scrollParent[0].scrollLeft + o.scrollSpeed;
				} else if(event.pageX - this.overflowOffset.left < o.scrollSensitivity) {
					this.scrollParent[0].scrollLeft = scrolled = this.scrollParent[0].scrollLeft - o.scrollSpeed;
				}

			} else {

				if(event.pageY - $(document).scrollTop() < o.scrollSensitivity) {
					scrolled = $(document).scrollTop($(document).scrollTop() - o.scrollSpeed);
				} else if($(window).height() - (event.pageY - $(document).scrollTop()) < o.scrollSensitivity) {
					scrolled = $(document).scrollTop($(document).scrollTop() + o.scrollSpeed);
				}

				if(event.pageX - $(document).scrollLeft() < o.scrollSensitivity) {
					scrolled = $(document).scrollLeft($(document).scrollLeft() - o.scrollSpeed);
				} else if($(window).width() - (event.pageX - $(document).scrollLeft()) < o.scrollSensitivity) {
					scrolled = $(document).scrollLeft($(document).scrollLeft() + o.scrollSpeed);
				}

			}

			if(scrolled !== false && $.ui.ddmanager && !o.dropBehaviour) {
				$.ui.ddmanager.prepareOffsets(this, event);
			}
		}

Changed March 12, 2013 01:22AM UTC by tj.vantoll comment:7

resolution: → duplicate
status: newclosed

Duplicate of #7546.This is a duplicate of #7546 so I'm closing this issue in favor of the existing one. @dcarrith, feel free to continue with your pull request but I believe the core problem is in

scrollParent
. See my last comment on #7546: http://bugs.jqueryui.com/ticket/7546#comment:6.