[refactor] Limit the condition of the option tag to selected attribute in isFormAttribute()

[kfule]

This PR limits the evaluation of whether a tag is option to only when setting the selected attribute.

Description

This PR changes the condition of vnode.tag === "option" && vnode.dom.parentNode === activeElement(vnode.dom) within isFormAttribute() to be like attr === "selected" && vnode.tag === "option" && vnode.dom.parentNode === activeElement(vnode.dom). The only change to the code is addition of a parenthesis.

The condition of the option tag is intended to fix #1916 and should be evaluated in conjunction with the selected attribute. This commit will improve code readability a bit, reduce the number of times to evaluate tags and also reduce the number of times to set the same attributes in option elements redundantly.

This PR is expected to improve performance when many option elements with attributes are listed.
(There are not many attributes for option elements that can be used other than selected, though.)

Motivation and Context

I was wondering about isFormAttribute()'s evaluation of option tag without an attribute. I looked into the code history of this condition, and found that it was for selected attributes, and that the parentheses were maybe simply forgotten.

How Has This Been Tested?

• npm run test

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project.
• I have added tests to cover my changes.
• All new and existing tests passed.
• My change requires a documentation update, and I've opened a pull request to update it already:
• I have read https://mithril.js.org/contributing.html.

[kfule]

(memo) Although I think this current PR is still meaningful, isFormAttribute() may have room for further consideration.

1. The issue Select box options not matching state on select change event #1916 still exists when using optgroup. (flems)
   The check if the parent node is active may be rather unchecked.

2. The check to see if an element with the selected attribute is active has existed since isFormAttribute() was first implemented. (d537153#diff-f50a6d08fa58d22d2e71df97ae8fc5727ea53bc49c69f457ef12bc6df3c6870bR386)
   Personally, it seems a bit strange that only elements with selected are checked to see if they are active. In particular, as commented in Select box options not matching state on select change event #1916, since selected is an attribute of the option element, so elements with selected are not active.
   It is possible that the code wanted to check whether an element is active for other attributes as well, as in the following.

(attr === "value" || attr === "checked" || attr === "selectedIndex" || attr === "selected") && vnode.dom === $doc.activeElement

---

Therefore, based on 1., 2. above, it may be a good idea to simply remove the check for whether an element is active, as follows

function isFormAttribute(attr) {
	return attr === "value" || attr === "checked" || attr === "selectedIndex" || attr === "selected"
}

With this change, npm run test still passes as is at least.

---

Edit: The fix for the optgroup in the other code part also seemed to eliminate the condition of whether the parent node was active. (The same idea may or may not be apply.)
fd484f9 (#1413)
(Also, this anti-blink part may be a problem specific to very old versions of Chrome. If it still happens now, I wonder if other form attributes are ok.)

Edit: blink-repro for selected flems on android chrome
It seems that setting top option.selected to false when select.selectedIndex===0 causes a blink. Since this only occurs under limited conditions and with somewhat inappropriate implementations, it seems better to leave it to the appropriate implementation in userland.

[kfule]

The test for #1916 was left as FIXME, so the test was fixed to take into account DomMock's limitations.
If code is to be changed as commented above, this test needs to be changed as well.

[dead-claudia]

I have a nagging feeling that was a mistake, but I'm not sure by who.

Edit: I see you made the same assessment.

[dead-claudia]

(memo) Although I think this current PR is still meaningful, isFormAttribute() may have room for further consideration.

[...]

I'd like to see some HTML spec digging to confirm that.

And if we do reduce it to just strings, we should just reduce the whole thing to a static array of strings we just .indexOf(key) >= 0.

[kfule]

@dead-claudia
Thanks for the merging and for your comments!
I too think further research is needed to make it just strings.