Regression: Click event suddenly stopped working (Reactstrap / Popper.js 1.x)

[sbaechler]

• @testing-library/user-event version: 13.1.1

• Testing Framework and version: Jest 26.6.3.
• DOM Environment: JSDOM 16.4.0, node 12.19.0

Relevant code or config

This code opens a modal that contains some buttons and clicks on them. The modal is provided by Reactstrap (8.9.0).

    it('can discard and apply a filter selection', () => {
        render(
            <TestRootContainer initialState={mockStore}>
                <BookingsFilter />
            </TestRootContainer>,
        );

        // open filter popover
        user.click(
            screen.getByRole('button', {
                name: 'cmb-web.filter.filter-by',
            }),
        );

        // expect filter apply button
        screen.getByRole('button', { name: 'cmb-web.filter.apply' });
        // expect no filters set
        screen.getByText('cmb-ui-core.active-filter-items.no-active-filters');
        // expect no reset button
        expect(screen.queryByRole('button', { name: 'cmb-web.filter.reset' })).toBe(null);

        // select a byType filter
        user.click(screen.getByRole('button', { name: 'cmb-ui-core.account-detail-filter.credit' }));
       //^^^^^^^^^^^^^
       //Here the code execution just stops. The onclick handler on the button is never called.

        // The test failed here because the button does not appear.
        user.click(screen.getByRole('button', { name: 'cmb-web.filter.reset' }));


        // Expect popover to have closed
        expect(screen.queryByRole('button', { name: 'cmb-web.filter.apply' })).toBe(null);

What you did:
This test failed after an upgrade of user-events from 12.6.3 to 13.1.1. It worked again when I downgraded back to 12.6.3.

What happened:
The test failed at the marked spot. Adding async / await did not help. Also using the bound getByRole did not help.

When run with the debugger the context never switched back to the React component after the click on the button. The
flow went straight to the next line in the test.

The test then failed in the next line since the button click handler had not been called the UI never updated.

Reproduction repository:

I'll try to find time to create a minimal repo that can reproduce the issue. But I hope that maybe you have an idea what could be the reason for this regression.

Problem description:
The modal component does register some event listeners on document that close it when the user clicked outside of it.
Could this be the reason?

If it helps, I can go through every release in between and try to see where the regression happened.

[ph-fritsche]

We've added support for pointer-events in v13.1.0.

Could you check the computed pointer-events value for the element that you're trying to click?
Per window.getComputedStyle(element)['pointer-events'] and through developer console of your browser.

[sbaechler]

pointer-events is auto.

[ph-fritsche]

This is strange. Could you collect the styles from the ancestors like:

const computedStyles = []
for (
    let el: Element | null = element;
    el?.ownerDocument;
    el = el.parentElement
  ) {
    computedStyles.push(window.getComputedStyle(el).pointerEvents)
}

Maybe we overlooked some edge case when implementing the helper.

[markivancho]

For me an issue triggers when I'm using DropDown menu from reactstrap;
they are using react-popper old version and there are this line
also there is old issue related to pointer-events and they seems to remove this style in new versions
floating-ui/react-popper#198

Tried to wrap my every click call in act but it didn't help, I'll continue the investigation and share other findings

[markivancho]

My issue was solved by waiting for popper to rerender with the correct styles

[abhinavpreetu]

I am facing the same issue with v13.1.x. I am using react-popper v1.3.6

We've added support for pointer-events in v13.1.0.

Could you check the computed pointer-events value for the element that you're trying to click?
Per window.getComputedStyle(element)['pointer-events'] and through developer console of your browser.

It returns auto in the browser's developer console but returns none when I log it in the test file.

As mentioned by @markivancho , react-popper adds pointer-events: none as initial style to the popper element. Then the element re-renders and updates the value to auto. This creates a race-condition as when the click event is triggered, the value of pointer-events is still none

Adding a waiting time of "0 ms" resolves this issue.
await new Promise(res => setTimeout(() => res(), 0))

But, it's not feasible to add this workaround for multiple failing tests.

[ph-fritsche]

@abhinavpreetu Thanks for clearing this up.

If an element is not clickable when you use userEvent.click, there should be no click event.

If multiple tests of yours require a specific setup, you could refactor this into a setup function that you call in your tests instead of render.

[vicrep]

@ph-fritsche I've also experienced this issue, and even though I fully understand the rationale behind this decision (and think it makes sense to consider no-pointer-events as being non-clickable), I think we could perhaps improve this behaviour.

Similar to how testing-library has the getBy query which will error if the element is not found, and the convenient findBy ( / waitFor) helpers to retry until the element is found, could it make sense to add a behaviour which would throw an error if we try to click a non-clickable element? (i.e. disabled, or no pointer events). (or at a minimum, a console.warning when this occurs)

This would really ease the DX for people dealing with external libraries where you can't easily control this behaviour, animated components, and as well make it way more clear why tests are failing. Right now, it'll just 'no-op' if the element is not clickable, giving the responsibility of raising an error to the next assertion, and thus easily making the cause of the test failure quite obscure.

This would also make it way easier to work around when you have animating components or inflexible libraries: you'd then simply have to wrap your click in

await waitFor(() => userEvents.click(screen.getByRole('button')))

Please let me know what you think and if this deserves its own ticket. I'd also be happy to try contribute this to the project if this is something you'd be interested in.

[ph-fritsche]

@vicrep Yes, throwing an error assuming someone who calls userEvent.click usually expects that element to be clicked is reasonable. Also easy to work around if someone wanted to assert that there is no click.

expect(() => userEvent.click(screen.getByRole('button'))).toThrow()

A PR would be welcome. 😃

[vicrep]

Cool! I'll try get something up next week :)

[vicrep]

As promised :) #647

[beckerei]

It's nice to have errors if elements can't be clicked.
But I can't solve the initial issue.

My issue was solved by waiting for popper to rerender with the correct styles

I tried a number of different ways to do this:

• doing a simple timeout, ranging from 0 to 1000ms
• overwriting pointer-events with auto at the point where I think it must be

•   await waitFor(() =>
      expect(
        hasPointerEvents(container.querySelector('bp3-transition-container')),
      ).toBe(true),
    );

•       await waitFor(() =>
      expect(
        userEvent.click(screen.getByLabelText('Tue Dec 10, 2019')),
      ).not.toThrow(),
    );

As of now I sadly can't upgrade the project away from popper v1, which is used by blueprint.

In the final application the pointer events are set to auto as expected.

I'm pretty much stuck here, the only way I know which "solves" this is by using fireEvent.click instead

@markivancho how did you solve this?

[ph-fritsche]

•       await waitFor(() =>
      expect(
        userEvent.click(screen.getByLabelText('Tue Dec 10, 2019')),
      ).not.toThrow(),
    );

This can not work. expect(fn).toThrow() requires a function as fn. It should just be

  await waitFor(() => userEvent.click(screen.getByLabelText('Tue Dec 10, 2019')))

If your problem goes beyond that, please set up a codesandbox! That's the easiest way for us to help without playing a guessing game ;)

[beckerei]

This can not work. expect(fn).toThrow() requires a function as fn. It should just be

You are right. But both don't work. I try to setup minimal sandbox.

[markivancho]

It's nice to have errors if elements can't be clicked.
But I can't solve the initial issue.

My issue was solved by waiting for popper to rerender with the correct styles

I tried a number of different ways to do this:

• doing a simple timeout, ranging from 0 to 1000ms
• overwriting pointer-events with auto at the point where I think it must be

•   await waitFor(() =>
      expect(
        hasPointerEvents(container.querySelector('bp3-transition-container')),
      ).toBe(true),
    );

•       await waitFor(() =>
      expect(
        userEvent.click(screen.getByLabelText('Tue Dec 10, 2019')),
      ).not.toThrow(),
    );

As of now I sadly can't upgrade the project away from popper v1, which is used by blueprint.

In the final application the pointer events are set to auto as expected.

I'm pretty much stuck here, the only way I know which "solves" this is by using fireEvent.click instead

@markivancho how did you solve this?

@beckerei you can try await new Promise(res => setTimeout(() => res(''), 10)); before clicking
or more correct way (in my opinion) to wait until parent won't have aria-hidden; I had some nesting, but hopefully you'll get the idea

await waitFor(() => {
      expect(getByText('simple text').parentNode?.parentNode?.parentNode).toHaveAttribute('aria-hidden', 'false');
    });

[beckerei]

I tried to work with .toBeVisible().
Awaiting a promise, event with a timeout of 1s did not work.

I'm currently trying to setup a codesandbox, but the limited implementation of jest and testing-lib don't work together.

[ph-fritsche]

Downgrade testing-library/dom to ^7.28 and you should be fine.
If you have own tests depending on jest.useRealTimer , we can just ignore them / work around in your reproduction.

[vicrep]

@ph-fritsche I was thinking, could we consider adding a force option to user events, which bypasses interactivity checks? That way people can still use library for user events instead of falling back to the inferior fireEvent.* , or inconsistently using one or the other. (Again, happy to contribute)

Something like

userEvent.click(element, null, { force: true })

[beckerei]

I did get the codesandbox to work. I will need to free me some time to set this up properly.
I'm not forcing pointer-event: auto in our styles (when node env is test), in context of styled components:

${process.env.NODE_ENV === 'test' && 'pointer-events: auto !important;'}

I still have a few failing tests, I'm not sure all of them are related to this problem.

But I already caught a bug in our tests, which is nice 👍
I would also be willing to contribute, safe you some free time, I'm able to do this during my working hours.

[ph-fritsche]

@vicrep userEvent is all about mimicking the behavior in the browser as close as possible.
I think we should not introduce any flags tampering with this doctrine.

If someone does not want to mimic the browser behavior, userEvent is not the right choice.

[ph-fritsche]

@beckerei Don't forget to share it. Most mistakes aren't unique and easy to spot if you've seen them a couple of times. ;)

[vicrep]

@ph-fritsche I understand where you're coming from, but let me try to explain my perspective.

As previously discussed, the nature of jsdom means we can't deal with actual layout-related assertions, which means that sadly, we'll basically never be able to reliably ensure that what we do on components can be done by a real user in a real browser (in jsdom).

userEvent is all about mimicking the behavior in the browser as close as possible.

I strongly agree with this, but don't think that's contradictory with allowing a force flag. In the end, the caveats with how JSDom work means that sometimes, especially when dealing with external libraries and stylesheets, you really don't have a choice and can't have JSDOM figure out that for instance, "yes, this is clickable because some external css says so, or because we were able to properly measure the layout of the screen and therefore make this element interactive".

When you reach this situation, you're left with two choices:

• hack your component or create complicated set-up / tear down so that userEvent.click allows the click to go through, or
• use fireEvent.click instead

Most people will go for the latter, and then you lose out of the additional behaviour that user-event provides when we do a click, making their tests less representative of a real browser's behaviour.

IMO, this library does two things:

• simulate the real (sequence of) events dispatched by a real browser / user interacting with the element
• ensure that you only dispatch events on elements that can be interacted with, as a real browser would (but again, we're not in a real browser so we can't be sure)

Being able to disable the second aspect, which can quite easily become an issue when you don't own 100% of the UI you're testing, still allows us to benefit from the first aspect, which is what I would even say drives most people to this library in the first place.

In the end, the decision is yours, but thought I'd shed some light on my train of thought, and still think it could be worth considering.

PS: FWIW, Cypress allows the exact same thing, even though the JSDOM argument doesn't hold against it:

• https://docs.cypress.io/guides/core-concepts/interacting-with-elements#Forcing
• https://docs.cypress.io/api/commands/click#Arguments , because sometimes, you sadly do need an escape hatch.

[ph-fritsche]

@vicrep Thanks for your input on this.

We want to provide a consistent API.
What about actions that manipulate the DOM? The pointerdown changes the activeElement. It does not do so if it is e.g. disabled. Will a force flag change the activeElement or not?
What if there is a event.preventDefault() on mousedown. Will we force a mouseup?
We provide methods that might change the DOM like type. Would a force flag manipulate the DOM and how would we define which changes to apply?

For all our changes, we've got a very straight-forward way to decide which is the desired behavior: Do the same thing in a browser. And then we look into if it is possible to mimic that.

If the dev knows better, there is fireEvent and it won't be removed because there are limits and exceptions to the userEvent approach.

I also think this issue is a poor motivation to introduce such changes, as it is just the dev running userEvent.click at the wrong time. If the dev sets up a test scenario that is different from the scenario he wants to test, we could as well just provide a "put a check mark on this test" API ;)

