toBeDisabled no longer considers parent element #265

simoneb · 2020-06-22T08:07:03Z

@testing-library/jest-dom version: 5.10.1
node version: 12.13.1
npm (or yarn) version: 1.22.4

dom-testing-library version: 6.16.0
react-testing-library version: 9.5.0

Relevant code or config:

const button = getByText('disabled button')

expect(button).toBeDisabled();

What you did:

The rendered HTML comes from Material UI and it's a simple <Button /> being passed properties { children: 'disabled button', disabled: true }.

<body style="">
	<div>
	  <button
	    class="MuiButtonBase-root MuiButton-root MuiButton-contained MuiButton-containedPrimary Mui-disabled Mui-disabled"
	    disabled=""
	    tabindex="-1"
	    type="button"
	  >
	    <span
	      class="MuiButton-label"
	    >
	      disabled button
	    </span>
	  </button>
	</div>
</body>

What happened:

In version jest-dom@5.7.0 the assertion .toBeDisabled() worked fine, in jest-dom@5.10.1 it fails with:

expect(element).toBeDisabled()

    Received element is not disabled:
      <span class="MuiButton-label" />

      36 |     debug();
      37 |     const button = getByText('disabled button');
    > 38 |     expect(button).toBeDisabled();
         |                    ^
      39 |   });
      40 | });
      41 |

It DOES work if I change the assertion to hit the parent element directly, which is indeed the element that's disabled:

const button = getByText('disabled button').parentElement;
expect(button).toBeDisabled();

Reproduction:

https://codesandbox.io/s/react-testing-library-demo-utmkj?file=/src/__tests__/button.js

Problem description:

It seems like earlier versions of jet-dom were considering parent elements to check whether an element was disabled, but no longer now.

The text was updated successfully, but these errors were encountered:

lourenci · 2020-06-22T11:27:13Z

The 5.10.1 has introduced a fix to not look at elements that are not allowed to be disabled. Take <a/> as an example, even inside a disabled fieldset, the <a/> is clickable by the user so it's not disabled.

I don't know what to think about a <span /> inside a disabled fieldset...

eps1lon · 2020-06-22T12:31:41Z

The problem is that you queried the span not the button and a span can't be disabled because it doesn't have any behavior.

To query the button, use getByRole('button', { name: 'disabled button' });

simoneb · 2020-06-22T13:02:06Z

@eps1lon fair enough, it makes sense but I thought I'd report it anyway since this used to work and upgrading broke our CI.

Thanks for the suggested way to query the button, I agree it's more correct and robust. Note that you're introducing a breaking change in a patch release though.

gnapse · 2020-06-22T14:00:39Z

Note that you're introducing a breaking change in a patch release though.

Sorry. That was my mistake when merging #261. In my mind I viewed it as a bug fix, since that's ultimately what it is. But you are right, and I'll keep that more in mind in the future. Thanks!

nickmccurdy · 2020-06-22T15:38:19Z

In my opinion, a breaking change that's intended exclusively as a bug fix usually doesn't need a major release, as relying on that behavior would generally not be considered part of the public API. A major release would make it harder for users to upgrade and fix this bug. However, I think it could help to update the release/documentation/readme/etc to make the behavioral change more obvious.

simoneb · 2020-06-22T15:52:32Z

This is more of a general discussion. I agree that this is a fix to address a behavior that should probably not have been there in the first place, but at the same time it's clear that it is also a breaking change, because whether it's a bug or not, user code is relying on this behavior.

I do agree that it doesn't deserve a major release, but when I review package upgrades I usually consider major bumps to require much attention, minor bumps a little less and patches to be usually non-breaking.

Now, this is a testing library, so it's not a big deal anyway, but clearly if it is a library used in production, I guess you can agree that getting a breaking change from a patch bump would be somewhat unexpected.

eps1lon · 2020-06-22T16:46:07Z

it's clear that it is also a breaking change, because whether it's a bug or not, user code is relying on this behavior.

That is unreasonable for library authors. If a dependant relies on a bug then this, while unfortunate, is a problem in the dependent code. SemVer makes it pretty clear that this was a patch.

You're essentially arguing to release any change in a SemVer major because for every package there is some code that can "break" if the package changes (even if you don't change documented behavior".

