Could you provide a repro that we can test in jsdom and a real browser. We need to verify this doesn't pollute actual browser tests and why this fix can't be applied upstream i.e. to jsdom.

@eps1lon, I'm happy to do that, but can you clarify what exactly you're looking for? 😅 I'm not sure I entirely understand.

Hi @VinceMalone could you please rebase this branch with master?

@eps1lon I added a check to see if window.TransitionEvent is not a function before monkey patching the fired event — this should prevent any issues with pre-existing working environments.

Yeah, I'm not comfortable with monkey patching incomplete environments. This can become super confusing if other libraries don't do that.

Why does a polyfill like https://codesandbox.io/s/jsdom-polyfill-transitionevent-bgfz1 doesn't work for you?

The polyfill works, but this change might still be worth it for the sake of others. There's a silent failure when an event type isn't supported — because window.Event is used as a backup. So in this case, it's not obvious why the transitionEventInit properties aren't working, despite fireEvent.transitionEnd() being available to use and jsdom being listed as a "supported" environment. This is what drove me to dig into the source and eventually create this PR.

I definitely get that monkey patching this isn't really ideal, but is it worth doing (like DataTransfer) until jsdom supports window.TransitionEvent?

I definitely get that monkey patching this isn't really ideal, but is it worth doing (like DataTransfer) until jsdom supports window.TransitionEvent?

I consider monkey-patching DataTransfer a mistake as well. Others seem to disagree.

fireEvent.transitionEnd silently working is just as problematic as polyfilling the properties silently. It'll just fail at a different point e.g. when assuming that changing a CSS property triggers a transition. This is the reason JSDOM doesn't polyfill the event but rather waits for a full implementation of the spec.

I don't believe we're actually improving the problem. Rather we're just kicking the problem to someone else by diverging even more. Before we had JSDOM vs browsers. Now we have JSDOM vs browsers vs testing-library in JSDOM. By using testing-library you're tied to a special environment that's more and more diverging from existing environments and I don't think this is the responsible thing to do.

So yeah, ultimately we should not fall back to window.Event. It's a slippery slope we're on so I'm going to repeat the same argument on every PR.

So in this case, it's not obvious why the transitionEventInit properties aren't working, despite fireEvent.transitionEnd()

That's a bit debateable because in the end you're getting an Event not TransitionEvent. I understand that this isn't obvsious because you receive it in a transitionend event handler but some hint does exist.

I I just don't have a good feeling about these monkey-patches since we have to maintain a very specific use case ("support a partial implementation of the Transition spec") which leaves us open to "how much should we fix in testing-library?" debates.

I'd be way more comfortable if we had a package like jsdom-event-polyfill that people would explicitly opt-in and where we could explain potential draw-backs.

@eps1lon those are all really good points; I definitely understand the urge to resist going down this path. It's tough to balance the pros can cons of existing decisions (falling back to window.Event or monkey patching DataTransfer), silent errors, and accommodating a wider pit of success. Until then, I'm happy to get onboard with whatever you decide 👍

This comment is to help others who might stumble on that issue

If anyone is looking for a polyfill or something, here's how we've managed to make it work with jest in our codebase.

hello-pangea/dnd@5ec5a74#diff-3ab6990a471fa18593a64b7c0de508ff724cd1cd0676d882d475fb40fe624660=

For Vitest I took the approach of addEventListener to get it working. I add the property on the capture phase of the event so that when onTransitionEnd fires we have the propertyName. This will probably also work with Jest.

I've created a codesandbox to show it working with tests/fireEvent.