Do not throw an error when clicking on a disabled element

[lolmaus]

The current behavior of the click helper is to throw an error "Can not click disabled element" when trying to do so.

The problem is that clicking on a disabled element is something that a user is perfectly capable of doing. Simulating this behavior in an acceptance test should not be punished.

When I make my test click a disabled button, I expect the test not to crash. I expect the test to proceed and to assert that nothing has changed.

Instead of clicking a disabled element, @ember/test-helpers forces me into asserting that the element contains the disabled attribute. This has two problems:

1. It's not the same thing. Asserting for an element's attribute does not guarantee lack of behavior on click.
2. The Cucumber BDD framework has a test matrix feature (aka Examples aka Where) which is intentionally limited: it does not have loops and conditionals for the sake of simplicity and readability. The fact that I need to use a different assertion depending on whether the button is disabled or not — makes it impossible to put it into a Cucumber test matrix.

[stefanpenner]

Sounds reasonable to me.

[ro0gr]

Asserting for an element's attribute does not guarantee lack of behavior on click.

I don't think there is a legit way to trigger click event handler over a disabled button. Also, if a button is disabled it would not trigger form submit. This is guaranteed by browser.

A rationale for this click restriction was an ability to fail as fast as possible in order to simplify a path to find a root cause when something goes wrong. There is just no practical sense from the user perspective to click on a disabled button, cause literally nothing can happen. This is relevant especially for complex scenarios when the end result fails due to some button is occasionally disabled in the middle of test scenario.

you can see a prior discussion here #778 (comment)

As a side note, I think, if it's really needed to click on disabled buttons, it seems easy to achieve by a wrapper util like

  function blindClick(...args) { return click(...args).catch(() => {})};

which would just ignore the restiction. However, I think, I'd consider switching from the "blind click" approach to checking the disabled explicitly.

[lolmaus]

I don't think there is a legit way to trigger click event handler over a disabled button. Also, if a button is disabled it would not trigger form submit. This is guaranteed by browser.

I'm not comfortable with that the framework makes such an assumption. This "guarantee" is not part of any spec that I'm aware of. And it assumes all browsers agree on it, which might be a fragile claim.

This case can be easily acceptance-tested and I see no reason why it should not be.

---

There is just no practical sense from the user perspective to click on a disabled button

Users do not always behave practically. I myself click disabled buttons occasionally: sometimes by accident, sometimes due to not realizing that it's disabled, sometimes to check if it's disabled...

---

A rationale for this click restriction was an ability to fail as fast as possible in order to simplify a path to find a root cause when something goes wrong.

I see you point but I can only partially agree. The matter is that acceptance tests should replicate user's behavior. When a user clicks a disabled button, the app does not crash, so the behavior of a test diverges from real life. Not that it matters much, yes, but still makes me feel on the fence.

---

if it's really needed to click on disabled buttons, it seems easy to achieve by a wrapper util like

Wouldn't it swallow legit app crashes?

To me it the opposite would make much more sense: the framework should not crash on clicking disabled buttons, and those who want such behavior could use special handling.

It doesn't have to be wrapper: could be a separate clickOrDie helper, or an argument to the standard click helper.

---

PS Sorry, closed the issue by accident. Reopened.

[ro0gr]

This "guarantee" is not part of any spec that I'm aware of.

I think this is something, that works for ages in all broswers. See https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/disabled

The Boolean disabled attribute, when present, makes the element not mutable, focusable, or even submitted with the form. The user can neither edit nor focus on the control, nor its form control descendants. If the disabled attribute is specified on a form control, the element and its form control descendants do not participate in constraint validation. Often browsers grey out such controls and it won't receive any browsing events, like mouse clicks or focus-related ones.

---

This case can be easily acceptance-tested and I see no reason why it should not be.

because there is no point to re-test what's already guaranteed by browsers as a part of standard.

[ro0gr]

When a user clicks a disabled button, the app does not crash, so the behavior of a test diverges from real life.

IMO, the tests and real user are 2 different environments. In tests developers are focused on finding the bugs, so we should adjust the environment to help with that.

In the link to the prior discussion I've sent before there are few pre-existing examples of another exceptions triggered by the framework, when it doesn't make sense to do smth from the user perspective. Also we have a similar strategy for attempts to edit disabled/readonly fields.

That said, I believe this click restriction is aligned with the rest of existing restrictions.

[lolmaus]

Often browsers grey out such controls and it won't receive any browsing events

That does not sound like a guarantee, does it?

---

because there is no point to re-test what's already guaranteed by browsers as a part of standard.

There may be cases that aren't that useless. Say, your UI has a handful of toggles represented by <button>. You program your test to click every toggle before asserting the result. When some buttons are disabled, you still want to make the assertions. You do not want the test to break on clicking disabled buttons!

