fix(button): sync disabled state in renderHiddenButton#31225
fix(button): sync disabled state in renderHiddenButton#31225Zac-Smucker-Bryan wants to merge 15 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.
| 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.
I added a test to packages/angular/test/base/e2e/src/lazy/form.spec.ts that compares both states and validates them as expected. The test fails if you comment out the suggested changes AND if you comment out the watch decorator for the disabled state in line 62 of button.tsx. It then works if you leave the watch decorator commented out but apply the changes OR if you add the decorator back in and also apply the changes. It seemed like the watch decorator was firing successfully always in the test environment, and as a consequence, that the test is at least good enough way to verify that if the watch decorator doesn't fire successfully, the fix should catch it.
I had a hard time replicating the race condition and wasn't sure if you wanted form.component.ts to be updated as well with ngOnInit (or else, a separate test component and ts file for this particular issue). When I was experimenting with other forms of testing and trying to use more of the Promise.resolve().then(...) in ngOnInit (and a couple of other things I tried to do with https://angular.dev/api/forms/AsyncValidatorFn), I kept getting timeout errors in the test. Let me know if you want want me to keep at your original suggestion and I will take another look.
There was a problem hiding this comment.
Thanks for digging into this so thoroughly! Your experiments confirmed what the mechanism suggests: the disabled change is masked by @Watch('disabled') in any environment where the watch fires, so the only thing that exposes the bug is the rare init-timing fluke, and that is not something we can trigger reliably. Your timeouts with ngOnInit and AsyncValidatorFn are that flakiness showing up.
When a regression is this hard to reproduce reliably, our approach is to skip the automated test rather than ship a flaky or misleading one. Instead we create a dev build and verify the fix directly against the reproduction from the issue. A test that passes whether or not the fix is present gives us false confidence, so we would rather have no test and a confirmed manual verification on the original repro. So please remove the disabled test.
One note on the dev build: only an Ionic team member can create one, so it isn't something you can run on your end. I'll see if I can get one going, though since this PR is on a fork it might not work, and I'll follow up here either way.
The type sync is a different case. There is no @Watch('type'), so the line you added is the only thing keeping the hidden button in sync, which means we can test it reliably. Please add a small e2e test that creates an ion-button with type="submit" inside a form, changes type to "reset" after load, and asserts the hidden form button reflects the new type. That test fails without the fix and passes with it, so it is worth keeping.
So the split is: no test for disabled (verified manually on a dev build instead), one deterministic test for type.
…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
Imports AsyncValidatorFn in form.component.ts for new function Adds test to form.spec.ts to simulate disabled state
This reverts commit 963ae43
Removing extra space in code comment on 2 lines
Test gets disabled state and tests if they match for both initial state and after set-values is called to simulate filling out the form.
Zac-Smucker-Bryan
changed the title
This reverts commit 2964bc6.
This reverts commit 2964bc6.
…ucker-Bryan/ionic-framework into zsb-ionic-framework-clean
There was a problem hiding this comment.
Two small description fixes.
Title: the PR now syncs both the disabled state and the type, but the title only mentions disabled. Please update it to cover both, for example:
fix(button): sync disabled state and type in renderHiddenButton
Issue linking: the resolves line still won't auto-close the issue on merge. It's currently resolves #[30968](...), and GitHub only treats it as a closing reference when the keyword is followed directly by # and the number. Please change it to:
Issue number: resolves #30968
GitHub turns that into a link on its own, so the markdown link isn't needed.
One process note for next time. The commit history here is hard to follow because it has several revert: commits that undo and redo the same changes, plus a merge commit from pulling the branch into itself. When you want to undo something you just committed, it's cleaner to manually remove that change and commit the corrected state, rather than adding a revert: on top, since each revert leaves both the original and the undo in the log and muddies the history.
It's not blocking and the final state is what matters once this is squashed, but a clean history makes the PR much easier to review as we go.
| }); | ||
| }); | ||
|
|
||
| test('should keep hidden submit button disabled state in sync', async ({ page }) => { |
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?
The visible and hidden button should always have the same disabled state.
<ion-button>with type="submit" should always submit the associated<form>when it is enabled.formButtonEl.disabled = this.disabled;andformButtonEl.type = this.type;to button.tsx inrenderHiddenButton(). This addition syncs the disabled status and type of the button when that happens.packages/angular/test/base/e2e/src/lazy/form.spec.ts.to test the behavior. This test gets the state of both the visible and hidden buttons, and compares both to each other and to what is expected given if the form is filled out or not.Does this introduce a breaking change?
Other information
Related to a stencil update that affected when @watch('disabled') fires.