IE11 & Chrome(PointerEvents enabled) & some Firefox scroll to the top of the page after dragging the bottom item with jquery 1.5 <-> 1.11

It happened in Chrome also.

It caused by this line:

var scrollAmount = self.checkScroll(self.currentMouseCoords.y);

pointerEvent & mousedown return different data.

We may borrow some code from this patch:
https://www.drupal.org/node/1261002#comment-8345317

(Or check what we did in D8.)

event.pageX also matter.

I confirm that with jQuery 1.7 and Chrome and drupal core 7.53 that my colleague is observing this issue on Chrome... I have firefox and haven't tested Chrome . I'm testing your patch now, my colleague will let me know if this resolves his issues. Meanwhile I'll test firefox to test for possible regressions.

If it makes a difference, our admin theme is 'adminimal' , overlay is disabled. Our jQuery is version 1.7

this patch didn't help for us, not sure why, perhaps its something wrong with the adminimal admin theme we're using.
I changed admin theme from adminimal to rubik/tao and the table drag is working correctly.

It looks like this is a problem with the adminimal theme.

@joseph.olstad,

Clear caches? Is it same behavior as Summary's GIF?

actually the problem I saw affected firefox as well. We ended up returning to adminimal theme, for now we're disabling tabledrag and using weight values instead.

anyone else still have a glitch on tabledrag when the item is moved either from the top of the section or to the very top of the section? the element doesn't let go of the mouse pointer. it could be something with the adminimal theme we're using where the css might be conflicting with the js, not sure. Aside from that our tabledrag works. This patch actually seems to make a small improvement for us where with it we can move something to the very top element but not from it. seems to affect firefox and chrome.

We were having behavior similar to the 7.53 GIF (Seven theme, jQuery 1.7), except that it wouldn't necessarily scroll all the way to the top, but it would scroll up and the cursor would get off calibration with the drag handle. Applying #5 patch resolved the issue for us.

Also not a tabledrag.js expert but the code is straightforward and well documented. RTBC'ing.

I tried patch #5 and it works for vertical dragging but not horizontal. For example, go to to /admin/structure/menu/manage/management and try moving some items left & right. Items are locked in their horizontal position and can't be moved right or left.

My test
- Drupal 7.56, seven theme
- jQuery 1.8 (using jquery_update)
- Chrome 61, FF56, Edge38
- test at /admin/structure/menu/manage/management
- drag top to bottom (should auto-scroll sensibly, item should stay with the mouse)
- drag bottom to top (should auto-scroll sensibly, item should stay with the mouse)
- drag left to right and right to left

I found that this patch does a better job https://www.drupal.org/node/2908543#comment-12298790

Can confirm this is also happening in GovCMS admin theme.

closed another issue as duplicate. Add the credits

based on #13

This is a WRONG patch!

correct patch

Fixed #13

(** IE has a performance issue on huge menus, not affected by this patch.)

Patch #19 works fine in Chrome 62, FF 56, and Edge 38 using jquery 1.10.

But IE11 has problems. Autoscroll goes really fast and mouse is separated from the item being dragged.

Chrome has a change on the scrollingElement to W3C Standard since V61. BUT the fixes also enable if you switch on "Experimental" flag in Chrome in previous versions.

I tested:
Chrome 62
Chrome 60
IE 11
IE 9

I think the patch still need some better doc. I need a thought on it. Thanks All

Is there a reason the patch treats "pointermove" and "pointerdown" differently (and, in addition, changes things for one of them inside the API function itself but for the other in only one of the pieces of code that calls it)? Is there a reason we can't just treat pageX and pageY the same way the current code treats clientX and clientY, e.g.:

var pageX = event.pageX || event.originalEvent.pageX;
var pageY = event.pageY || event.originalEvent.pageY;

(Or, maybe even simpler, just use event.originalEvent always - is there any scenario where using event.originalEvent wouldn't work?)

+  var scrollNode = document.scrollingElement  || document.documentElement;
   return {
-    x: clientX + document.body.scrollLeft - document.body.clientLeft,
-    y: clientY + document.body.scrollTop  - document.body.clientTop
+    x: clientX + scrollNode.scrollLeft - scrollNode.clientLeft,
+    y: clientY + scrollNode.scrollTop  - scrollNode.clientTop

I am not sure it's right to stop using document.body entirely - seems like some older browsers can only detect the scroll on the <body> (not on the <html> from document.documentElement)? In any case, definitely looking forward to the documentation for this part :)

@David_Rothstein,

In theory, your points are correct. But also incorrect with different conditions. (At least the real testing shown this isn't TRUE all time. I sold myself to real testing rather than theory in this odd case. I tried to simplify it)
The extra conditional used for unknown behaviors for the browser (versions) we can't test.

What's the current browser supports requirement in D7 now? If we assumed "Quirks Mode" will be happened, then some more conditional needed to be applied. e.g.:

/^CSS1/.test(document.compatMode) ? document.documentElement : document.body;

But if we apply a strategy to fix the most usages of browsers today first. This patch really to go IMO.

In theory, your points are correct. But also incorrect with different conditions. (At least the real testing shown this isn't TRUE all time.

What's the specific situation where it isn't true?

What's the current browser supports requirement in D7 now?

I guess it's still https://www.drupal.org/docs/7/system-requirements/browser-requirements in the sense that no one has tried to change that yet :) Although obviously the older ones are much lower priority.

But if we apply a strategy to fix the most usages of browsers today first.

Yes, I agree with that - but my concern is also making sure we don't break something that previously worked. And I think removing the document.body might do that in some browsers.

Is there actually a scenario where we need to use document.documentElement rather than document.body (in other words, where the approach that #2908543: Tabledrag is broken when you've scrolled down on the page in chrome 61 took for this part of the code doesn't work)? And even if there is, it seems like it still might be possible to fall back on checking document.body in the case where document.documentElement may be bogus (e.g. where all the scrollLeft/scrollTop/etc properties on it are 0).

The patch in #22 is working for me in Chrome.

The patch in #22 is working for me in Chrome Version 62.0.3202.75 (Official Build) (64-bit) on Ubuntu (Mate).
Thank you for putting in time & effort to get this fixed.

I also can confirm, that patch #22 solves the tabledrag while scrolling issue I experienced in Chrome 62.0.3202.94 (Mac OSX 10.11.6).

patch #22 works for me in Chrome 63, FF 57, Edge 38, IE11. Thanks!

Patch #22 also works for me in Chrome 63 (Mac) and with Drupal 7.56.

Thanks and a Happy New Year! :)

Patch #22 has solved the described problem, using Chrome on Drupal 7.56. Thanks :)

Here's a simplified patch based on the comments from David Rothstein in #25.

Using Browserstack, I tested both patches on a number of browsers. My test site was current Drupal 7.x from Git, with the jquery_update 2.7 module, with jquery updated to 1.10 on both frontend and admin pages. I tested by going to the /admin/structure/menu/manage/management, scrolling all the way to the bottom, and then clicking and dragging the bottom item up. I did drush cc all everytime I switch which patch was being tested (I actually have them in two separate git branches for easy switching, but that's not important to the results of the test).

Here's the environments where I tested:

• Windows XP: IE 6, 7 and 8 (old school, but still technically supported by Drupal 7)
• Windows 7: IE 9, 10 and 11
• Windows 10: Edge 14, 15, and 16
• MacOS X Sierra: Safari 10.1 and Firefox 58
• MacOS X Sierra: Chrome 64, 61, 60, 56 (sort of random selection, but meant to cover latest and before/after this issue)
• Ubuntu 16.04: Chrome 63 (this is actually my local - not browserstack :-))

I found no browser, where the patch from #22 behaved differently than the simplified one attached here!

If there is a case I missed, please let me know! Otherwise, I think it'd be better to go with the simplified version for same reasons as David R gave in #25.

I tested the patch in #36 on Mac OSX 10.11.6 with Chrome 64.0.3282.140, Firefox 58.0.2, and Safari 11.0.3. I also did Browserstack with IE 11 on Windows 10, which may be in use if the visitor normally uses an assistive technology that doesn't work well on Edge. On the long menu page at /admin/structure/menu/manage/management, both vertical and horizontal table drag worked fine in all the tested browsers.

Forgot to add, this was with jQuery Update set to use version 1.7.

Tested #36 on Chrome Version 64 & Drupal 7.57 and works fine

Chrome 64 appears to have fixed this on various Drupal installations (not just the latest Drupal version) without the patch?

Chrome 64 appears to have fixed this on various Drupal installations (not just the latest Drupal version) without the patch?

Which OS are you using?

I'm running 65.0.3325.146 on Mac OSX 10.13.3 and still experience the issue without the patch.

I just tried Chrome 64 and 65 on macOS and Win7. All four are still bugged without the patch. Even Chromium 64 on Ubuntu, as well.

I used the patch #19 for Chrome and its working fine for me .

@ashutosh.mishra , which jQuery version are you reporting your test results for? the default Drupal core jQuery? or a different version?

Just to mention that I had the exact same problem on Firefox. #36 solved it.
My config: Linux Mint 18.2 Firefox 59.0.2 Drupal 7.58 jQuery 1.7.2 jQuery UI 1.10.2

@joseph.olstad , I have used patch #19 for Drupal 7.58 ,jQuery 1.10.2 and jQuery UI 1.10.2

I tried this patch (#36) and it was giving me a console js error:

tabledrag.js?p7newj:587 Uncaught TypeError: Cannot read property 'clientX' of undefined
    at Drupal.tableDrag.mouseCoords (tabledrag.js?p7newj:587)
    at Drupal.tableDrag.getMouseOffset (tabledrag.js?p7newj:615)
    at HTMLAnchorElement.<anonymous> (tabledrag.js?p7newj:285)
    at HTMLAnchorElement.handle (jquery.js?v=1.4.4:64)
    at HTMLAnchorElement.o (jquery.js?v=1.4.4:57)

This error doesn't only happen on my project, but also on drupal 7.x with it.

I think one of the comments on #25 is better not to do, namely always using originalEvent.

Attached is a patch that revert that bit from #36.

Proper interdiff.

@hanoii Can you give information about the OS, browser and version? (I can see already see your jQuery version.) Anyway, I'd love to try and reproduce this, and should be able to with BrowserStack!

@dsnopek sure, that error I put on #46 was with a brand new D7 install just checked out from git.

- macOS High Sierra 10.13.4
- Chrome Version 65.0.3325.181 (Official Build) (64-bit) (it's currently being updated). Happens on Version 66.0.3359.117 (Official Build) (64-bit) too.

I also see that error on another project with a different jquery version.

Let me re-do it:

tabledrag.js?p7nhbc:587 Uncaught TypeError: Cannot read property 'clientX' of undefined
    at Drupal.tableDrag.mouseCoords (tabledrag.js?p7nhbc:587)
    at Drupal.tableDrag.getMouseOffset (tabledrag.js?p7nhbc:615)
    at HTMLAnchorElement.<anonymous> (tabledrag.js?p7nhbc:285)
    at HTMLAnchorElement.dispatch (jquery.min.js?v=1.10.2:5)
    at HTMLAnchorElement.v.handle (jquery.min.js?v=1.10.2:5)

That's on an actual project, with another jquery version and #36, #46 works. Both locally though.

Confirming the patch #46 works - Mac OS 10.11.6, Chrome 65, Drupal 7.59, jQuery 1.8

 Drupal.tableDrag.prototype.getMouseOffset = function (target, event) {
+  // Complete support for pointer events was only introduced to jQuery in
+  // version 1.11.1; between versions 1.7 and 1.11.0 pointer events have the
+  // clientX and clientY properties undefined. In those cases, the properties
+  // must be retrieved from the event.originalEvent object instead.
+  if (event.type === 'pointerdown') {
+    event = event.originalEvent;
+  }
+

Shouldn't this happen in the mouseCoords() function, instead of the getMouseOffset() function?
The mouseCoords() is called in multiple places, and we always want the correct fallback logic.

This would allow to remove the other fallback logic that was introduced in the past.

And shouldn't we attempt to be more generic, and not restrict this to 'pointerdown' events?
In our case this might be the only case we care about, but maybe we don't see the complete picture.

Looking at the original code that is currently in tabledrag.js:

  if (event.pageX || event.pageY) {
    return { x: event.pageX, y: event.pageY };
  }

I found that if event.clientX and event.clientY are missing, then event.pageX and event.pageY will also be missing.
In these cases we also want to use event.originalEvent.pageX and event.originalEvent.pageY.
The patch in #46 does this, but not in all cases.

  var clientX = event.clientX || event.originalEvent.clientX;
  var clientY = event.clientY || event.originalEvent.clientY;

One has to wonder why this was inserted before the if (event.pageX || event.pageY) {, and not after it, to avoid useless operations.

  return {
    x: clientX + document.body.scrollLeft - document.body.clientLeft,
    y: clientY + document.body.scrollTop  - document.body.clientTop
  };

In my browsers (Chromium, Firefox), document.body.scrollTop and document.body.clientTop are always 0 == zero.
So this formula seems to be useless.
On the other hand, document.body.parentElement.scrollTop DOES give me a non-zero value (unless the page is scrolled to the top).
document.body.parentElement is the same as document.scrollingElement.
Of course this calculation breaks down if we have multiple nested scroll containers.

/**
 * Get the mouse coordinates from the event (allowing for browser differences).
 */
Drupal.tableDrag.prototype.mouseCoords = function (event) {

I think we could reduce confusion if we give this function a better doc description.
What does it actually do?

I think its purpose is to give the absolute position of the event relative to the page top/left corner, even if this corner is outside of the viewport.

Currently this function is in Drupal.tableDrag.prototype.mouseCoords, so it is an object method.
I think it really should be a standalone static function..

The original version of tabledrag.js was introduced by Quicksketch et al in #181066: Drag and drop of table rows (e.g. block admin), commit 6049f23760cc3653e46ee4bf3f7843590d9954f8.

Maybe @Quicksketch still knows the purpose of the document.body.scrollTop stuff? Maybe for some old browsers this was the correct thing to do?

I tried BrowserStack with Safari Version 7.1.8 (9537.85.17.9.1)
(it was the only Safari available in the free trial)
What I found:
- document.body.scrollTop has the scroll position, so it is not zero (unless I am at the top)
- document.body.parentElement.scrollTop IS zero, no matter where I scroll.
- document.scrollingElement is not defined.
- window.scrollY is defined, and the same as document.body.scrollTop.
- the event.pageX works!

Some research.

MouseEvent.pageX
https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/pageX
missing in IE < 9, and Chrome < 45.0.
(not sure if jQuery helps with that)
http://www.jacklmoore.com/notes/mouse-position/
cross-browser snippet (**).
jQuery has its own way of fixing this (***).

MouseEvent.clientX
https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/clientY
missing in IE < 6, and Chrome < 56.

document.scrollingElement
https://developer.mozilla.org/en-US/docs/Web/API/document/scrollingElement
missing in all IE versions, and in Chrome < 44.0.

window.scrollY
https://developer.mozilla.org/en-US/docs/Web/API/Window/scrollY
missing in all IE versions.
The page has example code for full cross-browser compatibility! (*)

document.body.scrollTop
https://stackoverflow.com/questions/19635188/why-is-body-scrolltop-depre...
https://stackoverflow.com/questions/43717316/google-chrome-document-body...
"this behavior changed between Chrome 60 and Chrome 61:"
(possibly similar development in Safari)

(*) window.scrollY cross-browser
from https://developer.mozilla.org/en-US/docs/Web/API/Window/scrollY

var supportPageOffset = window.pageXOffset !== undefined;
var isCSS1Compat = ((document.compatMode || "") === "CSS1Compat");

var x = supportPageOffset ? window.pageXOffset : isCSS1Compat ? document.documentElement.scrollLeft : document.body.scrollLeft;
var y = supportPageOffset ? window.pageYOffset : isCSS1Compat ? document.documentElement.scrollTop : document.body.scrollTop;

(**) MouseEvent.pageX cross-browser
from http://www.jacklmoore.com/notes/mouse-position/

  var pageX = e.pageX;
  var pageY = e.pageY;
  if (pageX === undefined) {
    pageX = e.clientX + document.body.scrollLeft + document.documentElement.scrollLeft;
    pageY = e.clientY + document.body.scrollTop + document.documentElement.scrollTop;
  }

Note that those examples never subtract the scrollElement.clientLeft and *.clientTop.
In my browser, this is zero anyway.

(***) jQuery adding pageX and pageY to events.
https://code.jquery.com/jquery-1.4.1.js (the version that is in Drupal core without jQuery update)

		// Calculate pageX/Y if missing and clientX/Y available
		if ( event.pageX == null && event.clientX != null ) {
			var doc = document.documentElement, body = document.body;
			event.pageX = event.clientX + (doc && doc.scrollLeft || body && body.scrollLeft || 0) - (doc && doc.clientLeft || body && body.clientLeft || 0);
			event.pageY = event.clientY + (doc && doc.scrollTop  || body && body.scrollTop  || 0) - (doc && doc.clientTop  || body && body.clientTop  || 0);
		}

Btw, == null matches both null and undefined, but not zero.

This snippet in jQuery might fail if doc.ScrollTop is zero and the document.body has scroll enabled with CSS.. but this is a weird edge case.

Patch following.

Patch according to previous comments.

18:01:09 /usr/local/bin/composer  install --prefer-dist --no-suggest --no-interaction --no-progress --working-dir /var/lib/drupalci/workspace/jenkins-drupal_d7-76362/source
18:01:10 Composer Command Failed

What does this mean?
If the patch failed to apply, it would be a different message, right?

test runner is fixed, re-triggering.

the patch at #54 fixes the problem on FF 59.0 and drupal 7.59. Thank you.

The snippet lifted from jQuery.event.fix() is incomplete in #54 - there's supposed to be another line above:
var doc = document.documentElement, body = document.body;

Without this, the next line will error out because doc is undefined...

(Thanks @donquixote for all the research - I was looking at another issue affected by Chrome's change in scrollTop behaviour)

I needed this today, so I've added the missing line mentioned in #59.

@grahamC (#59)
Sorry, I saw your post and wanted to fix this, but then was busy with other stuff.
You are absolutely right!

@mw4ll4c3 (#60)
Thanks for the new patch version!
Can you post an interdiff?
I think it is quite obvious to those who are reading all the comments, but the interdiff will help people who just want a quick overview.
Thanks!

Patch passed the retest.

@mw4ll4c3 (#60)
Thanks for the new patch version! This patch is working for me in Chrome and in Firefox.

@60 - First, thanks for the patch!

Functionally, it looks solid, though we should probably be throwing Error objects rather than literals.

I have to run for a bit, though if someone else beats me to writing a patch, it would be most welcome.

Functionally, it looks solid, though we should probably be throwing Error objects rather than literals.

I did some grep in /misc/, and found only one case of "throw new", but many that throw a string.
I personally do not care much. But perhaps we can find documentation or a good argument why it should be one or the other?

In PHP an exception class allows to write selective catch blocks.
(and you can only throw throwable/exception objects)
In JS, I have no idea which benefit we would get from Error objects.
I found this, https://www.nczonline.net/blog/2009/03/10/the-art-of-throwing-javascript...

Again, I currently have no opinion.

@67 - tl;dr, it makes debugging easier for things that can already be obnoxious to debug. You've hit the nail right on the head with that article:

"The only surefire way to have all browsers display your custom error message is to use an Error object."

ESLint (https://eslint.org/docs/rules/no-throw-literal) would concur:

"It is considered good practice to only throw the Error object itself or an object using the Error object as base objects for user-defined exceptions. The fundamental benefit of Error objects is that they automatically keep track of where they were built and originated."

The ones outside of this patch that throw literals should probably be addressed in a separate issue.

EDIT: Going to try to carve out some time today or Monday to roll this all up.

@davidwatson , please include an interdifff for your patch, we'd like to see the changes between your patch and the previous.
here is the documentation on creating an interdiff https://www.drupal.org/documentation/git/interdiff

Interdiff is attached. The docs say to use "[new_comment_number]" but I went with 69 to match the filename of the new patch.

at the top I used != undefined to match both null and undefined.

+  // Match both null and undefined, but not zero, by using != instead of !==.
+  if (event.pageX != undefined && event.pageY != undefined) {
+    return {x: event.pageX, y: event.pageY};
+  }

but later I am using !== undefined, which does not match null.

+  // pageX and pageY properties undefined. In those cases, the properties must
+  // be retrieved from the event.originalEvent object instead.
+  if (event.originalEvent && event.originalEvent.pageX !== undefined && event.originalEvent.pageY !== undefined) {
+    return {x: event.originalEvent.pageX, y: event.originalEvent.pageY};
+  }

I don't remember why I did this.
Is the null case possible in one situation, but not in the other?
Considering that I was sloppy in another place as well, perhaps this was a mistake.

If there is no reason to treat these two situations differently, I think we should be consistent and use != undefined in both situations.

https://stackoverflow.com/a/3390468

safer to use typeof(variable) == "undefined" and a seperate check for null

see interdiff and new patch.

Noteworthy, I checked D8 tabledrag.js and it's using typeof
we should use typeof as well, see interdiff 74 and patch 74

safer to use typeof(variable) == "undefined" and a seperate check for null

From the stackexchange answer:

Using typeof is my preference. It will work when the variable has never been declared, unlike any comparison with the == or === operators or type coercion using if.

I think we do not want to support the "variable never defined" case here. Supporting this case in situations where it is not required obfuscates forgotten var declarations or misspelled variables. Same with NULL !== $value vs isset($value) in PHP.

(undefined, unlike null, may also be redefined in ECMAScript 3 environments, making it unreliable for comparison, although nearly all common environments now are compliant with ECMAScript 5 or above).

Hm, this seems more significant. I don't know if we have to worry about this ECMAScript 3 weirdness. Do we?

Just for giggles, we could also do this:
variable === [][0] instead of variable === undefined.

and a seperate check for null

Another thing that would work:
variable == null.
The expected values are null, undefined and numbers. For those, the formula has the correct outcome.
And null does not have the ECMAScript 3 redeclaration problem.

The unpleasant part of two distinct checks is the repeated statement to be compared.

Noteworthy, I checked D8 tabledrag.js and it's using typeof

If this is the case, I guess I am ok with it.

Btw why typeof(variable) == "undefined" and not typeof(variable) === "undefined"?

And, as in my comment #73: In your patch there is still different treatment in different conditional blocks.
Do we expect null in some situation, but not in another?

+  if (event.originalEvent && typeof(event.originalEvent.pageX) != "undefined" &&
+    typeof(event.originalEvent.pageY) != "undefined" &&
+    event.originalEvent.pageX != null && event.originalEvent.pageY != null) {

I don't like the line breaks here.
See #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines, especially #84 and #86.

And btw, if we already have distinct checks for undefined and null we should be using !== not !=.
Otherwise, one != null would kill both flies with one clap, and eliminate the need for line breaks.

@myself in #76:

I think we do not want to support the "variable never defined" case here.

This concern does not really matter for object properties.

Hi Guys
As much as I like too discussing PHP semantics here, I'm not comfortable of having a patched Drupal core running on my live sites.
Can we get back to the issue at hand and insure the issue is fixed before/or for the next D7 core upgrade?

While I don't expect that we copy the D8 tabledrag.js , D7 is dealing with different versions of jQuery.
D8 tabledrag.js does use typeof, and typeof is part of the javascript definition (not specific to jQuery). I'd say it's a safe bet that we too should also use typeof

as for !== and != "undefined" , "undefined" is a string, typeof (someUndefinedVariable) if it is undefined typeof will also ALWAYS return a string so no need to use !== , although it wouldn't hurt, shouldn't matter or make a difference for typeof.

feel free to post an updated patch with code formatting changes. I wasn't going to let a line go past 140 characters so I chopped them at 80.

Noteworthy, I checked D8 tabledrag.js and it's using typeof

I just checked myself, D8 tabledrag.js uses typeof for a totally different situation.
But overall, grep -R undefined core/misc/ in D8 reveals that most comparisons are like typeof a === 'undefined'. I don't find any case where undefined is used as a constant. So you are still right.

On the other hand, the jquery.js in D8 and also in D7 use undefined as a constant all the time, and comparisons like value !== undefined.

jquery.js also uses != null to match undefined and null:
egrep -R --context=2 "null" core/assets/vendor/jquery/jquery.js | egrep --context=2 "(==|!=)"

				// Handle cases where value is null/undef or number
				return ret == null ? "" : ret;

This seems quite a common technique, see https://stackoverflow.com/questions/2647867/how-to-determine-if-variable...
Spec: http://www.ecma-international.org/ecma-262/6.0/#sec-abstract-equality-co...

I trust jquery and the larger javascript community more than Drupal core, sorry :)

feel free to post an updated patch with code formatting changes. I wasn't going to let a line go past 140 characters so I chopped them at 80.

I shall do that.
Perhaps two versions so we have a choice.

Perhaps two versions so we have a choice.

I came to the conclusion that != null is the preferred way to do it, so I am not uploading a different version with typeof.

I wasn't going to let a line go past 140 characters so I chopped them at 80.

Just saying I don't mind line breaks (if there is no other way to keep it short), I just disagreed with the format.
I think it is more readable to
- put the && or || operator at the start of each line.
- either put every clause of the && on a separate line, or put all of them on one line.

I don't think we have a written or agreed-upon standard yet. It is pending in #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines, but people seem to mostly agree. (the issue is for PHP, but the same arguments apply for js)

Either way, using != null eliminates the need for those line breaks.

jQuery uses all kinds of code on purpose because it has to abstract things for us lazy programmers

typeof is pure Javascript , explicitly looking for what we want to know, if it is undefined or not. Patch 74 respects the logic of your patch while being more explicit.

I am not a core maintainer but they core maintainers (like David rothstein (resigned recently)) usually lean towards consistency with Drupal 8. typeof is not unique to Drupal core Javascript, maybe there is a reason why but usually just follow D8 if it makes sense to do so.

@Donquixote, appreciate your input here,
I've made the suggested adjustments to the code style, however I intended to maintain the strategy of testing for both undefined and not null. This is because of the plethora of browsers (versions and browser types) and javascript engines we're supporting, it's good to test for them both, extra safe.

The whole reason for this issue was because Chrome and Firefox put out some dud versions with regressions. They've since released newer versions that seem to have less issues, however, this patch seems like an improvement and has gotten a lot of reviews.

I'm not really changing anything here other than using the explicit typeof check for undefined , I believe this has better compatibility and to me it's easier to understand as a programmer reading the code.

See interdiff from 69 to 84, and the new patch.

This is because of the plethora of browsers and javascript engines we're supporting

If there is a browser where undefined == null returns false or undefined != null returns true, then you have a point. I wonder if there is any way to find out. I already posted a question comment under https://stackoverflow.com/a/2647888/246724.
On the other hand, I would argue that on such a browser, very elementary parts of jquery itself would fail to work, so we would already have a bigger problem.

The comparison to D8 you made earlier does not really hold up, I think, because the usage situations are not comparable.

I think we should get a 3rd opinion to resolve this.

-----

This being said, here is some code style feedback:

+    && event.originalEvent.pageY != null) {

I'd say put the ) { on the next line. But this is a matter of taste, one could say.

+    // And event.originalEvent.pageX is not null.
+    && event.originalEvent.pageX != null

Seems a bit overkill to express this in a comment.

+    && event.originalEvent.pageX != null

I still think this should be !== and not !=, for the same reason we use strict comparison in PHP.
The behavior will be the same (because we already checked the other case before), but semantically it will be more clear that you are testing for the exact value of null, and something else (like undefined). We can also expect that !== is slightly faster than != because there are fewer special cases (look at the spec for !=), but this is of course secondary.

D8 also uses !== "undefined" over != "undefined".

+  if (typeof(event.clientX) == "undefined" || typeof(event.clientY) == "undefined") {

If you want consistency with D8, then use typeof event.clientX === "undefined" without the brackets, and with === instead of !=.

suspect that js doesn't like comments in the conditionals

Passed on second run

Subscribing

@76 - We do need to worry about ECMAScript 3, as it turns out. D7 core is expected to support IE6+, while ECMAScript 5 didn't appear until IE9 (https://www.drupal.org/docs/7/system-requirements/browser-requirements).

@85 -

I think we should get a 3rd opinion to resolve this.

First, I appreciate the amiable appeals to internal consistency and best practice both. For the sake of quorum for consensus, != null in @81 seems cleaner, more aligned with best practice, and as stated, also does not have the redeclaration issue.

One more for RTBC on @81.

I can't find anything that substantiates "better compatibility" with 84/86's use of typeof (which not only gets into 1539712, but is more confusing to maintain regardless of formatting), so +1 and RTBC for #81.

Merging this in either form will be more beneficial than continuing to discuss what is maintainable, is compatible, and resolves a thorn in many site builders' sides for the past 16+ months. I appreciate the discussion and support taking the time to achieve high quality code, but at this point all options are of sufficient high quality so the ROI of continuing to refine has diminished into noise.

re: #85

D8 also uses !== "undefined" over != "undefined".

!= and == is sufficient in this case because we're comparing a string not null/0/false/1/true , null/0/false/1/true is where the third = makes a difference, for strings it does nothing , has no effect.

null/0/false/1/true can never be confused with the string "undefined" , not even close, even in the dumbest browser javascript engine. Remember, we're writing code for the dumbest browser possible.

another look at the D8 tabledrag.js

typeof is used here:
https://cgit.drupalcode.org/drupal/tree/core/misc/tabledrag.js#n7

and here:
https://cgit.drupalcode.org/drupal/tree/core/misc/tabledrag.js#n582

However !== null is also used here:
https://cgit.drupalcode.org/drupal/tree/core/misc/tabledrag.js#n473

so ya, either way looks good. My preference is with my latest patch using typeof.

#87 works great! Thanks.

Patch 87 works for me in Drupal 7.59 on FF 59.0. Thank you.

@joseph.olstad (#94):

!= and == is sufficient in this case because we're comparing a string not null/0/false/1/true , null/0/false/1/true is where the third = makes a difference, for strings it does nothing , has no effect.

null/0/false/1/true can never be confused with the string "undefined" , not even close, even in the dumbest browser javascript engine. Remember, we're writing code for the dumbest browser possible.

This is correct.
If both values have the same type then == and === are equivalent.
Still, if there is a choice, then === should be preferred over ==, because === is less "magic".

Your Drupal 8 examples below also use === instead of ==, even though == would do the job (we know that typeof returns string).

typeof is used here:
https://cgit.drupalcode.org/drupal/tree/core/misc/tabledrag.js#n7

Completely different purpose.
typeof Symbol === "function" returns false if Symbol is not defined (ECMAScript before 2015), whereas Symbol !== undefined would trigger an error.

In our case we are testing an object property, not a top-level variable, so we don't need typeof.

and here:
https://cgit.drupalcode.org/drupal/tree/core/misc/tabledrag.js#n582

Again, typeof is used here to avoid error on undefined top-level variables.
Not necessary for object properties.

However !== null is also used here:
https://cgit.drupalcode.org/drupal/tree/core/misc/tabledrag.js#n473

this.rowObject was initialized as null, so we no longer need to worry about undefined.
Therefore !== null is ok in this case.
If we had not initialized it, we would use != null to also detect the case where it is undefined.

@mforbes (#93):

Merging this in either form will be more beneficial than continuing to discuss what is maintainable, is compatible, and resolves a thorn in many site builders' sides for the past 16+ months. I appreciate the discussion and support taking the time to achieve high quality code, but at this point all options are of sufficient high quality so the ROI of continuing to refine has diminished into noise.

I generally with this statement, but:

• I don't think this is the reason why this was not committed yet. We have two "3rd opinions" in #92 and #93. All arguments are on the table. We only need a maintainer to make a choice and go with it. Either one of the solutions is better than nothing.
• Even if the impact of the difference is not great, I hope we can learn from such distinctions.

Patch #87 worked for me with Drupal 7.59

#87 works great in Drupal 7.59, jquery 1.8 using FF61, Chrome67 and Edge42.

But, there's a small problem with autoscroll in IE11. To reproduce this...

- start up IE11
- go to /admin/structure/menu/manage/management
- without scrolling the page, try to drag "Dashboard" down to the bottom of the window
- It should autoscroll when you get past the bottom, but it doesn't

If you scroll the page just a bit with the mousewheel, then this problem goes away and autoscroll works fine.

#99: Does what you've described happen only with patch #87 or regardless? If the latter, I think creating a separate issue might be best, because "should autoscroll ... but it doesn't" is discrete from the "scroll to the top of the page" behavior written in this issue's title.

Good point. I checked and the problem described in #99 happens without patch #87. It even happens in D8.

I've created https://www.drupal.org/project/drupal/issues/2985695

#87 works for me

Either way probably ok.

Just a note === is OK for string comparison when both variables compared are expected to be strings.

Depending on browser version and type, === can be faster (or slower) than ==. === is more precise true if same type and data. == is truthy 0 is same as false.

Hi guys,

I'd like to get this sorted so I can propose it for Drupal 7 asap.

JS is not my cup of tea, but I processed some documentation pages and noticed that there are JS style conventions in Drupal.
Drupal uses 'eslint-config-airbnb' as ESLint shareable config.

Apparently, using "!=" is allowed only when checking `null`.

Is it what you're trying to do with this patch? Or there is something else to check?

Could you please find an agreement between those two patches and then we'll proceed ahead asap.

Thanks you guys.

Thanks @Pol!
I was not aware that we are using eslint-config-airbnb. It seems like a good idea!

Apparently, using "!=" is allowed only when checking `null`.

Well, I find this very explicit rule in eslint-config-airbnb.
https://github.com/airbnb/javascript#comparison-operators--equality

15.1 Use === and !== over == and !=. eslint: eqeqeq

And in Drupal,
https://www.drupal.org/docs/develop/standards/javascript/javascript-codi...

Strict equality MUST be used in comparisons (=== or !==).

About the "when checking `null`", I find no explicit rule in the main page.
But I do find this discussion, which contains similar arguments as were made here,
https://github.com/airbnb/javascript/issues/60 "Say something about undefined".

Also this comment on a PR,
https://github.com/airbnb/javascript/issues/1473#issuecomment-312178468

There is only two reasons this is the case: it's a good practice to treat null and undefined as the same; and undefined can be redefined (ES5 only prevents it from being redefined in the global scope), so === undefined isn't safe, and typeof x === 'undefined' || x === null is a bit verbose.

I was going to consider it a matter of taste or opinion, but I feel validated by the links in #105 and #106.

The code style clearly says to use !== and === instead of != and ==, the discussions linked make valid arguments for a very specific exception for != null to test against null / undefined.

Patch #87 (file is named -86-) uses != instead of !== when it clearly should not. It is technically redundant, and violates the explicit convention without a valid justification.

Patch #81 (which I am re-uploading here as #106) uses !=, which formally violates the coding standard of only ever using !==, but it is validated by the discussions linked in #106, which argue for an exception for != null, and it explains this in a code comment.

I think we should update our own coding standards to mention the != null case, but I think we can move forward with this one without waiting for an explicit coding standards update.

I am re-uploading #81 here as #106, with an interdiff to #87.
I will not reiterate arguments made before, just check the previous comments.

I think RTBC still applies, because this is essentially the same patch that was set to RTBC before via #92 and #93.

Hi,

Thanks for the analysis and the summary, there is nothing to add.

I successfully tested the patch, setting it to "Needs review" to have tests running for the last time.

Hi,

Setting this issue back to RTBC, thanks both of you, I'm going to push this patch forward now.

Just tested the patch with success on 7.59. Thanks all!

#107 works for me. Thanks! I'm really happy I found this thread. I have been struggling with this for quite a while now.

#107 tested for more than 700 draggable items on page, works like charm

Thanks for the patch

This has had ample community review.
For the sake of progress let's get this in, I think FabianX and Pol will agree.

Hi Joseph,

Fabian and I are doing some triage prior committing this, but it's in our todo list for sure.

Fixing commit credits.

I think this was sufficiently tested, but:
For anyone doing tests:
Please report
- your browser name and version.
- the theme you are using, e.g. seven theme or something else.

It works in Firefox Quantum 62.0 (Ubuntu) with the seven theme.

This did not make 7.60. Changed tag to 7.61

patch 107 works for me in this setup:
D7.60
FF 62.0.3
Bootstrap 7.x-3.21.

Thank you

Patch #107 works for me as well on D7.60.
I don't think misc/tabledrag.js has changed in 7.60.

I don't mean to be complainer but I don't understand why this isn't in core yet? It's a major issue that affects every D7 install and the patch has been working, tested and reviewed for a long time. Are any of those statements incorrect?

@Shiraz Dindar You definitely have reason to complain and your statements are correct. I have already complained here over 5 month ago.
The only reason I add my results to this issues is because I will keep it alive until it is sorted.

Great was my disappointment that this bug was not included in the D7.60 upgrade. The patch has been tested over and over and polished up.
It is a major issue and a simple fix.

It bugs the hell out of me that I have to maintain a patch on Drupal core on my sites. This has been the longest in my 12 years working with Drupal that I have to do it.

Please, anybody, what will it take for the Drupal Demigods to take notice?

Sorry if it sounds like I'm a kid throwing out my toys, but in fact, I am at this stage.

@pol will be committing this soon I'm sure, and I agree, there's a lot of core issues ready to push that have tests and are well vetted.
it was the security advisory that was holding up things, now that that is done, I'd suggest to put the foot on the gas pedal and push this in and many others that are clearly ready.

I have to concur with @124 and @125—folks have been waiting for a fix for nearly two years now. Is there anything else we can do to make sure this goes out the door?

Hello,

I'm discussing about the next release with Fabian, we will try to include it in the next week's release (7.61).

Thanks!

Thanks for all your effort @Pol. Isn't Drupal 7.61 going to be released tomorrow and not "next week"?

7.61 is indeed planned for today according to https://groups.drupal.org/node/534483 although it's getting quite late in the day, so I worry that it will be postponed.

This is getting off-topic; see #99 - #100 at https://www.drupal.org/project/drupal/issues/2947772#comment-12829405 as precedent.

Decided to put this in 7.61 -- after testing it very carefully.

Thanks, commit and pushed to 7.x!

Thanks! Finally!

Great job, thanks, it works.

This is fixed, thanks everyone!

Removing the pending commit tag (it was already committed).