fix: Backward comaptability of Button disabled attribute

[BANOnotIT]

Closes #7905, closes #7816, closes #7269

📝 Description

disabled attribute can't be omitted because that requires BREAKING change

⛳️ Current behavior (updates)

All version @chakra-ui/button@>2.0.16 are incompatible with @chakra-ui/button@2.0.15

🚀 New behavior

It respects disabled attribute

💣 Is this a breaking change (Yes/No):

This is a revert of BREAKING change

📝 Additional Information

isDisabled can be a MINOR change but removing disabled prop behavior is a BRAKING change

[changeset-bot]

🦋 Changeset detected

Latest commit: 3561775

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@chakra-ui/button	Patch
@chakra-ui/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
chakra-ui-storybook	✅ Ready (Inspect)	Visit Preview	Aug 1, 2023 3:17pm

[Philzen]

@BANOnotIT why not simply git revert 0f376650efce9eb2d2cba3484ffe4e3a73364504? It comes down to the same thing w/o changing the Component's signature.

[BANOnotIT]

It comes down to the same thing w/o changing the Component's signature.

It doesn't since in mentioned commit disabled prop is placed after {...rest}. If you revert it then isDisabled={true} disabled={false} will result in override and element will not be disabled at all. My PR merges current behavior with the fact that disabled={true} should disable things anyway.

[Philzen]

It comes down to the same thing w/o changing the Component's signature.

@BANOnotIT indeed i was wrong here. But with your proposed PR for me the problem still exists that no workaround is possible for #7965, as isLoading will still switch it to disabled, even if disabled={false} (which also feels contradictory), which currently seriously hurts accessibility for visually impaired people.

Also, adding disabled to the Components' signature opposes what i believe @segunadebayo had in mind in the first place with 0f37665 – the idea here was to bring people to use isDisabled instead of disabled as far as i understand it.

[BANOnotIT]

which currently seriously hurts accessibility for visually impaired people.

Yeah... That's definitely needs some consideration.

adding disabled to the Components' signature

disabled is already in components' signature and was since 2.0.0. This PR just brings back functionality that was declared in 2.0.0 to comply with Semantic Versioning. It's critical in my project to bring back functionality that was declared and was neither properly deprecated nor given warnings. Mentioned commit is "BREAKING CHANGE" and can only occure in v3 and never in 2.*.

Imo it's more crucial to bring back functionality and then think on how we can design future interface better. But this attribute is used already and used in many projects in many places. One can't simply remove functionality in fix release.

[Philzen]

One can't simply remove functionality in fix release.

We're totally on the same page here, a breaking change in a minor version is unacceptable in a semver world.

This PR just brings back functionality that was declared in 2.0.0

No it doesn't (completely). Because in any version before 0f37665, i could set disabled={false} isLoading={true} to work around the accessability problem – but now it's broken and there's no way to keep the focus on the input field. So even if this PR gets merged, for me Chakra will still have a breaking change.

[BANOnotIT]

for me Chakra will still have a breaking change.

Got it. Then yeah, I think then only revert is a right thing to do.

cc @segunadebayo

[ShawnFumo]

I understand the thinking for this (always good to help with accessibility!), but is a bit frustrating that seemingly a decision was made to revert back in August and it never happened. Is there anything I can do to help this along? It is a moot point for us now (deploying a fix to prod today), but would like to avoid any other companies getting the unfortunate surprise that we did. Maybe we could get the original patch comment into the full changelog as well, since it seems like it never got added there.

My personal opinion is to do a quick revert, then if the functionality is important enough, do a bigger version change to indicate it is a breaking change and use TypeScript's Omit feature to remove disabled from Button entirely. That way it won't even compile and will be very clear what is going on. While there could be a console warning of some sort, I feel like this is a perfect situation where TypeScript types would be doing their job.

I know this probably feels like it was a small change, but you never know how end users may be using it. We unfortunately happened to be using disabled in a basic wrapped component for forms, and happened to be using it instead of isLoading, probably due to a stylistic choice back when it was implemented. Somehow we didn't catch it during testing after updating Chakra, and ended up with a few weeks of no double-click protection on form submissions in prod. That includes duplicate transactions sent to banks, needing ops and dev time to investigate, queries on all our clients' DBs to search for possible existing duplicates, reversal transactions created, damage control w/ clients, etc. Hoping we can avoid anyone else having to deal with this kind of headache if possible.

[Philzen]

My personal opinion is to do a quick revert, then if the functionality is important enough,

@ShawnFumo Yes, reverting that change would be most appreciated. Otherwise our current codebase is stuck with an older ChakraUI version, b/c we need to be able to set isLoading={true} without the button losing focus (which it does when being disabled).

[augustoabreu]

@Philzen any updates on this? this is a breaking change that was wrongly introduced and it's been hanging for a while...

[Philzen]

@Philzen any updates on this? this is a breaking change that was wrongly introduced and it's been hanging for a while...

@augustoabreu i've now created #8462 which simply reverts this breaking change. Just checked on the docs playground and it seems to work as expected again, try for yourself: https://chakra-ui-website-git-fork-philzen-7269-7816-7-fafd11-chakra-ui.vercel.app/docs/components/button

[segunadebayo]

Merged a recent PR for this already