Skip to content

test: refactor is-focusable tests#3855

Merged
straker merged 6 commits intodevelopfrom
is-focusable-tests
Jan 16, 2023
Merged

test: refactor is-focusable tests#3855
straker merged 6 commits intodevelopfrom
is-focusable-tests

Conversation

@straker
Copy link
Copy Markdown
Contributor

@straker straker commented Jan 11, 2023

Was starting to work on allowing the inert attribute to determine when things were focusable and noticed that there were no test files for is-natively-focusable, inserted-into-focus-order and focus-disabled. Turns out most of the tests were all part of the is-focusable test code so split them out into their own files. focus-disabled didn't have any dedicated tests even inside is-focusable, so had to add a bunch. This also meant that I needed to add focus-disabled to the commons object so I could access it like everything else.

Changes made:

  • copied and pasted the dom.isNativelyFocusable and dom.insertedIntoFocusOrder describes into their own files (with updated es6 syntax for arrow functions and const instead of var)
  • created focus-disabled tests (all new). Will need to verify that they cover all cases
  • added focus-disabled to the commons functions

@straker straker requested a review from a team as a code owner January 11, 2023 23:13
@straker straker changed the title tests: refactor is-focusable tests test: refactor is-focusable tests Jan 11, 2023
Copy link
Copy Markdown
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Couple minor points

assert.isTrue(focusDisabled(vNode));
});

it('returns false if element is inside a legend inside a disabled fieldset', () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

😮 How did you find that one??

assert.isFalse(isNativelyFocusable(el));
});

it('should return false for a div with a tabindex with spaces', () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What spaces?

Copy link
Copy Markdown
Contributor Author

@straker straker Jan 13, 2023

Choose a reason for hiding this comment

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

¯\(ツ)/¯. Seems the tabindex is set to 0 ?

Copy link
Copy Markdown
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Did you forget to push or something?

@straker straker merged commit ec4c9df into develop Jan 16, 2023
@straker straker deleted the is-focusable-tests branch January 16, 2023 15:42
straker added a commit that referenced this pull request Jan 23, 2023
* tests: refactor is-focusable tests

* negative tabindex

* Update test/commons/dom/is-natively-focusable.js

Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>

Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants