Add new Capybara/ClickLinkOrButtonStyle cop

[ydah]

Fix: #58

---

Before submitting the PR make sure the following are checked:

• Feature branch is up-to-date with main (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Updated documentation.
• Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
• The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

• Added the new cop to config/default.yml.
• The cop is configured as Enabled: pending in config/default.yml.
• The cop documents examples of good and bad code.
• The tests assert both that bad code is reported and that good code is not reported.
• Set VersionAdded: "<<next>>" in default/config.yml.

[mvz]

I'm not sure why it's supposed to be bad to use click_link_or_button when you don't know or care what interface element is used, but use a more specific method when you do know?

Put another way, the issue #58 says "There is no information which style is preferred.", but I don't think this is a matter of style at all!

[iainbeeston]

Can someone explain the rationale behind why using click_link and click_button is better than click_link_or_button and click_on? Maybe I'm missing something here, but I would have expected click_on and click_link_or_button to be better because your tests will be less tightly coupled to your HTML and it more closely reflects how a user would behave (there's no way a user can tell the difference between a button and a link styled to look like a button)

[ydah]

I thought strict was better when I first created this cop, but I reconsidered that you are right that click_on or click_link_or_button would indeed be less tied to the test and HTML and more faithful to how the user behaves. would you like to send a PR?

[iainbeeston]

@ydah To be honest my preference would be that this rule is disabled by default, but I appreciate that many other people would find it helpful

[ydah]

@iainbeeston Thank you for your feedback, we appreciate it. We will consider whether to enable or disable this cop by default at the time of the next major version update.

[samrjenkins]

For the reasons mentioned above, I too have always preferred click_on.

[franzliedke]

@mvz @iainbeeston @ydah Let me open another round. 🤓

Here's why I will definitely stay with the strict mode (if my team agrees ^^): buttons and links are very different in terms of semantics. No matter whether they look the same to the user, they should not be used interchangably.

I really like that writing good Capybara tests often forces me to solve problems that also solve problems for e.g. sight-impaired users (e.g. somehow creating a unique accessible label to avoid using IDs or other technical details as selectors). With the loose default, this cop would work against that.

[mvz]

For my use case 'strict' mode just means more work when the UI changes.

On the other hand, I don't want to be forced to change everywhere there is click_link to click_link_or_button.

For me, the one thing that would be useful is enforcing a choice of either click_on or click_link_or_button, rather than having a mix. This would mean still allowing click_link or click_button as well.

I see that a PR was merged changing to enforced_style: link_or_button, but I'm afraid this is equally useless to me.

[samrjenkins]

With the loose default, this cop would work against that.

@franzliedke I'm not sure I follow your reasoning. How does link-button agnosticism encourage more accessible markup?

I too am a strong advocate for using Capybara tests as a prompt for improving HTML accessibility, but I normally find myself reaching for title and aria-label attributes rather then converting <a>s to <button>s or vice versa. I'm not sure I see how stricter Capybara click_ matchers helps in this regard.

I also agree that links and buttons should not be used interchangeably; they have different use cases. However, to the user, they provide indistinguishable UX. They are semantically different to an engineer but semantically indistinguishable to the end user.

[franzliedke]

For my use case 'strict' mode just means more work when the UI changes.

@mvz I often find this argument confusing. When behavior changes (and that is the case when changing from button to link, or vice versa), I would expect having to make changes in tests.

I am not proposing to break your use case (you can still configure Rubocop to your preference, strict is just a default), I am just throwing my 2c in to vote against making this the default. If Rubocop makes a choice on a default, then that default should push (or gently nudge) people to do "the right thing", which - to me - is always accessibility over some notion of developer comfort.

For me, the one thing that would be useful is enforcing a choice of either click_on or click_link_or_button, rather than having a mix. This would mean still allowing click_link or click_button as well.

I am confused. According to the examples in #76, the new default link_or_button allows both click_on and click_link_or_button (they are just aliases anyway). And they disallow click_link and click_button.

@franzliedke I'm not sure I follow your reasoning. How does link-button agnosticism encourage more accessible markup?

@samrjenkins I meant to express the opposite: Encouraging devs to ignore the difference between button and link has the potential to hurt accessibility. Thus, the new "loose" (non-strict) default has the potential to hurt accessibility.

However, to the user, they provide indistinguishable UX. They are semantically different to an engineer but semantically indistinguishable to the end user.

I disagree. Keyboard interaction, mouseover, ctrl+click are all different, that's also user-visible behavior.

