Fix hidden with value should be blank to logic #51
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Added comprehensive tests that expose the bug where hidden field values incorrectly affect other fields' visibility logic. Go Unit Tests (go/validate_test.go): - TestCascadingVisibilityBugFix with 5 scenarios: 1. Cascading visibility - Field C depends on hidden Field B value 2. Deep cascading visibility - A→B→C→D all cascade 3. EqualsField with hidden target field 4. StringContains with hidden field 5. Multiple conditions with one field hidden Playwright E2E Tests (e2e/cascading-visibility-bug.spec.ts): - 4 scenarios × 3 browsers = 12 tests: 1. Cascading visibility - Original bug reproduction 2. Deep cascading - 4-level dependency chain 3. StringContains with hidden field 4. EqualsField with hidden target field All tests currently FAIL as expected, demonstrating the bug where: - Hidden fields retain values in form submissions - These values incorrectly trigger visibility of dependent fields - Example: car_brand='Toyota' (hidden) still shows prefer_japanese field The next commit will implement the fix to make these tests pass.
Implemented fix for bug where hidden field values incorrectly affected other fields' visibility. Now sanitizes form values by removing hidden field values before checking visibility rules. Server-Side (Go): - Added sanitizeFormValues() function in go/validate.go - Iteratively removes values for hidden fields until stable state - Integrated into ValidFormValues() before validation - Handles cascading dependencies, circular refs, deep nesting Client-Side (Elm): - Added sanitizeFormValues() and helper in src/Main.elm - Mirrors Go implementation with iterative stabilization - Updated OnFormValuesUpdated to sanitize after user input - Prevents hidden field values from affecting browser visibility Implementation Details: - Max 10 iterations to prevent infinite loops (typically 1-2 needed) - O(N×D) complexity where N=fields, D=depth - No breaking signature changes - Handles all comparison types: Equals, StringContains, EndsWith, GreaterThan, EqualsField Test Results: - All 5 Go unit test scenarios now PASS - All 12 Playwright E2E tests now PASS (4 scenarios × 3 browsers) - No regressions in existing tests - Comprehensive coverage of edge cases Example Fix: Before: car_brand='Toyota' (hidden) → prefer_japanese shows (bug) After: car_brand removed during sanitization → prefer_japanese hidden (correct) Files Modified: - go/validate.go (+44 lines) - src/Main.elm (+62 lines) - dist/tiny-form-fields.esm.js (rebuilt) - dist/tiny-form-fields.js (rebuilt)
✅ Deploy Preview for tiny-form-fields ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Owner
Author
|
Expected to fail tests without code change. Followup commit should make tests pass |
…e variables eliminates the risk of using the wrong variable since there's only one clear variable name now
… of hardcoded `10` > Maximum cascade depth = N-1 for N fields (worst case: all fields in a linear dependency chain) > ... > Trade-offs: A form with 1000 fields could theoretically iterate 1000 times but doing fewer than 1000 iterations for such a form with all fields in a linear dependency chain would be incorrect and thus unacceptable
choonkeat
force-pushed
the
fix-hidden-with-value-should-be-blank-to-logic
branch
from
f64cfc0 to
5b3251b
Compare
choonkeat
force-pushed
the
fix-hidden-with-value-should-be-blank-to-logic
branch
from
5b3251b to
e3cbd9f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix cascading visibility bug - hidden field values should not affect other fields
The Problem
Hidden fields keep their values, and these values incorrectly affect other fields' visibility.
Bug example:
The hidden "Car brand" field still has value "Toyota", incorrectly triggering the dependent field.
The Fix
Remove hidden field values before checking visibility rules.
Implementation:
sanitizeFormValues()that iterates through fields and removes values for hidden onesTest Coverage
Go unit tests: 5 scenarios covering cascading, deep nesting, all comparison types
Playwright E2E: 4 scenarios × 3 browsers = 12 tests
All pass ✅. No regressions.
Files Changed
go/validate.go(+44 lines)go/validate_test.go(+300 lines)src/Main.elm(+66 lines)e2e/cascading-visibility-bug.spec.ts(+275 lines, new file)Breaking Changes
Forms relying on hidden field values affecting visibility will behave differently. This is a bug fix, so most forms will see improved behavior.