Then changelog includes that toBeDisabled was changed. It was considered a bug fix because previously toBeDisabled would check elements that couldn't be disabled in the first place.

By definition a bug fix is backwards incompatible for some code i.e. can be considered a breaking change. The question we need to answer is if that previous behavior was documented as such.

gnapse · 2020-06-22T17:33:32Z

By definition a bug fix is backwards incompatible for some code i.e. can be considered a breaking change. The question we need to answer is if that previous behavior was documented as such.

Good point. I think this is the key argument here. It was certainly not documented that it would work on non-form elements. However, I can see how it can cause confusion since in their mind they were matching a button and not a span. This is a combination of unfortunate situations, because they were also not using the more semantic getByRole('button') and since the button had a span in it, that element was matched instead of the button.

All-in-all I get your point @simoneb, and I'll still try to take a better look at this in the future for tricky cases, and maybe make a better job at announcing the changes if possible. But overall it was ok to make it a bug fix in this case.

Thanks for bringing the issue up anyway. It certainly helps.

simoneb · 2020-06-22T18:51:55Z

This is not an issue whatsoever, it's an easy fix on our side, so thanks and keep up the great work.

)

kakapo4 · 2021-06-07T21:03:51Z

The problem is that you queried the span not the button and a span can't be disabled because it doesn't have any behavior.

That seems reasonable, but the documentation does say,

It matches if the element is a form control and the disabled attribute is specified on this element or the element is a descendant of a form element with a disabled attribute.

Am I misreading the second part incorrectly? To me it implies the span should match as it's, "a descendant of a form element with a disabled attribute"

gnapse · 2021-06-07T22:18:17Z

Good point. You are right.

Though I'd argue that we should fix the documentation in this case, and not the implementation. The main reason is the PR #261 and what it uncovered and fixed. It was incorrect to use this expression in the documentation, and before #261 that expression was also how the implementation behaved. The reason why it was expressed this way I believe is because of how the disabled attribute is documented (emphasis is added here by me):

The user can neither edit nor focus on the control, nor its form control descendants.

This means that form controls that are inside a disabled element are also disabled. But elements that are not form controls are not disabled (the concept of being disabled do not apply to them).

We currently still comply with detecting a form element as disabled if it's inside a disabled parent elment. This is evidenced by this test case:

jest-dom/src/__tests__/to-be-disabled.js

Line 43 in 9b26aef

expect(queryByTestId('fieldset-child-element')).toBeDisabled()

The target element queried there is a button that's not disabled explicitly, but it is inside a disabled fieldset, hence it is correctly detected as disabled.

I will rephrase that part of the documentation accordingly.

eps1lon closed this as completed Jun 22, 2020

alexmund-bbc referenced this issue Jul 8, 2020

fix: element not allowed to be disabled being returned as disabled (#261

5e39222

)

gnapse mentioned this issue Jun 7, 2021

docs: Update the documentation of toBeDisabled #375

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

toBeDisabled no longer considers parent element #265

toBeDisabled no longer considers parent element #265

simoneb commented Jun 22, 2020

lourenci commented Jun 22, 2020 •

edited

eps1lon commented Jun 22, 2020

simoneb commented Jun 22, 2020

gnapse commented Jun 22, 2020

nickmccurdy commented Jun 22, 2020 •

edited

simoneb commented Jun 22, 2020

eps1lon commented Jun 22, 2020

gnapse commented Jun 22, 2020

simoneb commented Jun 22, 2020

kakapo4 commented Jun 7, 2021

gnapse commented Jun 7, 2021

toBeDisabled no longer considers parent element #265

toBeDisabled no longer considers parent element #265

Comments

simoneb commented Jun 22, 2020

Relevant code or config:

What you did:

What happened:

Reproduction:

Problem description:

lourenci commented Jun 22, 2020 • edited

eps1lon commented Jun 22, 2020

simoneb commented Jun 22, 2020

gnapse commented Jun 22, 2020

nickmccurdy commented Jun 22, 2020 • edited

simoneb commented Jun 22, 2020

eps1lon commented Jun 22, 2020

gnapse commented Jun 22, 2020

simoneb commented Jun 22, 2020

kakapo4 commented Jun 7, 2021

gnapse commented Jun 7, 2021

lourenci commented Jun 22, 2020 •

edited

nickmccurdy commented Jun 22, 2020 •

edited