FORMS-25983 Add min/max cross-validation to authoring dialogs by AnurudraS · Pull Request #1910 · adobe/aem-core-forms-components

AnurudraS · 2026-06-23T06:42:32Z

Description

#1903

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes and the overall coverage did not decrease.
All unit tests pass on CircleCi.
I ran all tests locally and they pass.

codecov · 2026-06-23T07:05:44Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

AnurudraS · 2026-06-23T08:43:01Z

// todo: leadDigits and fracDigits are not supported as of today
commented by @rismehta in 2024 the check is not always failing but the code is there.

AnurudraS · 2026-07-13T06:56:52Z

Another closed PR was #1903

devgurjar

PR Review: FORMS-25983 Add min/max cross-validation to authoring dialogs

Summary

Adds authoring-time cross-validation preventing a min value greater than its paired max across textinput, numberinput, fileinput, datepicker, datetime, and the container components. Two shared helpers in utils.js provide real-time inline feedback (handleMinMaxValidation) and submit-time blocking (registerMinMaxValidator). The core mechanism is sound and the commit history shows genuine iteration on real bugs. However, a required CI check is currently failing (merge blocker) and the PR carries an undeclared, untested change. Inline comments below pin the specific items.

Change Surface

12 files, +344/-4. Client-side authoring JS + dialog XML + 2 Cypress specs.
New public API: Utils.handleMinMaxValidation, Utils.registerMinMaxValidator.
Scope: creep — redirect URL validation is unrelated to min/max (see inline).

Test Gaps

Only numberinput and textinput have Cypress coverage. datepicker, datetime, fileinput, and the container minOccur/maxOccur path are all wired up but untested — including the DATE_COMPARE code path and the shared foundation-contentloaded listener in utils.js. The new forms.redirect.absolutepath validator also has no test.

Security

No issues. Client-side authoring-only validation — no secrets, no injection sinks, no new network/permission/telemetry surface.

Nits (LOW, non-blocking)

New error strings are passed to Granite.I18n.getMessage() as English keys but no i18n dictionary entries were added — they will render untranslated in non-English locales.
Message strings are duplicated verbatim at each registerMinMaxValidator + handleMinMaxValidation call site; the MIN_MAX_PAIRS table pattern used for panelcontainer could DRY this up.

Verdict

Request changes — the failing required validate_test_files check is a hard merge blocker. Alongside that, the redirect-validation scope creep should be split out and the untested components should get coverage. The core min/max design is good and the security posture is clean.

Reviewed with ❤️ by Claude Code 🤖 and Scout

devgurjar · 2026-07-28T07:56:57Z

        });
 });