With a traditional acceptance test, you would need to wrap your clicks with an if clause, which feels like a bad practice because it diverges from what a user would do.

With a Cucumber BDD test, you can not make an if clause, so you are forced to split a test invoked 12 times with different params and expectations — into 12 distinct tests with enormous amount of code duplication and prone to diverge as changes are made over time.

---

so we should adjust the environment to help with that.

You could use your own helper:

export function clickOrDie(elementOrSelector) {
  const element = find(elementOrSelector);

  if (isFormControl(element) && element.disabled) {
    throw new Error(`Can not \`click\` disabled ${element}`);
  }
  
  return click(element);
}

This super small helper would serve you goal perfectly, without even a tiniest bit of incovenience.

Yet, you (and @rwjblue) chose to enforce this assumption on everyone without a way to opt out of it, causing practical problems to developers who have different opinions on testing or use testing setups different than yours. 😒

It looks like my only option is to copy-paste the click helper's code into my app's codebase and remove the throw. This will prevent the helper from receiving further updates. It also requires dealing with quite a few private imports, and might require adding (otherwise unnecessary) libs to package.json.

[lolmaus]

Another non-useless case is attempting to submit a form and triggering form validation errors.

Yes, this can be replicated by blur(). But again, it requries different logic for cases that are not different from user's perspective: blur is not even something a user can do, without doint something else along with it (such as an "impossible" click)!

[ro0gr]

You program your test to click every toggle before asserting the result.