[vicrep]

You bring up some very good points on this, I think doing so would require a little more thought and discussion than "just adding the force flag". Also agree that this issue isn't the right thread and motivation for this, I might open a new ticket if worth a more in-depth proposal, as I still believe it could bring some value.

I wasn't talking about waiting for actionability as a reason for this, but more for when you don't have a choice due to an external library's implementation, or you just want to be pragmatic and not waste time setting up something that you're not directly interested in testing (see example shared below).

Our of curiosity, did you see the Cypress doc link I shared specifically about this? They give a pretty good example of when force can come in handy, even without it being done to pull a VolksWagen on your tests 😛

[ph-fritsche]

I have not seen a valid example for not having a choice due to 3rd party code.
If you're not interested in mimicking the user, there are other tools for this.

Yes, I've seen the Cypress docs. The described behavior for cy.click({force: true}) is what fireEvent.click offers when using testing-library.
(And at the bottom of the page is a nice little alert about some exception to the exception...)

[vicrep]

Okay, looks like there's enough pushback, I'll give up on this proposal :) Thank you for the discussion, appreciated it.

Last thing, unless I'm missing something, I don't think cy.click({force: true}) is the same as fireEvent.click -> if you look at the driver implementation, it looks like they still run all the events when 'forcing' a click (mouse over, mouse down, mouse up).