+        it('shows inline error when minimum is set greater than maximum', function () {


[CRITICAL] Merge blocker — required validate_test_files check is failing.

The required check greps every modified .cy.js for it.only|describe.skip|it.skip and fails the build if any is present. By adding new tests to this file you pull the pre-existing it.skip('verify editFormat Value Getting saved correctly') (line 184) into scope, so the gate now fails. The same applies to textinput.authoring.cy.js:148 (it.skip('pasted component should have unique name')).

CI log: Test file ...numberinput.authoring.cy.js contains 'it.only', 'describe.skip', or 'it.skip'. This is not allowed.

Resolve before merge — either un-skip/remove the dormant tests, or move the new tests into a separate spec that doesn't carry a skip.

Reviewed with ❤️ by Claude Code 🤖 and Scout

devgurjar · 2026-07-28T07:56:57Z

    }

+    $(window).adaptTo("foundation-registry").register("foundation.validation.validator", {
+        selector: "[data-validation~='forms.redirect.absolutepath']",


[MEDIUM] Undeclared scope creep + no test.

This redirect absolute-path validator (and the validation="forms.redirect.absolutepath" attributes added to the v1/v2 dialog XML) is unrelated to the PR's stated min/max cross-validation scope. It was introduced in commit 0f04ff9cb, whose message is entirely about "Fix two bugs in handleMinMaxValidation" — so the change is undocumented in its commit and has zero test coverage.

Please split it into its own PR/commit with its own test. (The wiring itself is fine — v2's editor clientlib depends on core.forms.components.container.v1.editor, so this v1-registered validator does reach the v2 dialog.)

Reviewed with ❤️ by Claude Code 🤖 and Scout

devgurjar · 2026-07-28T07:56:57Z

                                      fieldLabel="Redirect URL/Path"
                                      rootPath="/content"
                                      name="./redirect"
+                                      validation="forms.redirect.absolutepath"


[MEDIUM] Part of the unrelated redirect-validation change (see the editDialog.js comment). This validation attribute belongs in a separate PR scoped to redirect validation, with test coverage.

Reviewed with ❤️ by Claude Code 🤖 and Scout

devgurjar · 2026-07-28T07:56:57Z

+                function validate() {
+                    var minVal = minField.value, maxVal = maxField.value;
+                    var invalid = !!(minVal && maxVal && compare(minVal, maxVal));
+                    minField.invalid = invalid;


[MEDIUM] validate() clobbers sibling error state.

When the cross-check passes this sets minField.invalid = maxField.invalid = false unconditionally and never clears errorMessage. Because invalid = !!(minVal && maxVal && compare(...)) is false whenever either field is blank, this can transiently overwrite an invalid state set by another validator (e.g. required), and a stale cross-validation errorMessage can resurface later when the field is marked invalid for a different reason.

The commit trail (commit 2 removed direct mutation → commit 3 reverted it to avoid recursion) shows this is a known compromise. Please add a code comment documenting why direct mutation is used, and clear errorMessage in the valid branch.

Reviewed with ❤️ by Claude Code 🤖 and Scout

- Move new min/max Cypress tests to separate spec files (numberinput.minmax.authoring.cy.js, textinput.minmax.authoring.cy.js) so validate_test_files CI check no longer scans files with pre-existing it.skip entries - Remove redirect URL validator scope creep from container v1 editDialog.js and the validation attribute from v1/v2 dialog XML — unrelated to min/max - In handleMinMaxValidation validate(), clear errorMessage in the valid branch and add comment explaining why direct mutation is used instead of event dispatch (avoids infinite recursion via jQuery trigger) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

adobe-bot · 2026-07-28T10:31:11Z

Accessibility Violations Found

Id	Impact
aria-required-attr	critical
empty-heading	minor
label-title-only	serious
page-has-heading-one	moderate
target-size	serious

adobe-bot · 2026-07-28T10:33:53Z

Accessibility Violations Found

Id	Impact
aria-required-attr	critical
empty-heading	minor
label-title-only	serious
page-has-heading-one	moderate
target-size	serious

adobe-bot · 2026-07-28T10:34:59Z

Accessibility Violations Found

Id	Impact
aria-required-attr	critical
empty-heading	minor
label-title-only	serious
target-size	serious

adobe-bot · 2026-07-28T10:35:41Z

Accessibility Violations Found

Id	Impact
aria-required-attr	critical
empty-heading	minor
label-title-only	serious
target-size	serious

adobe-bot · 2026-07-28T11:48:01Z

Accessibility Violations Found

Id	Impact
aria-required-attr	critical
empty-heading	minor
label-title-only	serious
target-size	serious

adobe-bot · 2026-07-28T14:50:17Z

Accessibility Violations Found

Id	Impact
aria-required-attr	critical
empty-heading	minor
label-title-only	serious
target-size	serious

adobe-bot · 2026-07-28T14:50:37Z

Accessibility Violations Found

Id	Impact
aria-required-attr	critical
empty-heading	minor
label-title-only	serious
target-size	serious

adobe-bot · 2026-07-28T16:29:01Z

Accessibility Violations Found

Id	Impact
aria-required-attr	critical
empty-heading	minor
label-title-only	serious
target-size	serious

adobe-bot · 2026-07-28T16:33:01Z

Accessibility Violations Found

Id	Impact
aria-required-attr	critical
empty-heading	minor
label-title-only	serious
target-size	serious

Prevent authors from setting a minimum value greater than its paired maximum (minLength/maxLength, minimum/maximum, minItems/maxItems, minimumDate/maximumDate, minimumDateTime/maximumDateTime, minOccur/maxOccur) across all affected form components. All pairs are registered centrally in Utils so new components only need one entry added — no per-component editDialog.js changes required. Real-time inline feedback via change listeners; dialog save blocked via foundation.validation.validator registered once at page load. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

1. Fix parseInt bug for decimal ranges in numberinput — use NUMBER_COMPARE (Number()) instead of INT_COMPARE (parseInt()) so min=1.6 / max=1.4 is correctly flagged as invalid. 2. Remove direct field.invalid mutation from handleMinMaxValidation — trigger change events instead so the foundation-validation framework re-runs all validators and avoids overwriting required/pattern errors. 3. Wrap foundation-contentloaded handler in Coral.commons.ready so fields are fully upgraded before values are read; add early exit for non-dialog fragments. 4. Move per-component selector strings and registrations into each component's own editDialog.js (textinput, numberinput, fileinput/v1, datepicker, datetime) using that file's existing selector constants. utils.js now only hosts the panelcontainer entry, which is shared across accordion/wizard/tabsontop/verticaltabs/fragment (none have editDialog.js). 5. Fix DATE_COMPARE to guard against invalid date strings. 6. Add wrapperClass to datetime dialog XML for minimumDateTime/maximumDateTime fields, replacing the brittle attribute-name selector. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…eview fixes 1. Pass jQuery-wrapped dialog instead of raw e.target to the handler — dialog.find() requires jQuery; passing e.target caused TypeError on every panelcontainer/tabsontop/accordion dialog open. 2. Revert validate() to direct invalid mutation — triggering change events in the listener caused infinite recursion because jQuery trigger fires native DOM events which re-enter the same addEventListener callback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…max tests cy.type() on a coral-numberinput focuses its inner <input>, so chaining .blur() on the wrapper element fails Cypress's focused-element check. Split each chain to use cy.focused().blur() instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…numberinput coral-numberinput is a custom element not in Cypress's clearable list. Use .find('input') to target the native inner input before .clear()/.type(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- Move new min/max Cypress tests to separate spec files (numberinput.minmax.authoring.cy.js, textinput.minmax.authoring.cy.js) so validate_test_files CI check no longer scans files with pre-existing it.skip entries - Remove redirect URL validator scope creep from container v1 editDialog.js and the validation attribute from v1/v2 dialog XML — unrelated to min/max - In handleMinMaxValidation validate(), clear errorMessage in the valid branch and add comment explaining why direct mutation is used instead of event dispatch (avoids infinite recursion via jQuery trigger) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…g minmax authoring tests Extract duplicated error message strings into named constants in all five editDialog.js files (textinput, numberinput, fileinput, datepicker, datetime) so each message is defined once and referenced in both registerMinMaxValidator and handleMinMaxValidation calls. Add Cypress authoring specs for the four components not yet covered by minmax cross-validation tests: datepicker, datetime, fileinput, and panelcontainer. Each spec covers the inline-error and dialog-save-blocker scenarios in dedicated *.minmax.authoring.cy.js files to avoid touching existing files that carry pre-existing it.skip entries. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…and wait for field visibility Match the pattern from the existing fileinput authoring test: target [name='./multiSelection'][type='checkbox'] to hit the native input inside coral-checkbox, and gate on display:block before interacting with the min/max coral-numberinput fields. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…h before interacting with min/max fields min/maxOccur coral-numberinput fields are disabled by default and only become enabled when the 'Make panel repeatable' coral-switch is turned on. Click the switch and gate on not[disabled] before typing values. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…efore interacting with min/max fields The minimumFiles/maximumFiles coral-numberinput fields live on the Validation tab, not Basic. multiSelection checkbox (Basic tab) makes the wrapper divs display:block but the parent coral-panel stays display:none until the Validation tab is selected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…tion to fileinput dialog tabs All three fileinput dialog versions (v1/v2/v3) were missing jcr:title on the validation tab node. Without it the coral-tab header has no text, so cy.contains('Validation').click() matched hidden content inside the panel instead of the tab button, leaving the coral-panel display:none when the test tried to interact with min/max fields. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

adobe-bot · 2026-07-29T10:22:36Z

Accessibility Violations Found

Id	Impact
aria-required-attr	critical
empty-heading	minor
label-title-only	serious
target-size	serious

adobe-bot · 2026-07-29T10:23:31Z

Accessibility Violations Found

Id	Impact
aria-required-attr	critical
empty-heading	minor
label-title-only	serious
target-size	serious

adobe-bot · 2026-07-29T10:27:33Z

Accessibility Violations Found

Id	Impact
aria-required-attr	critical
empty-heading	minor
label-title-only	serious
target-size	serious

…programmatically to reliably trigger validate listener Switches from .find('input').clear().type() + blur() to directly setting coral-numberinput.value and dispatching a change event, matching the pattern used in the datepicker/datetime tests. Also adds {force:true} to the multiSelection checkbox check to bypass the position:fixed backdrop z-index issue, and waits for minField to be visible before interacting. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

adobe-bot · 2026-07-30T06:18:10Z

Accessibility Violations Found

Id	Impact
aria-required-attr	critical
empty-heading	minor
label-title-only	serious
target-size	serious

adobe-bot · 2026-07-30T06:19:29Z

Accessibility Violations Found

Id	Impact
aria-required-attr	critical
empty-heading	minor
label-title-only	serious
target-size	serious

AnurudraS requested a review from devgurjar June 23, 2026 08:44

devgurjar requested changes Jul 28, 2026

View reviewed changes

github-actions Bot force-pushed the FORMS-25983-dev branch from bc77460 to e22456b Compare July 28, 2026 09:52

Anurudra Shukla and others added 11 commits July 29, 2026 09:43

github-actions Bot force-pushed the FORMS-25983-dev branch from 7577ad9 to 4104450 Compare July 29, 2026 09:43

Uh oh!

Conversation

AnurudraS commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

codecov Bot commented Jun 23, 2026

Codecov Report

Uh oh!

AnurudraS commented Jun 23, 2026

Uh oh!

AnurudraS commented Jul 13, 2026

Uh oh!

devgurjar left a comment

Choose a reason for hiding this comment

PR Review: FORMS-25983 Add min/max cross-validation to authoring dialogs

Summary

Change Surface

Test Gaps

Security

Nits (LOW, non-blocking)

Verdict

Uh oh!

devgurjar Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

devgurjar Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

devgurjar Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

devgurjar Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Jul 28, 2026

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 28, 2026

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 28, 2026

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 28, 2026

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 28, 2026

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 28, 2026

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 28, 2026

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 28, 2026

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 28, 2026

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 29, 2026

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 29, 2026

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 29, 2026

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 30, 2026

Accessibility Violations Found

Uh oh!

AnurudraS commented Jun 23, 2026 •

edited

Loading