Skip to content

fix(ion-button): sync disabled state in ion-button renderHiddenButton #31225

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Zac-Smucker-Bryan wants to merge 5 commits into
base: main
Choose a base branch
from
+40 −1
Open

fix(ion-button): sync disabled state in ion-button renderHiddenButton #31225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1d4c704
fix(ion-button): sync disabled state in ion-button renderHiddenButton
Zac-Smucker-Bryan Jun 17, 2026
2964bc6
test(ion-button): add e2e test for async disabled state change (issue…
Zac-Smucker-Bryan Jun 17, 2026
31aff31
npm run lint.fix
Zac-Smucker-Bryan Jun 17, 2026
c235b79
revert: fix(ion-button): sync disabled state in ion-button renderHidd…
Zac-Smucker-Bryan Jun 18, 2026
5fb20dd
revert: test(ion-button): add e2e test for async disabled state chang…
Zac-Smucker-Bryan Jun 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion core/src/components/button/button.tsx
View file
Open in desktop

@thetaPC thetaPC Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
 */

Original file line number Diff line number Diff line change
Expand Up @@ -195,9 +195,13 @@ export class Button implements ComponentInterface, AnchorInterface, ButtonInterf

/**
* 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. Sync the
* disabled state and type in it changes after button
* creation (e.g., runtime property updates).
*/
if (formButtonEl !== null && formEl.contains(formButtonEl)) {
// formButtonEl.disabled = this.disabled;
// formButtonEl.type = this.type;
return;
}

Expand Down
35 changes: 35 additions & 0 deletions core/src/components/button/test/form-reference/button.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,41 @@ configs({ directions: ['ltr'], modes: ['ios'] }).forEach(({ title, config }) =>

expect(submitEvent).toHaveReceivedEvent();
});

test('should submit the form when disabled state changes asynchronously', async ({ page }, testInfo) => {

@thetaPC thetaPC Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Zac-Smucker-Bryan Zac-Smucker-Bryan Jun 18, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

testInfo.annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/30968',
});

await page.setContent(
`
<form>
<input type="text" />
<ion-button type="submit" disabled>Submit</ion-button>
</form>
`,
config
);

const submitEvent = await page.spyOnEvent('submit');
const button = page.locator('ion-button');

// Simulate async disabled state change — e.g. async validator resolving
await button.evaluate((el: HTMLIonButtonElement) => {
el.disabled = true;
setTimeout(() => {
el.disabled = false;
}, 0);
});

// Wait for the async change to settle
await page.waitForTimeout(100);

await page.click('ion-button');

expect(submitEvent).toHaveReceivedEvent();
});
});

test.describe(title('should throw a warning if the form cannot be found'), () => {
Expand Down