testing-library / user-event

🐕 Simulate user events
https://testing-library.com/user-event
MIT License
2.18k stars 245 forks source link

userEvent.tab #167

Closed benmonro closed 4 years ago

benmonro commented 5 years ago

Add the ability to simulate a user tabbing through a UI for RTL: userEvent.tab(direction)

more of a question to see if you guys would be open to me opening a PR for userEvent.tab from what I've read in threads on RTL and JSDOM, the ability to tab is not really supported by JSDOM. However, I came up with a working solution that simulates tabbing. I'd love to open a PR and contribute here, but before I do wanted to check to make sure you guys would be open to it.

alexkrolick commented 5 years ago

Would this be like click where it takes a target element? So it would be like "press tab as many times as needed to get from the current focus to the target"?

benmonro commented 5 years ago

my thought is that this would just be a generic tab key event. My first pass (not accounting for direction) will be based on this code (though I will add more to it for the purposes of user-event)

export function fireTabKeyPress() {
    const focusableElements = document.querySelectorAll('input, button, select, textarea, a, [tabindex]');
    const list = Array.prototype.filter.call(focusableElements, function (item) { return item.getAttribute('tabindex') !== '-1'; });
    const index = list.indexOf(document.activeElement);
    const next = list[index + 1] || list[0];

    if (next.getAttribute('tabindex') === null) {
        next.setAttribute('tabindex', '0'); // jsdom requires tabIndex=0 for an item to become 'document.activeElement' (the browser does not)
    }
    next.focus();
}

you can then write tests like this:

const {getByText} = render(<MyComponent />);

userEvent.tab();

expect(document.activeElement).toBe(getByText("button 1"));

userEvent.tab();

expect(document.activeElement).toBe(getByText("button 2"));
benmonro commented 5 years ago

@alexkrolick would love to get some feedback on my PR. Do you think it can be merged?

Gpx commented 5 years ago

Hey @benmonro 👋 I don't know if you saw my comment https://github.com/testing-library/user-event/issues/156#issuecomment-521591344 I think this feature looks quite complex to implement correctly. Could you explain why you think it's worth adding? I write a lot of tests with RTL, but I never felt the need to have a tab event.

benmonro commented 5 years ago

@gpx I didn't see that, thanks for getting back to me.

I believe it's important to support a11y for keyboard only users. I live by the guiding principle of testing library:

We try to only expose methods and utilities that encourage you to write tests that closely resemble how your web pages are used.

since KB only users will tab through elements in your pages/components, I'm of the opinion that writing tests in this way is very important.

The implementation I put together in #169 is working for us at Walmart Labs. Would love to have you poke holes in that and let me know if there are any shortcomings, etc.

alexkrolick commented 5 years ago

@benmunro I don't have too much context on this codebase. The implementation seems good to me.

To @Gpx's point, I am not sure on the scope of user-event. The closest thing is probably PhantomJS (discontinued 2018), which did have the goal of emulating browser behavior for user interactions rather than plain event model of jsdom: https://phantomjs.org/api/webpage/method/send-event.html

So I think there is kind of a gap there between Cypress and JSDOM and right now the click/type interactions are simulating more of what browsers would do. One question is this: should there be a method just for the tab key or should it be baked into the keypress event handling? Should we also handle other key events, like enter to submit forms?

Overall I don't think we should totally defer to "headful" browsers for this stuff because the overhead is pretty high and making smaller, scoped tests is much easier when they live next to the rest of the tests for a component.

benmonro commented 5 years ago

@alexkrolick I did consider handling this like other key events, but there was one problem I ran into which was that the other events require an element as their 1st parameter, but I wanted to differentiate this event from those for 2 reasons. 1) when there is no element focused, it doesn't make sense to give it a target, as the user doesn't do that either. 2) to avoid confusion with keyboard events for entering a tab character into an input field or textarea.

I definitely appreciate the notion to have this go into the higher level e2e tests, but I would still contend that these user flows can be tested in integration tests ala RTL.

alexkrolick commented 5 years ago

when there is no element focused, it doesn't make sense to give it a target, as the user doesn't do that either

I think the target would be document.activeElement

This way when entering a tab character into an input field or textarea, it wouldn't be confused with that event

Isn't that a common source of focus traps? Can you still test that you don't have focus traps when the input bypasses the field?

benmonro commented 5 years ago

fair points/questions. I could try and play with integrating it w/ the keypress events and see how it shakes out w/ document.activeElement

I can also try a test w/ a focus trap.

If I do that can we merge? Not wanting to go through that if those issues get addressed and we end up not merging. :)

alexkrolick commented 5 years ago

@Gpx @kentcdodds ?

kentcdodds commented 5 years ago

I'd love for this to work. I'm unsure whether it can be implemented correctly, but if it can then I think it'd be a great addition.

benmonro commented 5 years ago

@kentcdodds would love to hear more details on your concerns/thoughts around doing it correctly. anything beyond the current implementation (in #169) + proposed changes to make it part of the keyPress events / support focus traps?

kentcdodds commented 5 years ago

I seem to recall that Cypress doesn't support tab because it's "really hard to get right" or something. I know those folks are smart so I just assumed they knew more than I do and that it would be hard to get right.

Gpx commented 4 years ago

Just released 🎉