SyedWasiHaider / vue-highlightable-input

A vue component to highlight text as you're typing
MIT License
183 stars 30 forks source link

Sorting of highlights array is inconsistent #18

Closed haimgel closed 5 years ago

haimgel commented 5 years ago

normalizedHighlights() sorts the highlights array, but it does so inconsistently, by just using > comparison. This is incorrect, the compare function should return -1, 0 or 1 and not true or false:

This results in different sorting results in Chrome and Safari/Firefox. Just try running this in a console:

['a', 'b'].sort((a,b) => a < b)

This will sort differently in Chrome and Firefox, and HighlightableInput also sorts differently in these browsers.

Moreover, I'd like to suggest making the sorting entirely optional. I'm using HighlightableInput with a set of regexes, and depend on the order being exactly like I specified, not modified in an arbitrary way.

SyedWasiHaider commented 5 years ago

Good catch. I think I need to use https://caniuse.com/#feat=localecompare.

SyedWasiHaider commented 5 years ago

Also I'm curious what kind of behavior you see in the actual vue component that made you notice this.

SyedWasiHaider commented 5 years ago

Alright so yeah sort is definitely broken. I also found the reason why I think I originally put in the sort. I wanted to maximize the number of highlights for a given input.

Here is an example. For your highlights have:

gregg, greg, and gg (in that order for your highlight array). greggregg would only have greggregg. Now if you reverse the order of the highlight array, you would get greggregg (which in my opinion is a better highlight). The localCompare sort will solve this particular issue (which I will push sometime this week).

It will, however, not solve the following case: greggreggg. The optimal highlight here should be the entire string. greg greg gg (spaces added to illustrate the point) but instead we would only highlight greggreggg. Maybe I've overthinking it but this probably requires a more elegant solution. Regardless, I'm curious how many users this will actually affect.

haimgel commented 5 years ago

Hey, sorry about the late reply! I'm using highlightable-input to highlight items surrounded in double-mustaches in the text. However, the token inside the mustache must be word for the item to be valid. E.g.

So this is the rules that I use:

        { text: /{{\w+}}/gm, style: 'color: green;' },
        { text: /{{.*?}}/gm, style: 'color: red' },
        { text: /{{\w+/gm, style: 'color: green' },

As you can see, the order matters here, and it makes no sense to sort regexes to begin with...

So right now I'm just disabling normalization altogether, by doing this:

HighlightableInput.methods.normalizedHighlights = function dummyNormalizedHighlights() { return this.highlight; };

But of course the above is not the proper, good way to solve this. I personally suggest to sort the rules if and only if all the rules are strings and not something else, in which case just leave them in the order they were written.

In this case you won't break (much) compatibility for those who depend on this sorting with simple string rules, and won't have this unintuitive sorting for those who use regexes and depend on a specific order of rules.

SyedWasiHaider commented 5 years ago

Hi @haimgel I think should be fixed in the last commit (or at least an improvement).