Button does not preserve disabled prop value when set to false

[DannyDelott]

Environment

• Package version(s):
• Browser and OS versions:

If possible, link to a minimal repro (fork this code sandbox):

Steps to reproduce

Assert in a test that a Blueprint <Button> is disabled when set to false.

Example (using Enzyme apis):

it('should be disabled', () => {
  const wrapper = mount(
    <Button disabled={false} onClick={...} />
  );

  expect(
    wrapper.find('button').props().disabled
  ).to.equal(false);
});

Actual behavior

expected undefined to equal false

Expected behavior

Blueprint should preserve the boolean literal false, instead of converting to undefined.

Possible solution

Looks like this line should explicitly check if disabled === false instead of doing a simple falsy check.

blueprint/packages/core/src/components/button/abstractButton.tsx

Line 104 in 1aa7160

const disabled = this.props.disabled || loading;

[adidahiya]

I don't think this is explicitly part of our API contract, nor am I convinced that it needs to be. We don't spread all props as attributes to HTML elements like <button>.

[jrafidi]

The change made to fix this issue caused a regression in behavior. Previously, loading would always disable a button. Now, loading will not disable if disable is set to false.

If we intend to make this change, I think it should be done in a major version since this behavior change will require a migration to fix.

[adidahiya]

@jrafidi is right, and after realizing the downstream impact of the change I made in #5360, I think we should revert it. My statement in the comment above #3819 (comment) is still true, and Blueprint doesn't need to guarantee that <button disabled={false}> will get rendered by <Button disabled={false}> as suggested in the test case in the OP here.

The previous behavior where loading={true} overrides disabled={false} is more useful to users, so I'm going to revert back to that.