Blueprint: Checkbox indeterminate should only be modified if set

[ernestofreyreg]

Environment

• __Package version(s)__: 3.14.1
• __Browser and OS versions__: Chrome Version 72.0.3626.121 (Official Build) (64-bit) on MacOS Mojave

Steps to reproduce

1. Create a <Checkbox /> without indeterminate or defaultIndeterminate prop
2. Component state will default indeterminate to false
3. this.input.indeterminate = state.indeterminate will be set (This fails on some JSDOM envs used for snapshot testing)

Actual behavior

internal state.indeterminate gets defaulted to false even if not specified.

Expected behavior

Value for state.indeterminate should be null

Possible solution

Fix default state.indeterminate to support null.

[adidahiya]

Well, state.indeterminate = false is not _incorrect_... so I don't see a good reason to change this behavior. Also I don't think you should be relying on internal state of the component like that for testing, it's not part of the public API of Blueprint.

[ernestofreyreg]

I think I didn’t submit a good description of the problem I found and tried to give hints on where I saw could be fixed on the codebase. Sorry for that. Will try to rephrase again.

The bug I found is when using snapshot testing on the Checkbox component. Fails because is trying to call intermediate = false on an input that doesn’t exist.

This is part of the output I get when trying to do snapshot testing to that Checkbox component.

TypeError: Cannot set property 'indeterminate' of null

      at Checkbox.Object.<anonymous>.Checkbox.updateIndeterminate (node_modules/@blueprintjs/core/src/components/forms/controls.tsx:256:37)
      at Checkbox.Object.<anonymous>.Checkbox.componentDidMount (node_modules/@blueprintjs/core/src/components/forms/controls.tsx:247:14)
      at commitLifeCycles (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:9068:22)
      at commitAllLifeCycles (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:10418:7)
      at HTMLUnknownElement.callCallback (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:2365:14)
      at Object.invokeGuardedCallbackDev (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:2415:16)
      at invokeGuardedCallback (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:2468:31)
      at commitRoot (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:10626:7)
      at completeRoot (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:12060:3)
      at performWorkOnRoot (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:11989:9)
      at performWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:11897:7)
      at performSyncWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:11871:3)
      at requestWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:11740:5)
      at scheduleWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:11550:5)
      at scheduleRootUpdate (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:12152:3)
      at updateContainerAtExpirationTime (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:12180:10)
      at updateContainer (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:12191:10)
      at create (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:12624:5)
      at getRenderedTree (node_modules/@storybook/addon-storyshots/dist/react/renderTree.js:24:14)
      at node_modules/@storybook/addon-storyshots/dist/test-bodies.js:21:18
      at Object.<anonymous> (node_modules/@storybook/addon-storyshots/dist/index.js:125:20)

Looking at the source code (@blueprintjs/core/src/components/forms/controls.tsx) I noticed that the intermediate prop gets stored on the state but defaults to false

    public state: ICheckboxState = {
        indeterminate: this.props.indeterminate || this.props.defaultIndeterminate || false,
    };

and the condition to set that intermediate value on that internal input is based on (indeterminate != null) which never happens since state.indeterminate is defaulted to false (!=null).

This caused the function that the function updateIndeterminate always executes the input.indeterminate assignment event if not desired (not setting a valid indeterminate boolean value via props)

    private updateIndeterminate() {
        if (this.state.indeterminate != null) {
            this.input.indeterminate = this.state.indeterminate;   // <- Always executed
        }
    }

Hope you get a better idea of the problem I am seeing now. Yes, I should not rely on internal state (I don't) is just that the component is behaving unexpectedly for certain props.

Thanks again.

[adidahiya]

Yes, I see the problem now, I think your assessment is correct. If we had strict null checks enabled, typescript could have caught this bug :)