[franzliedke]

@ydah I just realized that #76 calls the strict setting deprecated. 😱 I obviously prefer to keep strict as default, but at the very least I hope to have convinced you all to at least keep that mode available.

[samrjenkins]

@franzliedke apologies. I mistyped. I should have asked: How does link-button agnosticism DIScourage more accessible markup?

[franzliedke]

Ah, thanks for clarifying. 🙈

Agnosticism enforces the notion that there's no difference between links and buttons; that is factually wrong and hurts accessibility.

[mvz]

@franzliedke

When behavior changes (and that is the case when changing from button to link, or vice versa), I would expect having to make changes in tests.

I agree, but this should only be the case in tests where the change in behavior is relevant to thing being tested. If it is relevant, I would definitely use click_link or click_button. If it is not relevant, I would use click_on.

[franzliedke]

That's great. 👍🏼

Aren't linters supposed to provide the safe path forward, and point out potential problems. (There's always some human judgement necessary, especially when it comes to the amount and level of suggestions that Rubocop makes.) I argue for the default to be that safe path, and for anyone who wants to say "I know what I'm doing", there's rubocop:disable (or the config).

[mvz]

Yes, I'm not against having to configure this cop to do what I want, and I can also see that requiring a specific action would be a good default.

What I am against is the idea that this is a matter of 'style'.

[ydah]

My apologies. I reminded myself that this cop is certainly not something that should be unified one way or the other, and that it is important to use it appropriately. How about we remove this cop for the next major version update?

[franzliedke]

I definitely like having it available, even if I don't prefer the default. 😉

[graaff]

@franzliedke apologies. I mistyped. I should have asked: How does link-button agnosticism DIScourage more accessible markup?

I'm late to the party here since this only showed up in a release now, but the answer to this question is that assistive software for people with disabilities uses the distinction between links and buttons to better help explain what will happen when one is followed or pressed.

If your site is audited for WCAG 2.1 compliance using e.g. links for in-page actions will definitely get flagged and your site won't be compliant. So knowing when to use a button or a link is important even when visible there is not going to be a difference.

I really hope that this default can be reverted so that the default will be to be explicit about using links or buttons and thus create more accessible websites.

[ermolaev]

now false positive if finding link by href

click_link(href: seo_link)

# Capybara/ClickLinkOrButtonStyle: Use click_link_or_button or click_on instead of click_link.

Failure/Error: click_on(href: seo_link)
ArgumentError:
     Invalid option(s) :href, should be one of :above, :below, :left_of, :right_of, :near, :count, :minimum, :maximum, :between, :text, :id, :class, :style, :visible, :obscured, :exact, :exact_text, :normalize_ws, :match, :wait, :filter_set, :focused, :disabled

[jgarber623]

Similar to @ermolaev's comment above, I'm seeing the same error from Capybara when following this cop's advice:

# Test passes
# Capybara/ClickLinkOrButtonStyle complains
click_button title: "Delete"

# Test fails
# Capybara/ClickLinkOrButtonStyle happy
click_on title: "Delete"

The exception (same as @ermolaev's):

     Failure/Error: click_on title: 'Delete'
     
     ArgumentError:
       Invalid option(s) :title, should be one of :above, :below, :left_of, :right_of, :near, :count, :minimum, :maximum, :between, :text, :id, :class, :style, :visible, :obscured, :exact, :exact_text, :normalize_ws, :match, :wait, :filter_set, :focused, :disabled

From that, it's not entirely clear to me if this is an issue with this cop's guidance or in Capybara itself. The various methods appear equivalent and I'm having trouble tracing the source of the ArgumentError in the Capybara::Node::Actions class:

# Lines 25–28
def click_link_or_button(locator = nil, **options)
  find(:link_or_button, locator, **options).click
end
alias_method :click_on, :click_link_or_button

# Lines 41–43
def click_link(locator = nil, **options)
  find(:link, locator, **options).click
end

# Lines 57–59
def click_button(locator = nil, **options)
  find(:button, locator, **options).click
end

[ydah]

I think proceeding in the direction of deleting Capybara/ClickLinkOrButtonStyle while creating a transition destination as follows. If you have any opinions, I would like to hear them.

• Add Capybara/AmbiguousClick cop and make soft-deprecated Capybara/ClickLinkOrButtonStyle cop. If you want to use EnforcedStyle: strict, use Capybara/AmbiguousClick cop instead #99