Add new rule no-action-on-submit-button

[JoaoDsv]

Fixes #44.

It implements the rule proposal of no-action-on-submit-button.

no-action-on-submit-button

This rule requires all <button> elements with a type="submit" attribute to not have any action.

When the type attribute of <button> elements is submit, the action should be on the <form> element instead of directly on the button.

By default, the type attribute of <button> elements is submit.

Examples

This rule forbids the following:

<form>
  <button type='submit' {{on 'click' this.handleClick}} />
  <button type='submit' {{action 'handleClick'}} />
  <button {{on 'click' this.handleClick}} />
  <button {{action 'handleClick'}} />
</form>

This rule allows the following:

// In a <form>
<form>
  <button type='button' {{on 'click' this.handleClick}} />
  <button type='button' {{action 'handleClick'}} />
  <button type='submit' />
  <button />
</form>

// Outside a <form>
<button type='submit' {{on 'click' this.handleClick}} />
<button type='submit' {{action 'handleClick'}} />
<button {{on 'click' this.handleClick}} />
<button {{action 'handleClick'}} />

[bertdeblock]

Should we also check if the parent form already has an action defined and if so, assume that the action on the submit button is an arbitrary event? E.g. for tracking purposes. Or would this be to far-fetched?

[bmish]

@JoaoDsv there are conflicts. Is this close to being ready to merge?

[bmish]

I fixed the conflicts, tests, and many (but not all) of the comments.

There's another comment here that may need to be addressed still: #44 (comment)

[bertdeblock]

If we need to check if the button has a form parent, we could make this function a shared helper and use that here as well? Though, if this rule specifically checks for type="submit" and buttons without a type (we also have the require-button-type rule to prevent this) that already seems like a good start?

[bmish]

@JoaoDsv if you'd like to finish this PR up, let us know. Otherwise, I think @bertdeblock and I will try to finish it up. @bertdeblock, I'll give you access to push updates to this PR, so we can finish addressing comments including reducing false positives and improving consistency.

[bmish]

To be clear, I do think we should address this comment to reduce false positives: #44 (comment)

I'm not going to work on this now so @bertdeblock you can take over if you're interested.

[JoaoDsv]

Thanks for your feedbacks @bmish @bertdeblock !
Sorry for the late answer, I'm happy to try my best to address your comments

Feel free to commit at any time to help me to get this PR merged, it will be very welcome 🙂

[JoaoDsv]

I addressed the remaining feedbacks ✅

Except reducing the false positives by checking the presence a parent form element. I'll handle it ASAP and let you know!

[MelSumner]

This would be disallowed by the require-button-type rule.

[JoaoDsv]

Thanks for your comment @MelSumner. Sure, but aren't rules independent from each other?

My point is: should we take an other rule into account in the unit test of this current rule?

If users decide to disable the require-button-type rule, then this syntax will be allowed by the current rule, because it's not its role to prevent the missing type.

What is your opinion about it?

[MelSumner]

So I try to write new rules with existing rules in mind; I don't want a rule I create to fail another existing rule.

But @rwjblue might have some more clarity here.

(PS overall I am excited about this rule!)

[JoaoDsv]

Hi @MelSumner

I'm back at work on this rule, which I'd forgotten for a long time 🙇

This rule is not encouraging <button /> without (explicit) type. It's essentially a syntax that won't make this rule fail, so I listed it in the "valid" scenarios.

Would you prefer me to remove <button /> from these scenarios?

• The downside is that this scenario won't be clearly mentioned while likely to happen.
• The advantage is that it won't encourage people using this syntax (not allowed by require-button-type dedicated rule)

[bmish]

Looks good except we still need the parent <form> check.

[bmish]

@JoaoDsv thanks for your continued work on this. Let us know when it's ready (with my last comment addressed).

[JoaoDsv]

Hi @bmish @bertdeblock

I have belatedly addressed your request about the parent <form> in this last commit: 24567e2. Is it what you had in mind?

ℹ️ Nicely spotted, as this also seems to solve @rwjblue's related concern: #44 (comment).

Would you mind having a last review and if everything looks ready, approve it?

[bmish]

Shouldn't we add a test for a deeply-nested button like this to make sure we aren't only checking the direct parent?

• <form><div><button {{action this.handleClick}} /></div></form>

[JoaoDsv]

Good idea! I added it as last scenario ✅

[bmish]

@JoaoDsv just to confirm, we should enable this as recommended in the next major release, right?

[JoaoDsv]

@JoaoDsv just to confirm, we should enable this as recommended in the next major release, right?

I'd indeed suggest to:

• add the rule in the next minor release
• flag the rule as recommended in the next major release

By doing so, we additionally have some time to implement a --fix option to make the upgrade to the next major smoother. Wdyt @bmish?

[bmish]

Sounds good, thanks for your work to get this finished!