Remove mystical incantations related to selectedIndex

Description

This line is an example of programming by mystical incantation:

// Accessing this property makes selected-by-default
// options in Safari work properly
if ( elem.parentNode ) {
    elem.parentNode.selectedIndex;
}

And there is at least one more just like it. Logically, the expression is a no-op. The comment indicates that somebody once saw it cause some side effect in an unspecified version of Safari.

For this reason, no lint will pass such a line. This comment confirms that our lint was forced to ignore the transgression.

// eslint rule "no-unused-expressions" is disabled for this code

If a fuse blows, you don’t put tinfoil between the contacts; it’s trying to tell you that something is wrong.

Furthermore, what makes us think that the parent of an OPTION will be a SELECT? Could be an OPTGROUP.

As this issue has the potential to foul up form input, it should be given a high priority.

I’ll leave the solution to this one as an exercise for the moment. A solution was given to Resig several years ago and discussed in the old jQuery forum, as well as on Usenet. He mentioned something about not wanting to add an extra line and confirmed that he couldn’t find a browser that “proved” it didn’t work (i.e. show me where it fails). Hint: the no-op line has the answer and discards it. 😉

Link to test case

No test case. Not known for sure where or when it might fail. Do know that SELECT elements without at least one OPTION containing a SELECTED attribute are vulnerable (as that creates an implicitly selected OPTION).

Likely not an issue once the page is loaded, but without reviewing the DOM standards, can’t say for sure when browsers are required to set the selected property to reflect an implicit selection. Instead of worrying about that (or testing), let’s just fix the logic.

PS. There is an attempt at a feature test that sets a flag that isn’t checked in relation to the above line, but it is checked in other areas of the code with similar lines:

var input = document.createElement( "input" ),
		select = document.createElement( "select" ),
		opt = select.appendChild( document.createElement( "option" ) );

	...

	// Support: IE <=11 only
	// Must access selectedIndex to make default options select
	support.optSelected = opt.selected;

One problem with that test is that it is testing a disembodied SELECT and there’s certainly no guarantee that it will pass based on any standard that I know of (again, would have to consult the specifications). So false positives for this “problem” will likely occur, adding additional no-op execution for no reason at all. I know that jQuery is supposed to work with document fragments, but the SELECT tested is without even that.

Another problem is it doesn’t test the assumed solution to see if it improves the situation, only that there may be a problem. Again, this will result in no-op execution without any reason to expect it will do anything useful.

A third problem is that the name of the flag (optSelected) is not very descriptive of what it is supposed to mean. If it wasn’t tacked on to this support object, we could make the name as long as we want as the minification process would reduce it. Regardless, saving a few bytes at the expense of clarity is a bad trade (not to mention that HTTP compression will likely end up saving the bytes anyway).

If we just fix the logic for the half dozen related lines then we don’t need this test at all. And BTW, what does “Support: IE <= 11 only” imply? See similar comments throughout and they don’t make a lot of sense to me (e.g. the original hack in this case was for some unspecified Safari version).

Author: Fantashit

1 thought on “Remove mystical incantations related to selectedIndex

  1. Just guessing that is what is meant by the “not in our current codebase” remark.

    It is what i meant and this is still not a sizzle repo. Sizzle and jquery are different projects, these differences are important, in this case this is important because we are planning to remove sizzle dep entirely.

    Some browsers have been known to delay setting the selected property of the implicitly selected OPTION. It’s a very old problem and typically isn’t an issue unless trying to reference this property before the document is ready.

    You should start from that, but with the test-case on jsbin/jsfiddle that’s what others were trying to tell you.

    First of all, lose the indirect feature test as it is using a disembodied SELECT. Second, get rid of the set hack as once we set the property, it is clearly no longer an issue. And third, get rid of the get hack as well as it simply discards the answer.

    And that’s what you should do in pull requests with new tests and without falling old tests.

    the code you linked to is the same nonsense that I discussed with Resig and others several years back (and cited here again). Nothing has changed, although if you look carefully, the hack is not even applied evenly (as I also mentioned above)…

    And that’s what should be avoided. Since important information tend to get lost if you supply it with philosophy, irrelevant stories and etc.

    See more on https://contribute.jquery.org/

Comments are closed.