Make SpinnerButton form-aware for invalid form submissions

[aduth]

🛠 Summary of changes

As a follow-on to #7761, this aims to allow SpinnerButton to be self-aware of form validation issues, so that the button will not show the spinning animation if the form is prevented from submitting.

In #7761, the CaptchaSubmitButtonElement was explicitly preventing the SpinnerButton from spinning when the form was invalid, but this could be managed by the SpinnerButton itself.

[eric-gade]

If you make toggleSpinner an observedAttribute / callback you wouldn't need to set the attributes here -- they could just be reactive. Not sure if there's any real advantage thought because it might be more verbose in the end.

[aduth]

Are you thinking something like a spinner-active attribute for the element that we'd reactively disable/enable the button, in place of what we currently use the .spinner-button--spinner-active CSS class for? I quite like that.

[eric-gade]

Yeah, though on second thought might be cleaner to keep the toggleSpinner's setting of the button disabled attributes, like so:

static get observedAttributes(){
    return [
      'spinner-active'
    ];
  }

  attributeChangedCallback(name, oldVal, newVal){
    if(name === 'spinner-active'){
      this.toggleSpinner(!!newVal);
    }
  }

  toggleSpinner(isActive){
    if(!isActive){
      this.button.removeAttribute("disabled");
    } else {
      this.button.setAttribute("disabled", "");
    }
  }

And now you can just use spinner-button[spinner-active] as the selector for animation, rather than a class

[eric-gade]

Or better yet:

attributeChangedCallback(name, oldVal, newVal){
    if(name === 'spinner-active'){
      this.button.toggleAttribute('disabled', !!newVal);
    }
  }

[aduth]

I like this plan. I'd probably want the separate method since we'll still need to deal with the delayed display of the "action message". I might propose to separate out that refactoring to a quick follow-on pull request, to keep this one a bit more focused on submit handling.

[aduth]

This dependency on an external element when the node is connected is a bit clunky to deal with, but unfortunately the form's submit event is the most reliable way to know that all of the client-side validation has passed, which is critical both to know when captcha test should be run, as well as when the spinner button's animation should start.

[aduth]

It's an edge-case, but I was curious the specific behavior of custom elements when being moved around the page while already live / attached. It looks like they will call disconnectedCallback and then connectedCallback in its new position, which will behave how we want it to.

https://codepen.io/aduth/pen/rNZodWj?editors=1111

[zachmargolis]

LGTM