-
-
Notifications
You must be signed in to change notification settings - Fork 257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not throw an error when clicking on a disabled element #1100
Comments
Sounds reasonable to me. |
I don't think there is a legit way to trigger A rationale for this 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 |
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.
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...
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.
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 PS Sorry, closed the issue by accident. Reopened. |
I think this is something, that works for ages in all broswers. See https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/disabled
because there is no point to re-test what's already guaranteed by browsers as a part of standard. |
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 |
That does not sound like a guarantee, does it?
There may be cases that aren't that useless. Say, your UI has a handful of toggles represented by With a traditional acceptance test, you would need to wrap your clicks with an 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.
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 |
Another non-useless case is attempting to submit a form and triggering form validation errors. Yes, this can be replicated by |
"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!).
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).
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 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. |
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 😉 |
Of course, I meant something like
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:
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 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 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".
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.
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:
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. |
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 Just... why... |
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.
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
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 + Would like to hear the other opinions. |
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
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
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. 😔
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. |
Sorry, I may have be perceived as an ignorant person, but I'm not. In my last comment I've been thinking about the ways to an opt out strategy. Let's focus on an issue rather than a person plz 🙏 I do really think the new behaviour is better. And maybe even for cucumber scenarios. The new behavior has been clearly shipped in a major release, which is supposed to be potentially breaking. I do really like to find a path forward by avoiding a 💩 feeling. For this I think, it'd be more productive to hear from the other folks. Cause we might miss some more general scenarios, which can shed some more light on the issue. |
The spec does have a concrete opinion on this: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#enabling-and-disabling-form-controls:-the-disabled-attribute I believe that suggests the existing behavior is alright, and it is likely appropriate to close this issue. But WDYT? Please let me know if my reading on ^^ is in error |
This spec unifies the behavior across modern browsers: clicking on disabled elements will not produce events. But it does not mean that the user cannot click a disabled element. It only means that the click will be ignored, but it can still be made. And it does not imply that tests should fail on click. @stefanpenner Failing tests is merely a convenience for developers. If we make an assumption that clicking on a disabled element is never intentional, then such eager error throwing will help us detect inconsistent tests early and point at an offending line, rather than trying to figure out the consequences (which expect the click to produce some result). My point is that this assumption is NOT universally correct and should not be imposed on everyone. It should be opt-in: anyone who wants to rely on this assumption, could do so via either |
|
It's an interesting debate. In general I prefer to not "test the platform or framework" in my app tests. Testing that a disabled button doesn't emit events falls into this category (for me at-least) and I would rather test the existence of the disabled attribute + rely on the browsers tests suites for the rest. I can see some folks wouldn't feel comfortable with just that, and would want to write the test anyways. Ultimately I guess that is a fair opinion to hold. I see several options:
|
I never suggested that browser logic should be tested. My point has been that crashing the test when a disabled button is clicked is harmful in at least three cases:
I find it quite a bad tendency that a minority is consciously set to run into frustration, for the sake of making it slightly more convenient for the majority (most of which will not even notice and appreciate the change). I believe "the path forward" should be inlcusive. A change should not be imposed on everyone if it's counter-productive to at least some users. Especially if there's no opt-out. Especially if it is being decided quitely in an addon's PR that very few have seen. If the crashing approach is beneficial to most, it should be properly documented in the testing section of the guides. That's enough. Those who really care about the efficiency of recovering from clicking a disabled button by mistake, will use the documented apporach. You don't have to enforce this approach on everyone, including developers who click disabled buttons not by mistake.
If you mean the naming, that was merely a "working title", of course. Should use a softer language for the real deal. If you mean the approach of importing a separate helper, then I disagree that it's brutal: I find a separate helper to be most reasonable:
Anticipating a possible counter-argument, I would like to say that an opt-out helper |
Fwiw, I've come across the error-throw-on-disabled behavior that was added in #778 and at first found it confusing, but agree generally with the practicality it provides for tracking down invalid results (we don't use the mentioned BDD methodology, and my first run-in with this error throw resulted in an immediate code change because it pointed out an actual code issue). I am also sympathetic to what @lolmaus is saying here, and it was even pointed out in the error-adding PR that opt-in behavior might be a sensible tool to add here (along with documentation of course!), and that different browser versions can handle event firing on For general flexibility, and considering the BDD community, whatever their size may be, it would be nice to have individual opt out like I'll finish by recommending a great book I've read recently, A Philosophy of Software Design. The related bit here is a focus on allowing the most common useful cases for invoking a module to be the defaults. For these test helpers, there was already a precedent to error on invalid focusing, and I'd say most consumers (including my team) actually benefit from that general fail-fast behavior being the default, and would not want to have to redecorate all of our test imports to opt back in to that sensible (for test practicality) default behavior. |
Just to note here, I'm doing some looking around before opening a PR to document this error-throwing decision in |
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 thedisabled
attribute. This has two problems:Examples
akaWhere
) 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.The text was updated successfully, but these errors were encountered: