Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix sort order in Chrome #1019

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix sort order in Chrome #1019

wants to merge 1 commit into from

Conversation

michalcarson
Copy link

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

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

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
Copy link
Author

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
Copy link

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
#1019 (comment).

GerHobbelt added a commit to GerHobbelt/SlickGrid that referenced this pull request Nov 23, 2014
6pac referenced this pull request in 6pac/SlickGrid Mar 12, 2015
@6pac
Copy link

6pac commented Mar 13, 2015

This has been integrated into my 'alternative master'. Any testing is appreciated. See #1055

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants