fix(button): restore disabled state set from outside when loading sta…

[EndzeitBegins]

…te changes to default

Signed-off-by: EndzeitBegins 16666115+EndzeitBegins@users.noreply.github.com

PR Checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)
• If applicable, have a visual design approval

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• clarity.design website / infrastructure changes
• Other... Please describe:

What is the current behavior?

Issue Number: #129

What is the new behavior?

When changing to the loadingState to default the disabled value set from outside is restored, whether is was set before or while the loadingState was non-default.

Does this PR introduce a breaking change?

• Yes
• No

Other information

My solution includes replacing the field disabled in CdsBaseButton with a corresponding getter and setter in order to override its behavior in CdsButton. This in turn adds a private field _disabled to CdsBaseButton. To avoid collisions I renamed _disabled to _disabled_from_outside in CdsButton.
According to npm run public-api:check this results in changes to the "public" API, even though only private fields were added / renamed and the field disabled replaced with getter / setter.

[vmwclabot]

@EndzeitBegins, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

[ashleyryan]

I'm confused by this comment, by default the loadingState is the default loading state

[ashleyryan]

Can you keep the existing names and implementations for these unit tests. I think it's helpful to have tests where the implicit behavior happens - ie, no one ever set the disabled status before mucking with the loading states

[ashleyryan]

Thanks for taking a stab at this, I'm reviewing and looking through it now. I don't love the idea of completely disregarding the value attempting to be set in the setter.

We already had the _disabled hidden variable that was tracking the manually set disabled status. And uses it to set the disabled value when the loading state changes back to default. I'm trying to first understand why this isn't doing the job in Angular. Is it because Angular is bulk updating the web component properties at the same time (settings loadingState and disabled at the same time?) or in a different order than expected (setting loadingState before disabled)? That would help me better understand your fix.

Likewise, I'm not sure I follow your unit tests updates and what they're attempting to do that's different than what they did before.

[vmwclabot]

@EndzeitBegins, VMware has approved your signed contributor license agreement.

[EndzeitBegins]

Thank you for taking a look at my proposal @ashleyryan.

There were two tests which ensured that the disabled state (true / false), which is set while the loadingState is "default", thus before changing the loadingState to "loading", is restored, once the loadingState becomes "default" again. I just renamed them and set the disabled=false explicitly to highlight the difference between the two tests.
This use case works.

However, I added two tests which reproduce the problem I originally encountered using the Angular integration. As I'm able to reproduce this without Angular being involved, I'm quite certain this is a bug in the core framework.
What my test case shows, is that when changing the disabled state when the loadingState is non-default, e.g. "loading", the value is not restored once the loadingState is set back to "default" again.

I'm haven't worked with lit before but from what I was seeing this is due to the outside "change" in disabled not being registered, as disabled is true already due to the loadingState being "loading".

Also, in my proposal the disabled state is never completely disregarded. The opposite is the case, once the "default" loadingState is reached again, the disabled state set from outside is applied again - in contrast to the current implementation, which just ignores that disabled=true was being set while the loadingState was "loading".

---

Trying to summarize the difference:

// Assuming defaults
disabled = false;
loadingState = "default";

// Use case 1 - setting before loadingState / with default 
disabled = true;
loadingState = "loading";
loadingState = "default"; // return back to "default"
-> disabled is true (which is correct imo)

// Use case 2 - setting after loadingState / with non-default
loadingState = "loading";
disabled = true;
loadingState = "default"; // return back to "default", restores value before loadingState changed 
-> disabled is false (which is incorrect imo)

[ashleyryan]

I was playing around with my own implementation loosely based off of yours and I was able to get around having this conditional logic here. I think we can always call super here and it still more or less works the same.

[ashleyryan]

I'm trying to remember why, but I think there was a reason I had to have the undefined check here in the last PR. Otherwise I think disabled gets called if other prop values change - we only want to run this is loadingState is the prop changing

[ashleyryan]

I remember why - props always had loadingState, but unless it's being changed, it's undefined. I found that without the undefined check, the else statement was being called on the initial render when loadingState was set, but the button hadn't been painted to the dom yet so the width was 0px.

I added a test for this situation.

[ashleyryan]

This is causing some of the react snapshot/unit tests to fail and I'm investigating why.

[EndzeitBegins]

Thank you @ashleyryan for investing time looking into this issue.
I'm not quite certain whether there is something I can help you with at this moment.
Let me know if I can be of any help.

Should I change my pull-request in any way? Or do you simply take over the issue?

---

I've found a slightly different solution that also passes the added tests. But still fails the aforementioned snapshot tests, as lit seems to add the attribute disabled="true" instead of former disabled="" to the DOM. Sadly I don't know why that is the case or whether that is an actual problem or not.
However it no longer needs the conditional logic in the setter as you've lined out.

The idea is to basically use the getter to determine the disabled state both on the value stored in the parent and the loadingState:

  get disabled(): boolean {
    return super.disabled || !this.isDefaultLoadingState(this.loadingState);
  }

  set disabled(value: boolean) {
    super.disabled = value;
  }

Coupled with notifying lit about the potential update whenever the loadingState changes, this seems to have the same effect, without requiring an additional private property or explicit setting of the disabled state.

  update(props: PropertyValues<this>) {
    if (props.has('loadingState')) {
      // change in loadingState might also affect disabled state
      this.requestUpdate('disabled', super.disabled);

      if (this.isDefaultLoadingState(this.loadingState)) {
        this.style.removeProperty('width');
      } else {
        this.style.width = getElementWidth(this);
      }
    }

    super.update(props);
  }

With this approach, the override of firstUpdated can be dropped altogether.

[ashleyryan]

I can either take the PR over or you can try and make some of the changes you or I have suggested.

Why don't you try updating your commit to have the alternative implementation, I like that approach. We also need to update the commit message to include a link to the original issue. So update it to something like

fix(button): restore disabled state when loading state finishes

fixes #130

I've spent most of the day digging into why lit is doing that with boolean attributes and I've been talking to them. I've narrowed it down to something not working correctly when using a getter or setter vs a simple property. I think it's fine and I can update the react tests if necessary. But let's try the updated solution for now.

I'm also happy to take it over, if you'd like me to.

[ashleyryan]

I ended up taking it over @EndzeitBegins . I couldn't get your suggested approach to work - even though the tests were passing I was finding that in storybook when I was manipulating the component with

document.querySelector('cds-button').loadingState = 'loading';
document.querySelector('cds-button').loadingState = 'default';

That it wasn't losing the disabled status for some reason.

[EndzeitBegins]

Thanks for looking into this and finding a solution that works with the remaining use cases, @ashleyryan.

I hope my initial test cases were helpful in aiding the way at least. Looking forward to the merge!

[github-actions]

🎉 This PR is included in version 6.1.1 🎉

The release is available on:

• GitHub release
• v6.1.1

Your semantic-release bot 📦🚀

[github-actions]

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed PRs after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.