Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: account for sorting bug in node 10 #205

Merged
merged 2 commits into from Jan 3, 2020
Merged

fix: account for sorting bug in node 10 #205

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2accff8
fix: account for sorting bug in node 10
Jan 3, 2020
7acdf84
just one sort regardless of node version
Jan 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 25 additions & 1 deletion __tests__/react/tab.js
View file
Open in desktop
Expand Up @@ -157,7 +157,8 @@ describe("userEvent.tab", () => {
expect(link).toHaveFocus();
});

it("should stay within a focus trab", () => {

it("should stay within a focus trap", () => {
const { getAllByTestId, getByTestId } = render(
<>
<div data-testid="div1">
Expand Down Expand Up @@ -219,4 +220,27 @@ describe("userEvent.tab", () => {
// cycle goes back to first element
expect(checkbox2).toHaveFocus();
});

// prior to node 11, Array.sort was unstable for arrays w/ length > 10.
// https://twitter.com/mathias/status/1036626116654637057
// for this reason, the tab() function needs to account for this in it's sorting.
// for example under node 10 in this test:
// > 'abcdefghijklmnopqrstuvwxyz'.split('').sort(() => 0).join('')
// will give you 'nacdefghijklmbopqrstuvwxyz'
it("should support unstable sorting environments like node 10", () => {
const letters = 'abcdefghijklmnopqrstuvwxyz';

const { getByTestId } = render(<>
{letters.split('').map(letter => <input key={letter} type="text" data-testid={letter} />)}
</>)

expect.assertions(26);


letters.split('').forEach(letter => {
userEvent.tab();
expect(getByTestId(letter)).toHaveFocus();
})

})
});
31 changes: 23 additions & 8 deletions src/index.js
View file
Open in desktop
@@ -1,7 +1,7 @@
import { fireEvent } from "@testing-library/dom";

function wait(time) {
return new Promise(function(resolve) {
return new Promise(function (resolve) {
setTimeout(() => resolve(), time);
});
}
Expand All @@ -12,6 +12,23 @@ function findTagInParents(element, tagName) {
return findTagInParents(element.parentNode, tagName);
}

let sortListByTabIndex;
let nodeVersion = +process.version.slice(1).split('.')[0];
if (nodeVersion < 11) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way we can make this consistent regardless of node version? I'm not super thrilled about the added complexity here 😬

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I thought about that but it would mean an additional map over the entire list. Felt like it would be punishing node 12 for the sins of node 10. Also didn't think it warranted adding a separate sort just because of this. As always open to other options I might be missing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be worried about perf at this stage... We're paying the price of complexity for an unnoticeable perf gain 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alrighty. pushed a version that just has a single sort @kentcdodds

sortListByTabIndex = list => list.map((el, idx) => ({ el, idx }))
.sort((a, b) => {
const tabIndexA = +a.el.getAttribute("tabindex");
const tabIndexB = +b.el.getAttribute("tabindex");
return tabIndexA < tabIndexB ? -1 : tabIndexA > tabIndexB ? 1 : a.idx - b.idx;
}).map(({ el }) => el);
} else {
sortListByTabIndex = list => list.sort((a, b) => {
const tabIndexA = +a.getAttribute("tabindex");
const tabIndexB = +b.getAttribute("tabindex");
return tabIndexA < tabIndexB ? -1 : tabIndexA > tabIndexB ? 1 : 0;
});
}

function clickLabel(label) {
fireEvent.mouseOver(label);
fireEvent.mouseMove(label);
Expand Down Expand Up @@ -232,15 +249,13 @@ const userEvent = {
const focusableElements = focusTrap.querySelectorAll(
"input, button, select, textarea, a[href], [tabindex]"
);
const list = Array.prototype.filter
.call(focusableElements, function(item) {
let list = Array.prototype.filter
.call(focusableElements, function (item) {
return item.getAttribute("tabindex") !== "-1";
})
.sort((a, b) => {
const tabIndexA = a.getAttribute("tabindex");
const tabIndexB = b.getAttribute("tabindex");
return tabIndexA < tabIndexB ? -1 : tabIndexA > tabIndexB ? 1 : 0;
});

list = sortListByTabIndex(list);

const index = list.indexOf(document.activeElement);

let nextIndex = shift ? index - 1 : index + 1;
Expand Down