fix(ion-button): sync disabled state in ion-button renderHiddenButton#31225
fix(ion-button): sync disabled state in ion-button renderHiddenButton#31225Zac-Smucker-Bryan wants to merge 5 commits into
Conversation
|
@Zac-Smucker-Bryan is attempting to deploy a commit to the Ionic Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
thetaPC
left a comment
thetaPC
left a comment
There was a problem hiding this comment.
Small thing on the PR title: fix(ion-button): sync disabled state in ion-button renderHiddenButton
Two tweaks. The scope convention here uses the component name without the ion- prefix (for example fix(button), fix(modal), fix(checkbox)), so the scope should be button. And since the scope already says it's about the button, repeating ion-button in the description is redundant. Suggested:
fix(button): sync disabled state in renderHiddenButton
Small thing on the PR description:
The issue link in the description won't auto close the issue. GitHub's closing keywords only fire when the keyword is followed directly by # and the number, like resolves #30968. Here it's resolves #[30968](...), so the # is followed by a [ and GitHub doesn't parse it as an issue reference. The markdown link renders fine visually but the linkage and auto close on merge won't happen.
Please change it to:
Issue number: resolves #30968
GitHub will turn that into a link on its own, so the markdown link isn't needed.
Once you push your latest changes, make sure that the PR description is still valid.
There was a problem hiding this comment.
We need to update the following comment to reflect the changes and why they were added.
/**
* If the form already has a rendered form button
* then do not append a new one again.
*/
| * then do not append a new one again. | ||
| */ | ||
| if (formButtonEl !== null && formEl.contains(formButtonEl)) { | ||
| formButtonEl.disabled = this.disabled; |
There was a problem hiding this comment.
We should also keep type in sync on the hidden button. The early return path never updates it, so a runtime change from submit to reset (or vice versa) leaves the hidden button with its original type and it performs the wrong form action.
| formButtonEl.disabled = this.disabled; | |
| formButtonEl.disabled = this.disabled; | |
| formButtonEl.type = this.type; |
| expect(submitEvent).toHaveReceivedEvent(); | ||
| }); | ||
|
|
||
| test('should submit the form when disabled state changes asynchronously', async ({ page }, testInfo) => { |
There was a problem hiding this comment.
This test does not actually catch the regression. It still passes without the fix. I verified by reverting the formButtonEl.disabled = this.disabled; line, keeping the test, and rebuilding: it stays green.
The reason is timing. The bug only happens when the disabled change races with Stencil's initialization, which is the window where @Watch('disabled') gets missed. This test changes disabled with el.evaluate and setTimeout after the component has fully loaded, and by that point the watch is active again and syncs normally. So it reduces to "set disabled to false after load," which is the same as the manual workaround in the issue (clear the input, then retype) that already worked before your fix. Also worth noting that el.disabled = true is a no-op there since the button already starts disabled in the markup.
There is no post-load DOM API to suppress the watch, so core e2e cannot reproduce this one deterministically. The faithful reproduction needs a framework binding that sets disabled during init, which is exactly the Angular async validator scenario from the issue.
Please move the regression test to packages/angular/test/base/e2e/src/lazy/form.spec.ts. Set up a submit button that starts disabled and has its disabled state flip to false asynchronously during init (an async validator resolving, or a Promise.resolve().then(...) in ngOnInit), then assert the form submits on click. Confirm it fails without the fix before pushing, since that is the whole point of the test.
If you are feeling adventurous, add the same coverage to the standalone test app too.
There was a problem hiding this comment.
@thetaPC Thanks for the detailed feedback - it helps me learn and I appreciate it. I have a version of the test, in form.spec.ts that I would like to test before making the changes in button.tsx (mine plus the one you suggested in #31225 (comment)), to be pushed later once I can confirm it fails before the push. But, I am struggling to get e2e running in the Angular testing folder in a way that I wasn't when the test was in core. Any guidance?
The link to Ionic's E2E testing guide is 404ing for me and I didn't quite know from the docs in https://github.com/ionic-team/ionic-framework/blob/main/docs/core/testing/README.md or https://github.com/ionic-team/ionic-framework/blob/main/docs/angular/testing.md about how to perform the E2E Angular tests specifically.
…enButton This reverts commit 1d4c704. Will retest changes without the fix and new test location
Zac-Smucker-Bryan
force-pushed
the
zsb-ionic-framework-clean
branch
from
66bb0b2 to
c986c8e
Compare
…e (issue ionic-team#30968) This reverts commit 2964bc6.
Zac-Smucker-Bryan
force-pushed
the
zsb-ionic-framework-clean
branch
from
c986c8e to
5fb20dd
Compare
2 participants
Issue number: resolves #30968
What is the current behavior?
<ion-button>with type="submit" does not cause the associated<form>to be submitted even though it is not disabled. When the text of the input field is cleared and then input re-added, you can submit the input.What is the new behavior?
<ion-button>with type="submit" should always submit the associated<form>when it is clicked and not disabled.formButtonEl.disabled = this.disabled;to button.tsx inrenderHiddenButton(). This addition syncs the disabled status of the button when that happens.Does this introduce a breaking change?
Other information
It seems to be related to a stencil update that affected when @watch('disabled') fires. As far as I know it is not related to Angular directly.