Do not assume global HTMLSelectElement

[nathanforce]

What: Do not assume that HTMLSelectElement is defined globally.

Why: Running Node without a shimmed browser environment has no browser globals.

How: Following patterns elsewhere in the code, lookup HTMLSelectElement from the document

Checklist:

• Documentation N/A
• Tests
• Typings N/A
• Ready to be merged
  

I've not added any tests here. I think there may be value in a suite of tests that verifies the library functions in a non-dom environment but didn't want to bloat the PR.

[codecov]

Codecov Report

Merging #550 (7319fe4) into master (390013e) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #550   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          684       684           
  Branches       214       214           
=========================================
  Hits           684       684           

Impacted Files	Coverage Δ
src/select-options.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 390013e...7319fe4. Read the comment docs.

[ph-fritsche]

document.defaultView might be null.

When it yields a window I guess this usually has the HTML elements als properties, but it is not required to do so per specs.
So I guess there could also be environments not implementing window.HTMLSelectElement.

[nathanforce]

I suppose you're right. It feels like that is starting to push the limits of assumptions made by this library and by the testing-library ecosystem. My understanding is that testing library expects to run against a DOM or a DOM-like thing, but not necessarily run in that environment, so assuming the global is not desirable.

Maybe all usage of this "lookup from defaultView" pattern should be updated to safely guard against null. Here is another usage:

user-event/src/utils.js

Lines 259 to 260 in 0d30a79

	(element instanceof element.ownerDocument.defaultView.HTMLInputElement &&
	CLICKABLE_INPUT_TYPES.includes(element.type))

All said, it feels much less likely that somebody would be using the testing-library ecosystem against a DOM that does not implement the spec compared with using it in a pure Node environment. Outside of Jest, most test runners I've seen do not pollute the global with DOM globals.

[ph-fritsche]

but not necessarily run in that environment, so assuming the global is not desirable.

Agreed.

a DOM that does not implement the spec compared with using it in a pure Node environment

My point being that when you have a pure Node environment you use some DOM implementation.
That DOM implementation might go beyond the specs but it does not violate them when it does not provide the HTMLElement constructors as properties of window.

Maybe all usage of this "lookup from defaultView" pattern should be updated to safely guard against null.

I think so.

I think this whole "lookup from defaultView" is flawed because it assumes that the element constructor is on the window
So I'm not sure if it is any safer than assuming the global.

Outside of Jest, most test runners I've seen do not pollute the global with DOM globals.

The clean solution might be to let the user plug in a callback to verify the interface.
Defaulting to a helper which uses fallbacks:

1. element.ownerDocument.defaultView
2. global
3. element.tagName ?

So the vast majority of users is fine and those using exotic environments can plug in a workaround.

Could be implemented in @testing-libary/dom.

[nathanforce]

@ph-fritsche How would you suggest we proceed? Without this safety check I cannot use the latest release of this package.

Overall I agree there should be a system wide solution for ensuring the library is runnable against all dom-like environments.

I think this whole "lookup from defaultView" is flawed because it assumes that the element constructor is on the window
So I'm not sure if it is any safer than assuming the global.

In theory you may be right, but in practice I think that a very strong majority of users will be using JSDOM, so my changes here are safer than what exists now, but agree there is room for improvement.

As this change conforms with existing solutions within the codebase I propose we proceed in order to avoid runtime errors and then take the time to solve the larger issue.

[ph-fritsche]

@nathanforce I added a helper that should solve your problem in #552.
If testing-library/dom-testing-library#885 gets merged, we can remove the code from this library once the peerDependency is bumped and resolve any problems that might come up with different environments there.

[ph-fritsche]

@nathanforce Your problem is fixed in v12.6.3 🎉