"Every" in tests sounds suspicuos to me. Often it may lead to fragility, cause if some new control is added to the app, the end result would differ, and the test would fail. However, if this is a good thing to do, I suspect there should be a way to say "click every active toggle" in Cucumber(sorry, I'm not a user of Cucumber, so can't say for sure if it's really possible, but I believe it should!).

With a traditional acceptance test, you would need to wrap your clicks with an if clause

Not really. With a traditional acceptance test, I know the current state app state beforehand, and I know which controls I want to click on in order to complete the flow. Doing it in a "every" fashion may lead to fragility, as I said above. So I would rather click over known controls for both traditinal and cucumber tests(or use "every active" approach).

Another non-useless case is attempting to submit a form and triggering form validation errors.

I agree, I can umagine a user clicking on a disabled submit in real live. But in fact, by "clicking" like that, they don't actually click the button, they just trigger the blur! And as you said it can be achieved by a public blur( helper, or you can also click( elsewhere but button to achieve the blur behavior.

I can see how someone may find it confusing, but if talk in terms of semantics, the only purpose of a button, is to react to clicks. If it's disabled, it just can't do it's only job. So, most of the time cliking on such elements are actually errors. And we can catch such errors for everyone for free, which sounds like a good thing to have. From the other hand, I believe there are alternative publlic APIs to cover use cases you've listed in the thread.

[ro0gr]

It looks like my only option is to copy-paste the click helper's code into my app's codebase and remove the throw. This will prevent the helper from receiving further updates. It also requires dealing with quite a few private imports, and might require adding (otherwise unnecessary) libs to package.json.

a less verbose approach could be to filter out unwanted exceptions, like:

function blindClick(...args) { 
  return click(...args).catch((e) => {
    if (!e.message.startsWith('Can not `click` disabled')) {
      throw e;
    }
  })
};

I believe the filter can be improved, and it might also have sense to emit custom error type here from the test helpers. But still, it feels more reliable than dealing with private utils, and extra deps. However, I hope to conveince you that the current behavior is not that bad and may not worth maintaining a custom helper 😉

[lolmaus]

"Every" in tests sounds suspicuos to me. Often it may lead to fragility, cause if some new control is added to the app, the end result would differ, and the test would fail..

Of course, I meant something like click('.product-options .toggle') and not click('button'). 😕

---

I suspect there should be a way to say "click every active toggle" in Cucumber

Technically there is, but that's against the spirit of BDD, according to which you should write scenarios from the user's perspective, replciating their actions.

E. g. the user goes like "I want a sandwich with everything!", and they proceed to burst-clicking every ingredient toggle in sequence. They don't notice that the cream cheese option got disabled when they clicked pickles, they would still burst-click through cream cheese.

That's what users do, and that's what the BDD user story would say when it leaves the design phase and gets attached to the development ticket.

I don't want to stray away from this story because someone wanted eager errors and decided that everyone should have eager errors enforced without opt-out.

Just imagine a PR gets rejected with:

• Why did you change this scenario? The team had spent so much time to agree upon it.
• Long story. There's this guy on GitHub...
  _{He has an opinion...
  And then Robert Jackson merged...
  That was 1.5 years ago, and now...
  No, it's not documented but...
  No, there is no way to opt out of it...
  No, catching it would swallow legit errors and produce false positives...
  Well, yeah, I could probably wrap every click with a dedicated undoing of what that PR does...
  I guess I should add a comment with a link to an issue that got rejected... Developers will sure not lose any of their precious time reading through that thread, forming opionions and then disputing over it with each other...}

---

With a traditional acceptance test, you would need to wrap your clicks with an if clause

Not really. With a traditional acceptance test, I know the current state app state beforehand, and I know which controls I want to click on in order to complete the flow.

I've been talking specifically about a single test with a matrix with 12 combinations of seeded data and expected result. I don't want to create exceptions for certain input conditions. I've seen such tests with numerous conditions, effectively that's write-only code. 👎 It takes enormous time to grasp it. And as the app matures, such tests keep growing in complexity, with excetptions from exceptions, nested loops (e. g. you add emoji reactions, now each comment of each thread of each post needs to be checked for every emoji — true story).

I just want a simple test with a matrix. Say, I have a wizard with a breadcrumb-like navigation. I can navigate back to any previous stage, but I can only navigate one step forward. All stages are visible in the navigation.

I would like to test that navigation items are properly disabled. I'm testing it from the user perspective who does not know what an HTML atttribute is.

Scenario: Navigating from stage [Initial] to stage [Target]
  When I visit URL "/stage/[Initial]"
  And I click Stage [Target]
  Then I should be at URL "/stage/[Resulting]"

  Where:
    | Initial | Target | Resulting |
    | 1       | 1      | 1         |
    | 1       | 2      | 2         |
    | 1       | 3      | 1         |

Note how when I try to navigate one step ahead, I do. But when I try to navigate two steps ahead, I stay on the initial route.

Yes, this technically can be resolved with something like And I click Stage [Target] unless it's disabled, but that's not what the user does. That's not the story that the team wants to cover. I have found the spec that guarantees that the click event would not be propagated, but the designer and the manager are not aware of it and they will have questions.

In order to work around the crash-on-disabled without straying away from the user story (which in BDD terms is sometimes called "spec", by the way), I have to split this test into two:

Scenario: Navigating from stage [Initial] to stage [Target] successfully
  When I visit URL "/stage/[Initial]"
  And I click Stage [Target]
  Then I should be at URL "/stage/[Target]"

  Where:
    | Initial | Target |
    | 1       | 2      |
    | 2       | 3      |
    | 2       | 1      |

Scenario: Navigating from stage [Initial] to stage [Target] unsuccessfully
  When I visit URL "/stage/[Initial]"
  Then I the button Stage [Target] should be disabled
  Then I should be at URL "/stage/[Initial]"

  Where:
    | Initial | Target |
    | 1       | 1      |
    | 1       | 3      |

But since the user is not capable of inspecting the HTML attributes of a button, the should be disabled step is not representing a user interaction and should be removed. (The visual part of disabling is covered by Percy. If you assume that the disabled state guarantees a visual change in the button , then eventually you'll have false positives: someone breaks the CSS and you won't notice because of your belief that [disabled] matters more than what user actually sees and actually does.)

As a result, the latter scenario would be reduced to this:

Scenario: Navigating from stage [Initial] to stage [Target] unsuccessfully
  When I visit URL "/stage/[Initial]"
  Then I should be at URL "/stage/[Initial]"

Which is ridiculous. I just want one meaningful test, aligned with my team's BDD workflow. Not a test with half of the cases and another do-nothing test representing the other half, which remains untested because, quote-unquote, "Often browsers grey out such controls and it won't receive any browsing events".

---

But in fact, by "clicking" like that, they don't actually click the button, they just trigger the blur!

They do actually click the button. You probably wanted to say "they do not actually produce the click event".

Note how you, the author of this PR who understands the matter better than anyone else, still fall into making this implicit substitution in your mental model! Thinking that the user didn't click when they did click.

There's a book "The inmates are running the asylum" by Alan Cooper that is dedicated to this specific problem: the problem that developers substitute user needs with their own needs. The metaphor in the book's title says that letting developers design apps is akin to letting crazy patients rule the mental hospital, because developers would inevitably cut corners and care about their own priorities while neglecting user priorities and actual user experience.

BDD is a methodology aimed at avoiding this trap via discipline.

---

a less verbose approach could be to filter out unwanted exceptions

Why is it me that has do this in every app? Your opinionated approach to testing is efficient in detecting inconsistencies early, good for you. But why do you impose it on everyone, including people with very different approaches and very different priorities, causing them a lot of trouble (which I hope you now feel from my examples above), instead of just doing what you want in your apps?

---

I suggest the following path forward:

1. The default behavior of the click helper is changed so that it does not throw when clicking a disabled element.
2. The throwing behavior is made available as optional, either via a separate helper or via an argument to the click helper.
3. This gets properly documented.

I'm not sure about the fill helper and the rest. The fill helper does not quite represent the user's action. What the user actually does is clicking an input and then typing on the keyboard, which is very different from signlaing the input to replace its value. On the one hand, the user might hypothetically attempt typing on the keyboard after clicking a disabled element. On the other hand, replicating it in a test is a very weird thing to do and is certianly not part of anyone's test routine (we don't test for typing into a disabled field, and I know nobody who does). The fill helper seems to be a reasonable shortcut through the BDD discipline.

