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.

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

feat: add toBePartiallyChecked matcher #249

Merged
merged 2 commits into from May 19, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
48 changes: 47 additions & 1 deletion README.md
Expand Up @@ -46,7 +46,6 @@ clear to read and to maintain.
<!-- START doctoc generated TOC please keep comment here to allow auto update -->
<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->


- [Installation](#installation)
- [Usage](#usage)
- [Custom matchers](#custom-matchers)
Expand All @@ -69,6 +68,7 @@ clear to read and to maintain.
- [`toHaveValue`](#tohavevalue)
- [`toHaveDisplayValue`](#tohavedisplayvalue)
- [`toBeChecked`](#tobechecked)
- [`toBePartiallyChecked`](#tobepartiallychecked)
- [`toHaveDescription`](#tohavedescription)
- [Deprecated matchers](#deprecated-matchers)
- [`toBeInTheDOM`](#tobeinthedom)
Expand Down Expand Up @@ -895,6 +895,52 @@ expect(ariaSwitchUnchecked).not.toBeChecked()

<hr />

### `toBePartiallyChecked`

```typescript
toBePartiallyChecked()
```

This allows you to check whether the given element is partially checked. It
accepts an `input` of type `checkbox` and elements with a `role` of `checkbox`
with a `aria-checked="mixed"`, or `input` of type `checkbox` with
`indeterminate` set to `true`

#### Examples

```html
<input type="checkbox" aria-checked="mixed" data-testid="aria-checkbox-mixed" />
<input type="checkbox" checked data-testid="input-checkbox-checked" />
<input type="checkbox" data-testid="input-checkbox-unchecked" />
<div role="checkbox" aria-checked="true" data-testid="aria-checkbox-checked" />
<div
role="checkbox"
aria-checked="false"
data-testid="aria-checkbox-unchecked"
/>
<input type="checkbox" data-testid="input-checkbox-indeterminate" />
```

```javascript
const ariaCheckboxMixed = getByTestId('aria-checkbox-mixed')
const inputCheckboxChecked = getByTestId('input-checkbox-checked')
const inputCheckboxUnchecked = getByTestId('input-checkbox-unchecked')
const ariaCheckboxChecked = getByTestId('aria-checkbox-checked')
const ariaCheckboxUnchecked = getByTestId('aria-checkbox-unchecked')
const inputCheckboxIndeterminate = getByTestId('input-checkbox-indeterminate')

expect(ariaCheckboxMixed).toBePartiallyChecked()
expect(inputCheckboxChecked).not.toBePartiallyChecked()
expect(inputCheckboxUnchecked).not.toBePartiallyChecked()
expect(ariaCheckboxChecked).not.toBePartiallyChecked()
expect(ariaCheckboxUnchecked).not.toBePartiallyChecked()

inputCheckboxIndeterminate.indeterminate = true
expect(inputCheckboxIndeterminate).toBePartiallyChecked()
```

<hr />

### `toHaveDescription`

```typescript
Expand Down
124 changes: 124 additions & 0 deletions src/__tests__/to-be-partially-checked.js
@@ -0,0 +1,124 @@
import {render} from './helpers/test-utils'

describe('.toBePartiallyChecked', () => {
test('handles input checkbox with aria-checked', () => {
const {queryByTestId} = render(`
<input type="checkbox" aria-checked="mixed" data-testid="checkbox-mixed" />
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, I don't think we should support aria-checked in a <input type="checkbox" />. It should be supported only on non-native checkbox elements with role="checkbox".

The reason being that a <input type="checkbox" /> won't convey to the user visually that it is partially checked. The only way the native checkbox can be partially checked is when its indeterminate property is set to true.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the same when I tested it in the broswer, as you say indeterminate set to true has a visual effect on the checkbox:

image

The only thing that made me change my mind is that in the accessibility consoleboth of them had role checkbox and checked mixed, but yeah, I think it should be better to check for input+indeterminate and role checkbox + aria mixed, let me change it

Copy link
Member

@gnapse gnapse May 12, 2020

Choose a reason for hiding this comment

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

in the accessibility console both of them had role checkbox and checked mixed

Would you care to share more about this detail?

I'm also particularly interested to know about the input checkbox with indeterminate = true. How is it conveyed in the accessibility console.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both inputs show the same data in the Accessibility tab in the Chrome Devtools:

Input with indeterminate:

image

Input with aria-checked="mixed":

image

You can check the Codesandbox demo I've been using.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, my bad then. If the <input type="checkbox" aria-checked="mixed" /> shows as checked: mixed even if indeterminate was not set true, then I think it was ok the first time around, and I assumed without checking what you correctly went out to check.

What do you think?

If you agree, can you easily revert the changes after my earlier review? Sorry for having made you work extra for nothing.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I was also asking for your opinion here. But I'm conflicted. The <input type="checkbox" aria-checked="mixed" /> looks visually as if unchecked, but semantically (from the aria perspective) it is still partially checked.

I'm leaning towards trusting the accessibility info. If someone uses an input checkbox with aria-checked="mixed" they would presumably take over the rendering to reflect that using icons and stuff.

So yes, in the end I'm leaning to that (after having thought about it out loud here 😄)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought the same at first, since it looks unchecked, but then I thought it's just a matter of how chrome styles the checkbox.

Let's support both of them then :)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so the last commit needs to be reverted, right? It was good the first time around, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll revert it and change only the check inside isValidAriaElement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be good now :)

<input type="checkbox" checked data-testid="checkbox-checked" />
<input type="checkbox" data-testid="checkbox-unchecked" />
`)

expect(queryByTestId('checkbox-mixed')).toBePartiallyChecked()
expect(queryByTestId('checkbox-checked')).not.toBePartiallyChecked()
expect(queryByTestId('checkbox-unchecked')).not.toBePartiallyChecked()
})

test('handles input checkbox set as indeterminate', () => {
const {queryByTestId} = render(`
<input type="checkbox" data-testid="checkbox-mixed" />
<input type="checkbox" checked data-testid="checkbox-checked" />
<input type="checkbox" data-testid="checkbox-unchecked" />
`)

queryByTestId('checkbox-mixed').indeterminate = true

expect(queryByTestId('checkbox-mixed')).toBePartiallyChecked()
expect(queryByTestId('checkbox-checked')).not.toBePartiallyChecked()
expect(queryByTestId('checkbox-unchecked')).not.toBePartiallyChecked()
})

test('handles element with role="checkbox"', () => {
const {queryByTestId} = render(`
<div role="checkbox" aria-checked="mixed" data-testid="aria-checkbox-mixed" />
<div role="checkbox" aria-checked="true" data-testid="aria-checkbox-checked" />
<div role="checkbox" aria-checked="false" data-testid="aria-checkbox-unchecked" />
`)

expect(queryByTestId('aria-checkbox-mixed')).toBePartiallyChecked()
expect(queryByTestId('aria-checkbox-checked')).not.toBePartiallyChecked()
expect(queryByTestId('aria-checkbox-unchecked')).not.toBePartiallyChecked()
})

test('throws when input checkbox is mixed but expected not to be', () => {
const {queryByTestId} = render(
`<input type="checkbox" aria-checked="mixed" data-testid="checkbox-mixed" />`,
)

expect(() =>
expect(queryByTestId('checkbox-mixed')).not.toBePartiallyChecked(),
).toThrowError()
})

test('throws when input checkbox is indeterminate but expected not to be', () => {
const {queryByTestId} = render(
`<input type="checkbox" data-testid="checkbox-mixed" />`,
)

queryByTestId('checkbox-mixed').indeterminate = true

expect(() =>
expect(queryByTestId('input-mixed')).not.toBePartiallyChecked(),
).toThrowError()
})

test('throws when input checkbox is not checked but expected to be', () => {
const {queryByTestId} = render(
`<input type="checkbox" data-testid="checkbox-empty" />`,
)

expect(() =>
expect(queryByTestId('checkbox-empty')).toBePartiallyChecked(),
).toThrowError()
})

test('throws when element with role="checkbox" is partially checked but expected not to be', () => {
const {queryByTestId} = render(
`<div role="checkbox" aria-checked="mixed" data-testid="aria-checkbox-mixed" />`,
)

expect(() =>
expect(queryByTestId('aria-checkbox-mixed')).not.toBePartiallyChecked(),
).toThrowError()
})

test('throws when element with role="checkbox" is checked but expected to be partially checked', () => {
const {queryByTestId} = render(
`<div role="checkbox" aria-checked="true" data-testid="aria-checkbox-checked" />`,
)

expect(() =>
expect(queryByTestId('aria-checkbox-checked')).toBePartiallyChecked(),
).toThrowError()
})

test('throws when element with role="checkbox" is not checked but expected to be', () => {
const {queryByTestId} = render(
`<div role="checkbox" aria-checked="false" data-testid="aria-checkbox" />`,
)

expect(() =>
expect(queryByTestId('aria-checkbox')).toBePartiallyChecked(),
).toThrowError()
})

test('throws when element with role="checkbox" has an invalid aria-checked attribute', () => {
const {queryByTestId} = render(
`<div role="checkbox" aria-checked="something" data-testid="aria-checkbox-invalid" />`,
)

expect(() =>
expect(queryByTestId('aria-checkbox-invalid')).toBePartiallyChecked(),
).toThrowError(
'only inputs with type="checkbox" or elements with role="checkbox" and a valid aria-checked attribute can be used with .toBePartiallyChecked(). Use .toHaveValue() instead',
)
})

test('throws when the element is not an input', () => {
const {queryByTestId} = render(`<select data-testid="select"></select>`)
expect(() =>
expect(queryByTestId('select')).toBePartiallyChecked(),
).toThrowError(
'only inputs with type="checkbox" or elements with role="checkbox" and a valid aria-checked attribute can be used with .toBePartiallyChecked(). Use .toHaveValue() instead',
)
})
})
2 changes: 2 additions & 0 deletions src/matchers.js
Expand Up @@ -16,6 +16,7 @@ import {toBeInvalid, toBeValid} from './to-be-invalid'
import {toHaveValue} from './to-have-value'
import {toHaveDisplayValue} from './to-have-display-value'
import {toBeChecked} from './to-be-checked'
import {toBePartiallyChecked} from './to-be-partially-checked'
import {toHaveDescription} from './to-have-description'

export {
Expand All @@ -39,5 +40,6 @@ export {
toHaveValue,
toHaveDisplayValue,
toBeChecked,
toBePartiallyChecked,
toHaveDescription,
}
54 changes: 54 additions & 0 deletions src/to-be-partially-checked.js
@@ -0,0 +1,54 @@
import {matcherHint, printReceived} from 'jest-matcher-utils'
import {checkHtmlElement} from './utils'

export function toBePartiallyChecked(element) {
checkHtmlElement(element, toBePartiallyChecked, this)

const isValidInput = () => {
return (
element.tagName.toLowerCase() === 'input' && element.type === 'checkbox'
)
}

const isValidAriaElement = () => {
return (
element.getAttribute('role') === 'checkbox' &&
element.getAttribute('aria-checked') === 'mixed'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is already done in isPartiallyChecked probably shouldn't be here, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yup, I don't think this should be part of this function. This function should only check if the element has a valid role for which this check makes sense. Then the actual check with the boolean value you set the pass property to, that's what then determines if the element is partially checked or not.

)
}

if (!isValidInput() && !isValidAriaElement()) {
return {
pass: false,
message: () =>
'only inputs with type="checkbox" or elements with role="checkbox" and a valid aria-checked attribute can be used with .toBePartiallyChecked(). Use .toHaveValue() instead',
}
}

const isPartiallyChecked = () => {
const isAriaMixed = element.getAttribute('aria-checked') === 'mixed'

if (isValidInput()) {
return element.indeterminate || isAriaMixed
}

return isAriaMixed
}

return {
pass: isPartiallyChecked(),
message: () => {
const is = isPartiallyChecked() ? 'is' : 'is not'
return [
matcherHint(
`${this.isNot ? '.not' : ''}.toBePartiallyChecked`,
'element',
'',
),
'',
`Received element ${is} partially checked:`,
` ${printReceived(element.cloneNode(false))}`,
].join('\n')
},
}
}