mleibman / SlickGrid

A lightning fast JavaScript grid/spreadsheet
http://wiki.github.com/mleibman/SlickGrid
MIT License
6.81k stars 1.98k forks source link

fix sort order in Chrome #1019

Open michalcarson opened 9 years ago

michalcarson commented 9 years ago

Default "comparer" function was returning unreliable result in Chrome, giving consistent but wrong sort order. Results in Firefox were correct.

GerHobbelt commented 9 years ago

what went wrong?

I don't know of any scenario that would behave different in different browsers unless you hit the sort algorithm specifics which are not fixed in the JavaScript spec(s): returning zero as a comparison result will produce undefined behaviour as Array.sort is an "unstable" sort algorithm (quicksort & friends).

michalcarson commented 9 years ago

http://jsfiddle.net/michalcarson/j8dcmxwx/

Just threw this fiddle together to compare the results of the two methods. I may have something whacked here because the result of the current comparator (a.value - b.value) is always coming up as NaN. That's not what I was expecting. But if that's correct, it's no wonder the sort is not working with that comparator.

The data used is similar to the data my users were looking at when they complained about the order. The dataView will not put country names in the right order under Chrome. They sort correctly under Firefox.

GerHobbelt commented 9 years ago

Aha. You're subtracting strings. The subtraction will attempt to coerce these values to numbers first (can't subtracts strings) and since the string content is definitely not numeric the coerce (which is very similar to running them them through Parsefloat() each before subtracting) and thus match the spec by producing NaN for each. And then it becomes subtracting two NaNs, which produces a comparator result that outside the spec (NaN compares false to everything else, including itself).

Your 'greater than' comparison does not suffer from the same 'must coerce to number first' conundrum hence sensible results everywhere.

I'd say this is browser specific, but what you observe is different browsers acting different on outside-the-spec inputs, which is to be expected. Frankly, I'm surprised FF does something 'seemingly sane' with this...

This explains why the patch is working. Thanks for your response & jsfiddle. Indeed a 'greater than'-based default comparer is "better" (more flexible) than one which is subtraction based.

Thanks!


a test run in the browser (Chrome) which shows this and other trouble with a wide range of inputs to sort(): note that even 'greater than' doesn't work for all (e.g. when you have objects or NaNs in the collection to sort itself).

Also note the difference when comparing strings and numbers: the type coercion is in different directions.


browser test; shows different behaviour or comparison vs. subtract (add console.log()s) and several exhibits of the instability of Array.sort() (a known artifact of the quicksort algorithm family) -- only when the comparison function can guarantee unique identification of each array element and produce a consistent less-than response to that would such instability 'disappear'; FF and Chrome may use different sort algorithms but then I wonder how the new comparison function you introduce would work out "better", as it does not remove the instability per se; it only changes behaviour re implicit type coercion.

