Click on a disabled button with nested elements is silently ignored

[isaacplmann]

• cypress-testing-library version: 2.3.5
• cypress version: 3.1.5
• node version: 8.15
• npm (or yarn) version: yarn 1.12.3

Note: This issue is related to getByText, but the problem is in the way it behaves in relation to cypress, not a bug in the actual getByText function itself.

Relevant code or config

<button disabled>
  <span>Hi</span>
</button>

What you did:

cy.getByText('Hi').click()

What happened:
The span element was clicked, even though the button was disabled. The click is ignored by the button though, since it is disabled.

Workaround:
You can revert to normal cypress commands and do this:

cy.contains('button', 'Hi').click();

Cypress will wait until the button is enabled before trying to click. Or if the timeout expires, it will throw an error.

Suggested solution:
I'm not sure? Silently failing seems like the wrong thing though. Perhaps I just shouldn't use cy.getByText to try to click on a button.

[isaacplmann]

This seems like it is related to @bahmutov's PR #31 .

That PR is about adding cypress' default retry until the query is satisfied. This issue is kind of the converse - wait to perform an action (in this case click) until it is allowed.

[sompylasar]

Imagine what happens if an actual user clicks a disabled button. What should happen? The user might not wait while tge button becomes enabled unless that is their explicit behavior that you assume in the test by adding a wait. What if the button still does something on that click while it's disabled, e.g. shows a "please wait patiently" hint, should that be tested, too?

[isaacplmann]

Adding waits in your e2e tests is the path to misery and suffering - that's why people use Cypress in the first place.

The button has styles applied to it so that a real user can see that it is disabled. I'm fine with you saying that cypress-testing-library will not function the same way as normal cypress commands do, that should just be documented.

[kentcdodds]

I think we should do what Cypress does. This project needs a little help getting things resolved like #31. I don't have the bandwidth to work on it right now though 😬

I'm pretty sure that if you were to select the span and fire a click event on it with regular cypress you'd have the same experience.

I think this should be resolved with testing-library/dom-testing-library#190 and then you could specify a selector. I'm not certain though.

[smeijer]

I'm having difficulties with clicking links/buttons that contain text nested in spans. Is that related to this issue?

<button>
  <span>login</span>
</button>

cy.findByText('login') // resolves the span
cy.findByText('login', { selector: 'button' }); // doesn't resolve anything

[NicholasBoll]

Interesting. I'll verify later today and see if that is an issue in this library or in Testing Library DOM.

There had been many changes to this library since this issue was opened. I'll also see if there original issue is still valid.

[NicholasBoll]

I've verified this issue, now looking why it is happening.

[NicholasBoll]

Okays, so this library calls out to @testing-library/dom.

cy.findByText calls down to:
https://github.com/testing-library/dom-testing-library/blob/7f8c100e1139c656ca817c23f223e7c15df26774/src/queries/text.js#L29

This calls out to:
https://github.com/testing-library/dom-testing-library/blob/master/src/get-node-text.js#L5-L14

I ran an experiment that you can run in your browser's console:

const TEXT_NODE = 3

function getNodeText(node) {
  if (node.matches('input[type=submit], input[type=button]')) {
    return node.value
  }

  return Array.from(node.childNodes)
    .filter(child => child.nodeType === TEXT_NODE && Boolean(child.textContent))
    .map(c => c.textContent)
    .join('')
}

const button = document.createElement('button')
const span = document.createElement('span')
span.innerHTML = 'Test'
button.appendChild(span)

getNodeText(button) // ""
getNodeText(span) // "Test"

I think @kentcdodds knows there has been some churn around how this should work. He may be able to shed some more light on this. It is definitely working as implemented. There is no way that I can see to override how the query works when it calls getNodeText. It is looking for text node children (filtering out element nodes like span). I'm sure there's a reason for this.

[NicholasBoll]

Original issue is still a thing. The span will be returned and clicked. Cypress will not wait for actionability before clicking the span.

[NicholasBoll]

It looks like @testing-library/dom is being bumped to v7 which is changing the way *ByLabelText works. A selector can be passed to scope which element is being matched against, but not in how it determines text.

This looks like a tricky issue. It seems like you'd expect cy.findByText('login', { selector: 'button' }) to return a button that contains text login, similar to cy.contains('button', 'login'), but I followed a few threads around the issue. The main one giving problems is this example:

<label>
  My Select
  <select>
    <option>Option 1</option>
  </select>
</label>

There was numerous efforts to prevent Option 1 from being considered part of the text of the label. It is hard to say what is expected in all cases.

It might be worth it to build up a specification of what getNodeText should do in different use-cases. It is very possible we really want all text to be concatenated in most cases with a few cases excluded (like nested option tags).

Any change that isn't an opt-in through an option would be a breaking change. It might be up to Kent what should be done. I could file an issue on DOM Testing Library with a proposal to see if we could get consensus on what should be done in various use-cases.

cy.findByText and cy.contains to similar things. Cypress Testing Library does walk an interesting line because Cypress has built-in affordances and expectations for how Custom DOM Commands should work.

[kentcdodds]

Thanks for all of this! If you could open an issue for us to discuss this that would be great! It would be good to get any potentially breaking changes implemented before we cut the next major release.

[kentcdodds]

I think this should be resolved (especially with the functionality of the findByRole query). So I'm going to close this.