But despite that consideration, if my click suggestion passes, it may be reasonable to revert the fill behavior as well, for the sake of consistency with click behavior.

[lolmaus]

if (!e.message.startsWith('Can not `click` disabled')) {

Then multiple tests will break when the phrasing is updated in the dependency. 👎 That's unlikely, of course, but according to Murphy's law, it will happen someday. Just imagine CI suddenly failing a PR with a completely unrelated change, and the failure is not repoducible locally (because only the CI has the updated dependency). Do you really want someone somewhere be pulling their hair in frustration over this?

The core team would of course be in denial saying that the exact phrasing was never designated as public API.

This could be addressed by checking for e.message.includes('disabled') rather than the exact phrasing from your PR. But then the check becomes heuristical and prone to false positives, causing a different, but still very frustrating moment of hair-pulling. 👎

Just... why...

[ro0gr]

Why is it me that has do this in every app?

cause you've just suggested a more intimate way to use private apis and extra deps. just trying to figure out a path forward rather than backward.

Then multiple tests will break when the phrasing is updated in the dependency. 👎

also, you can see I've suggested to add a Typed Error for cases like this, to put more future proofness via a public API. Not sure if it's fine from the core team standpoint though.

also an actual message is covered by the test

ember-test-helpers/tests/unit/dom/click-test.js

Line 253 in 0dfa427

new Error('Can not `click` disabled [object HTMLInputElement]')

, so I would optimistically call it a public API 🤠

---

Honestly, I think the discussion goes a bit out of scope of the ember/test-helpers. There are some arguable points to me, like navigation done via buttons, etc.. But I don't want to let this thread grow indefinitely 😆

IMO, in scope of the ember/test-helpers the current restriction makes sense for most of the users(though I don't have statistics, it might be that majority does BDD). If such, I think it does make sense to leave it as default, since I haven't noticed similar reports yet.

I would not argue about BDD, cause it's your battlefield. But it seems like there is an abstraction layer over the test-helpers, so we might probably need to find a way to allow the default behavior to be disabled(via a option or custom Error Type + catch, as suggested above).

Would like to hear the other opinions.

[lolmaus]

Honestly, I think the discussion goes a bit out of scope of the ember/test-helpers

I don't want to let this thread grow indefinitely 😆

Above, I have gone qute a length trying to explain how much pain and trouble your, seemingly small, decision is causing me. My rant is not offtopic, at least for me it isn't.

I agree that BDD should totally be out of @ember/test-helpers scope. But the matter is that your tiny, yet very opinionated decision has caused @ember/test-helpers to affect the BDD workflow! It's not just my caprice to discus BDD in this thread. It's that your decision has consequences that you ask me to stop discussing.

---

the current restriction makes sense for most of the users

That does not mean that it must be enforced, let alone without an ability to opt out.

That does not even mean that it has to be enabled by default!

E. g. the absolute majority of Ember apps uses ember-truth-helpers, yet those helpers are not bundled with Ember and remain optional. Optional and out by default — even though including them would have no consequences to app (except for a tiny increase in disto size). Unlike your change, which, being enforced, hurts a minority of users.

---

so we might probably need to find a way to allow the default behavior to be disabled(via a option or custom Error Type + catch, as suggested above)

The fact that you would not even consider my suggestion (do not throw by default, anyone who needs it can use it effortlessly) is... sad. 😔

---

just trying to figure out a path forward rather than backward

Sometimes a step forward is a step into 💩. In such case it make sense to take a step back, and then forward again in a slightly altered direction.

I'm not saying your PR is 💩 or anything like that! I mean no offence or disrespect. I'm saying that my code is 💩 because you decided for everyone which direction should be considered forward. After being forced to follow that direction, most Ember users did not end up standing in 💩and in fact some even ended up with better vantage. But a few like me found themselves standing in 💩. And when I ask to reconsider the angle of "forward", so that I can avoid 💩, you ignore my request and instead offer me a scoop.