a = [0, 1e-100, 1e-200, -1e-100, -1e-200, NaN, NaN, Infinity, -Infinity]
a.push("xxx")
a.push(-1e99)
a.push("1e-200")
a.push("1e-100")
a.push({})
a.push(1e99)
a
//  --> [0, 1e-100, 1e-200, -1e-100, -1e-200, NaN, NaN, Infinity,
-Infinity, "xxx", -1e+99, "1e-200", "1e-100", Object, 1e+99]
a.sort(function (a, b) { return (b === a ? 0 : b > a ? 1 : -1); });
//  --> [Infinity, "xxx", Object, NaN, NaN, 1e+99, 1e-100, "1e-200",
"1e-100", 1e-200, 0, -1e-200, -1e-100, -1e+99, -Infinity]
a.sort(function (a, b) { return (b - a); });
//  --> [Infinity, 1e+99, 1e-100, "1e-100", NaN, "1e-200", Object, "xxx",
NaN, 1e-200, 0, -1e-200, -1e-100, -1e+99, -Infinity]
a.sort(function (a, b) { return (isNaN(a) ? 1 : isNaN(b) ? -1 : b === a ? 0
: b > a ? 1 : -1); });
//  --> [Infinity, 1e+99, 1e-100, "1e-200", "1e-100", 1e-200, 0, -1e-200,
-1e-100, -1e+99, -Infinity, "xxx", NaN, Object, NaN]
a.sort(function (a, b) { return (isNaN(a) ? 1 : isNaN(b) ? -1 : b - a); });
//  --> [Infinity, 1e+99, 1e-100, "1e-100", "1e-200", 1e-200, 0, -1e-200,
-1e-100, -1e+99, -Infinity, NaN, Object, NaN, "xxx"]
a.sort(function (a, b) { return (isNaN(a) ? 1 : isNaN(b) ? -1 : b === a ? 0
: b > a ? 1 : -1); });
//  --> [Infinity, 1e+99, 1e-100, "1e-200", "1e-100", 1e-200, 0, -1e-200,
-1e-100, -1e+99, -Infinity, "xxx", NaN, Object, NaN]
a.sort(function (a, b) { return (b === a ? 0 : b > a ? 1 : -1); });
//  --> [Infinity, NaN, "xxx", 1e-100, 1e-200, 0, -1e-100, -1e+99,
-Infinity, Object, 1e+99, "1e-200", "1e-100", NaN, -1e-200]
a.sort(function (a, b) { return (b === a ? 0 : b > a ? 1 : -1); });
//  --> [Infinity, 1e+99, 1e-100, 1e-200, "xxx", NaN, Object, "1e-200",
"1e-100", 0, NaN, -1e-200, -1e-100, -1e+99, -Infinity]
a.sort(function (a, b) { return (b === a ? 0 : b > a ? 1 : -1); });
//  --> [Infinity, 1e-100, 1e-200, "xxx", NaN, Object, 1e+99, NaN,
"1e-200", "1e-100", 0, -1e-200, -1e-100, -1e+99, -Infinity]
a.sort(function (a, b) { return (isNaN(a) ? 1 : isNaN(b) ? -1 : b - a); });
//  --> [Infinity, 1e+99, 1e-100, "1e-100", 1e-200, "1e-200", 0, -1e-200,
-1e-100, -1e+99, -Infinity, NaN, "xxx", NaN, Object]
a.sort(function (a, b) { return (isNaN(a) ? 1 : isNaN(b) ? -1 : b - a); });
//  --> [Infinity, 1e+99, 1e-100, "1e-100", 1e-200, "1e-200", 0, -1e-200,
-1e-100, -1e+99, -Infinity, Object, NaN, "xxx", NaN]

Infinity - Infinity   // even when you start without NaNs you can still get
one from a subtraction:
//  --> NaN
a.push(Infinity)   // --> with the rest of the dataset this means the
'Infinity's won't end up next to one together

a.sort(function (a, b) { return (b === a ? 0 : b > a ? 1 : -1); });
//  --> [Infinity, NaN, "xxx", NaN, Object, Infinity, 1e+99, "1e-200",
"1e-100", 1e-100, 1e-200, 0, -1e-200, -1e-100, -1e+99, -Infinity]
a.sort(function (a, b) { return (b === a ? 0 : b > a ? 1 : -1); });
//  --> [Infinity, "xxx", NaN, Object, Infinity, 1e+99, "1e-200", NaN,
1e-100, "1e-100", 1e-200, 0, -1e-200, -1e-100, -1e+99, -Infinity]
a.sort(function (a, b) { return (b === a ? 0 : b > a ? 1 : -1); });
//  --> [Infinity, NaN, Object, Infinity, 1e+99, "1e-100", NaN, "xxx",
1e-100, "1e-200", 1e-200, 0, -1e-200, -1e-100, -1e+99, -Infinity]

Met vriendelijke groeten / Best regards,

Ger Hobbelt


web: http://www.hobbelt.com/ http://www.hebbut.net/ mail: ger@hobbelt.com

mobile: +31-6-11 120 978

http://jsfiddle.net/michalcarson/j8dcmxwx/

Just threw this fiddle together to compare the results of the two methods. I may have something whacked here because the result of the current comparator (a.value - b.value) is always coming up as NaN. That's not what I was expecting. But if that's correct, it's no wonder the sort is not working with that comparator.

The data used is similar to the data my users were looking at when they complained about the order. The dataView will not put country names in the right order under Chrome. They sort correctly under Firefox.

— Reply to this email directly or view it on GitHub https://github.com/mleibman/SlickGrid/pull/1019#issuecomment-60816619.

6pac commented 9 years ago

This has been integrated into my 'alternative master'. Any testing is appreciated. See https://github.com/mleibman/SlickGrid/issues/1055