[vicrep]

And just for clarity, I wasn't trying to change the underlying paradigm / vision of this library, and apologies if that's how it came out. I was coming more from an adoption perspective, by giving as much incentive to consumers to consistently stick to this library as opposed to a hybrid between fireEvent and userEvent.

[beckerei]

I think at the end on has to admit that tests still run in a virtual environment, which is browser like but not a real browser.
There are certain limits in terms of what can be emulated properly. In our case the way popover v1 ends their transition seems to be incomplete (I haven't figured out the exact reason).
In general it's an improvement that userEvent now throws errors instead of failing silently, this gave me to opportunity to identify the root causes of failing tests a lot faster. And I can also follow and agree with the argument not to execute a click in these cases.

While my solution may no be the best, it's okayish. The better solution would most likely be to update the underlying libs.

[ph-fritsche]

@vicrep Don't get me wrong. If you feel strongly about this, a feature request in a separate issue is welcome.
It's just that at the moment I fear it brings a couple of new problems while not really solving one.
If you come up with a consistent policy for such a flag that we could implement across our APIs and it receives positive feedback, I would consider an implementation.

[sbaechler]

@beckerei Are you using a mock for the popper.js module?

I noticed that I have pointer-events: none on the div with the class popover show. And this never changed unless I removed the popper.js mock. But then the tests failed for other reasons (NaN).

[beckerei]

@beckerei Are you using a mock for the popper.js module?

It's indeed mocked. But as you also pointed out, it's mocked for a reason. We currently don't have problems with popper. But most of the tests and impl. haven't been touched since April. I assume we will upgrade to popper v2 in the near future. Not sure what problems we will encounter with it.
I let my colleagues know about this thread (again) as I'm most likely not doing the work. If we figure something out we will share our knowledge.

[acherkashin]

If somebody still has this problem skipPointerEventsCheck option was introduced and can be used in the following way:

userEvent.click(await screen.findByText('Zoom'), {}, { skipPointerEventsCheck: true });

[barretron]

Workaround for version 14.0.0+:

import { screen, waitFor } from '@testing-library/react';
import userEvent, { PointerEventsCheckLevel } from '@testing-library/user-event';

test('workaround for pointer-events caching', async () => {
  // keep helpful pointer-event checks enabled until
  // they start causing errors (e.g. with popper.js + react-bootstrap)
  const user = userEvent.setup();

  // click a button with label 'Open Menu'
  // that opens a menu div with transition
  await user.click(screen.getByText('Open Menu'));

  // wait for div[role=menu] to be interactive
  await waitFor(() => {
    // do a manual check that confirms the popup
    // actually removes its pointer-events style
    expect(
      window.getComputedStyle(screen.getByRole('menu')).pointerEvents
    ).not.toBe('none');
  });

  // create a sub-instance of userEvent, disabling pointerEventsCheck,
  // then click a menu item
  await user
    .setup({ pointerEventsCheck: PointerEventsCheckLevel.Never })
    .click(await screen.findByText('Menu Item'));
});