Breaking change released as a minor version

[mansona]

Hey folks 👋

I just got hit by the change #1185 actually changing behaviour that has broken a test using floating dependencies: GavinJoyce/ember-headlessui#140

Can we back that PR out, release a patch release and then release it as a major version bump?

[scalvert]

This was not an intentional breaking change, therefore it doesn't really make sense to re-release as a major version. It makes more sense to fix forward.

It looks like @shamrt is taking a look at this. Let's try to get this fixed and re-release as a patch.

[rwjblue]

Ya, like @scalvert mentioned the changes in #1185 should not have been breaking (fireEvent itself was not public API, and AFAICT ember-headlessui was not importing it privately). We need to debug the failure to understand what is actually going on in that test case.

@mansona - Have you debugged it yet?

[shamrt]

I'm debugging, but it appears to be a pernicious issue. I'll report back once I've identified the issue, and will likely fix and create a PR in the process.

[mansona]

@rwjblue no I haven't done any debugging yet. I'm happy to help out but I figured since @shamrt has a reproduction and the full context of the original PR I would take more time ramping up. Although if you wanted to share your work so far @shamrt I might be able to find some time to help debug this 👍

[mansona]

@rwjblue I've finally been able to spend a little bit of time investigating what went wrong. It's a bit tricky for me to explain especially because the PR was squash merged into master 🙈 so I'll have to link to the original commits in the PR and I hope that the links will work as expected 🙃

I did a quick git bisect to identify the exact commit that caused the issue. I guess unsurprisingly it's the commit that actually refactors the click() handler to use this new async structure 5b2c2ae

I was a little bit suspicious about this commit because the focus and click events are relevant to the test that I'm running so I decided to edit the commit to split it out into 6 separate commits (one for each file that changed) and it does in fact seem that it's the change to the click() handler that caused the issue. (I even re-ordered things a few times to see if changing the focus event first would change things but it was always the change to the click event that caused the problem).

Suspecting that there might be a timing issue somewhere (as we are changing the async nature of things in this PR) I added a console log to the fireEvent function to log the element and the eventType and we found a difference!

For some reason the blur and focusOut events are not getting fired before the click event. I haven't tracked down the exact reason why this might be causing the test failure but it represents a clear digression from the previous behaviour. I don't have enough context why this change might cause that behaviour difference but I wonder if maybe we should consider backing out the PR for now until we can figure it out because there is clearly a behaviour change.

What do we think?

[mansona]

Hey has anyone had a chance to look at my investigation yet? If anyone who has more context might be able to point me in some direction with my work that would be much appreciated 👍