Support arbitrary selectors for targets

[joshjordan]

Cause why not? For my team, this was the expected behavior and hopscotch does not currently throw a useful error message when it does not get what it expects. We were confused when encountering exceptions when given a perfectly valid selector, especially since the code that calls this handles multiple matches gracefully.

This implementation passes all tests that are currently passing in master, and preserves backwards compatibility for existing calls to getStepTargetHelper.

[timlindvall]

This change looks good to me. Makes sense that these sorts of selectors can be supported... we can make the presumption that tour authors won't pick selectors like "div" that'll return a lot of potentially unintended matches (FWIW, you could end up in the same scenario with classes anyway, in which case Hopscotch just picks the first match).

[joshjordan]

@zimmi88 that was my thought as well. They are also likely to test their implementation, even at a cursory level, to vet selector issues.

[kate2753]

It seems that the only thing you need to do to allow arbitrary selectors is to remove this if statement.

[joshjordan]

That's not true - that would break cases where someone had passed for use in document.getElementById (that is, an id that did not begin with "#"). If we simply removed this if, then you'd return document.querySelector(target), which would be null in the case of an id parameter.

[kate2753]

This regex makes sure that selector begins with # or .. So you should not have any ids that did not begin with #. This change basically makes it so any string will be treated like an id selector. This seems confusing to me. Now we support both CSS style selectors and random string that will be treated like IDs.

[kate2753]

I agree, having arbitrary selectors provides more options. This is definitely something I would like Hopscotch to support.

[gkoo]

Yeah, this was kind of an arbitrary restriction that I put in when I wrote it, mainly because I was imagining people putting in selectors such as "div" or "p" and found that kind of silly. That being said, I do think it'd be perfectly fine to let tours define targets with arbitrary selectors. They probably know what they're doing.

The fastest way to see this added is to submit a PR -- I don't have much time to look at these issues anymore, but would be happy to review.

[gkoo]

Whoops, did not realize that I was commenting within a PR! My bad. (Thought it was an issue.) I will take a look soon.

[joshjordan]

@kate2753 I can provide some additional unit tests to prove out the backwards compatibility, if you like, and then we can use them to iterate on implementation.

[gkoo]

This looks fine to me. The only concern to me was confusing an id for a querySelector, which @joshjordan has accounted for. Thanks for the PR, Josh!

[kate2753]

I think we should support CSS style selectors only. And the logic of this function should be:

1. If there is jQuery, use jQuery. jQuery supports more complex selectors than document.querySelector. If you have jQuery on the page why not use it?
2. If there is Sizze, use sizzle
3. If there is document.querySelector use it.
4. If selector starts with # use document.getElementById(target.substring(1))
   And perhaps optional 5) If selector starts with . use document.getElementsByClassName and pick first match

This should be backward compatible and more efficient, since we do only one document.getElementById lookup.

[gkoo]

Sounds mostly fine to me, with two exceptions.

1. What about targets that are just id strings? e.g. target: "my-id" Maybe this was a questionable decision on my part to support a target of this format earlier on, since it sort of complicates refactoring this function now.
2. If the target is a query selector string such as "#my-div-id .inner-element" or ".my-div-class .inner-element" then (4) and (5) won't behave as intended.

[kate2753]

Doh! I've missed original line 365 where we did do another document.getElementById just for the passed in string. Without that it seemed that this function would return null if selector string did not start with # or .. Now this implementation makes sense.

If we want to continue to be backward compatible, we'll have to stick with this implementation. It seems a bit inefficient to run document.getElementById for every target even if we end up using jQuery or sizzle. I do not see a way around it though.

I would still swap the order of jquery and document.querySelector. Also, we can probably eliminate second document.getElementById. This seems to be backward compatible:

var idSelector = (/^#/.test(target)) ? target.substring(1) : target,
  result = document.getElementById(idSelector);
if (result) {
  return result;
}
if (hasJquery) {
  result = jQuery(target);
  return result.length ? result[0] : null;
}
if (Sizzle) {
  result = new Sizzle(target);
  return result.length ? result[0] : null;
}
if (document.querySelector) {
  return document.querySelector(target);
}

After giving it some thought document.getElementsByClassName does not appear to be very useful here. There likely will be multiple elements with the same class name, and just selecting by class name won't be useful to find the element you need.