Skip to content

Fix required validation for calendar selection - #620

Open
CSWinnall wants to merge 1 commit into
devfrom
fix/req-datepicker-selection
Open

CSWinnall wants to merge 1 commit into
devfrom
fix/req-datepicker-selection

Conversation

@CSWinnall

Copy link
Copy Markdown
Collaborator

Summary

  • Revalidate _req date/time fields when a date is selected from the jQuery UI calendar.
  • Preserve the picker’s existing onSelect callback before running validation.

Changelog

  • Required date fields now enable form submission when users select a calendar date.

Validation

  • node --check KTL.js
  • git diff --check

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

🟢 Approval recommended

The change is small, scoped, and preserves existing datepicker behavior while addressing the missing validation trigger on calendar selection.

Pull request overview

This PR fixes required field validation for date/time inputs when users select a date via the jQuery UI datepicker, ensuring the submit button state updates even when a native change event is not emitted. It does so by wiring a datepicker onSelect wrapper that preserves any existing callback and then triggers the existing required-field validation logic.

Changes:

  • Wires required text-input fields to also bind a datepicker onSelect handler when the input is a jQuery UI datepicker.
  • Preserves any pre-existing datepicker onSelect handler before running required-field revalidation.
  • Triggers revalidation asynchronously after date selection to keep submit-button state in sync.
File summaries
File Description
KTL.js Adds datepicker onSelect wiring so required date fields revalidate correctly after calendar selection while preserving existing callbacks.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI left a comment

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.

🟢 Approval recommended

The change is narrowly scoped, preserves existing datepicker behavior, and safely triggers the existing validation path to address the missing revalidation on calendar selection.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@cortexrd cortexrd left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Automated review (Claude Code, run by Norm). Summary: installing an onSelect on required date inputs suppresses the native change event jQuery UI would otherwise fire, which regresses persistent form, KTL.fieldValueChanged, the app onFieldValueChanged callback and Knack's to_date sync, while not changing behavior for stock Knack (calendar picks already fire change.ktl_req today). Details inline. Recommended direction: one load-time wrapper around $.datepicker._selectDate that re-triggers change after any onSelect, plus a decision on which date_time sub-inputs _req covers (the empty sibling time input is the likely actual cause of the reported symptom).

Comment thread KTL.js
|| datePickerInput.data('ktlReqDatePickerBound')) return;

const existingOnSelect = datePickerInput.datepicker('option', 'onSelect');
datePickerInput.datepicker('option', 'onSelect', function (dateText, instance) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Regression: installing an onSelect suppresses jQuery UI's native change.

In Knack's bundled jQuery UI, _selectDate does onSelect ? onSelect.apply(...) : inst.input.trigger('change'). Knack's form init sets no onSelect, so today a calendar pick fires change. After this PR, on any required date field a calendar pick fires only this wrapper, so:

  • the .kn-input-date_time change hook (~6754) never runs, so persistentForm.ktlOnFieldValueChanged never saves the date (the input/focusout path ~8324 explicitly skips .knack-date, so there is no fallback: reload and the picked date is gone)
  • KTL.fieldValueChanged and the public ktl.fields.onFieldValueChanged callback never fire
  • ktlCond .one('change') subscriptions go dead
  • Knack's own $('#view-field').change(...) that copies the date into input[name=to_date] for calendar-format fields stops, so the submitted event has a stale end date

Side effect: with an onSelect installed, jQuery UI's Enter-key branch calls onSelect instead of _hideDatepicker(), so Enter no longer closes the calendar on required date fields.

Minimum fix: else datePickerInput.trigger('change') in the wrapper. Better: see the comment at 18433.

Comment thread KTL.js
* @param {HTMLInputElement} input - Datepicker input to wire.
* @returns {void}
*/
function wireDatePickerValidation(input) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The premise doesn't hold for stock Knack. renderForm runs synchronously before knack-view-render and attaches no onSelect, so jQuery UI already triggers change on every calendar pick, and the existing change.ktl_req binding (18219-18223) plus the 6754 -> KTL.fieldValueChanged -> 18450 path already call validateNonEmptyTextField. The only case where change is suppressed is an app-installed onSelect. Was the symptom reproduced on a stock form? If the real trigger is an app onSelect or the empty time sibling (see 18232), this change doesn't address it.

If the goal is to make calendar picks fire change regardless of who set onSelect, the general fix is about six lines, once at load:

const origSelectDate = $.datepicker._selectDate;
$.datepicker._selectDate = function (id, dateStr) {
    const inst = this._getInst($(id)[0]);
    origSelectDate.apply(this, arguments);
    if (inst && this._get(inst, 'onSelect')) inst.input.trigger('change');
};

That restores change for every consumer and makes wireDatePickerValidation, the flag and the per-render wiring unnecessary.

Comment thread KTL.js
|| datePickerInput.data('ktlReqDatePickerBound')) return;

const existingOnSelect = datePickerInput.datepicker('option', 'onSelect');
datePickerInput.datepicker('option', 'onSelect', function (dateText, instance) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

datepicker('option', ...) is not a pure setter. _optionDatepicker calls _getDateDatepicker(el, true) -> _setDateFromField -> _setDate, which re-parses and rewrites the input value. Since fieldIsRequired awaits validateKtlCond before wiring, this lands after _cfdt / _sfv / persistent-form restores or app prefill: an ISO 2026-09-11 or a partially typed value in a required date field is silently blanked (then the initial pass marks it empty and disables Submit), and a parseable 9/1/2026 is rewritten to 09/01/2026. Both happen via .val() with no event, so nothing else notices.

It also does _curInst == inst && _hideDatepicker() and unconditionally _updateDatepicker(inst), which rebuilds the single shared #ui-datepicker-div and rebinds its day cells to the wired input. A calendar open on another field when a required date field is wired (late async wiring, refreshView, a second form rendering) switches to the required field's month and the next day click writes into the wrong input.

Side-effect-free alternative if the per-input override is kept: $.data(input, 'datepicker').settings.onSelect = fn (jQuery UI's _get reads settings dynamically).

Comment thread KTL.js
|| typeof datePickerInput.datepicker !== 'function'
|| datePickerInput.data('ktlReqDatePickerBound')) return;

const existingOnSelect = datePickerInput.datepicker('option', 'onSelect');

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Chaining existingOnSelect is order-dependent. Wiring runs after await Promise.all([processFieldKeywords(), processViewKeywords()]), i.e. after every synchronous knack-view-render / scene-render handler. _optionDatepicker uses extendRemove(inst.settings, opts), a plain overwrite, so any app code that sets onSelect later replaces KTL's wrapper, and ktlReqDatePickerBound then prevents re-wiring. Real app code does exactly this (OIT Project Portfolio Manager calls datepicker('option', 'onSelect', fn) from knack-scene-render.scene_147 with a 100 ms retry), so the motivating scenario is lost again on that path.

Comment thread KTL.js
});
inputField.each((_, input) => wireDatePickerValidation(input));
// Initial validation pass
inputField.each((_, el) => validateNonEmptyTextField(viewContainer, el));

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Likely actual root cause, left untouched. inputField is every non-hidden input under [data-input-id]. Knack's date_time template renders type="text" name="time" whenever time_format != 'Ignore Time', prefilled only when time_type != 'none'. With date-default None + time-default None (the configuration where the date is empty on a new record), the time input renders empty, keeps ktlNotValid_empty, and hasVisibleEmpty (~18420) keeps Submit disabled after the calendar pick. The wrapper revalidates only the date input, so this PR changes nothing for that case. The fix belongs in deciding which sub-inputs _req means for date_time (date only, or date+time per format).

Comment thread KTL.js

setTimeout(() => validateNonEmptyTextField(viewContainer, input), 0);
});
datePickerInput.data('ktlReqDatePickerBound', true);

@cortexrd cortexrd Sep 11, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

ktlReqDatePickerBound is never cleared and outlives the datepicker instance: datepicker('destroy') removes only the datepicker data and marker class. refreshView re-runs fieldIsRequired on the same DOM, where this guard skips wiring while change.ktl_req is rebound fresh, so the onSelect keeps pass-1 closures. Any destroy + re-create or later onSelect set leaves a fresh instance with no KTL onSelect and a flag that blocks re-wiring for the life of the element. A stateless check avoids the flag entirely: tag the wrapper function (wrapper.ktlReq = true) and skip when existingOnSelect?.ktlReq is set.

Comment thread KTL.js
if (typeof existingOnSelect === 'function')
existingOnSelect.call(this, dateText, instance);

setTimeout(() => validateNonEmptyTextField(viewContainer, input), 0);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Nits:

  • setTimeout(..., 0) buys nothing: _selectDate does inst.input.val(dateStr) before calling onSelect, so the value is already set. This makes it the only async validation path in fieldIsRequired.
  • typeof datePickerInput.datepicker !== 'function' (18436) is unreachable: hasDatepicker is only ever added by jQuery UI's _connectDatepicker / _inlineDatepicker.
  • 18230 and 18232 are two back-to-back inputField.each loops over the same set.
  • The JSDoc says jQuery UI calls onSelect "without reliably emitting a native change event"; the actual behavior is deterministic (it fires change only when no onSelect is set). Worth stating the observed symptom instead.

@cortexrd

Copy link
Copy Markdown
Owner

TL;DR for the review above (the seven inline comments are one issue seen from several angles, not seven separate tasks; no need to resolve each one).

The problem in one sentence: jQuery UI's datepicker fires the input's native change event only when there is no onSelect. This PR installs an onSelect on every required date field, so calendar picks stop firing change there, and everything in KTL that listens for change on date inputs (persistent form, KTL.fieldValueChanged, the app onFieldValueChanged callback, Knack's own from/to date copy) silently stops working for those fields.

What to do:

  1. Remove wireDatePickerValidation, the ktlReqDatePickerBound flag and the inputField.each(... wireDatePickerValidation ...) line.

  2. Replace them with one wrapper installed once at KTL load (not per field, not per render), which re-triggers change after any onSelect runs:

    const origSelectDate = $.datepicker._selectDate;
    $.datepicker._selectDate = function (id, dateStr) {
        const inst = this._getInst($(id)[0]);
        origSelectDate.apply(this, arguments);
        if (inst && this._get(inst, 'onSelect')) inst.input.trigger('change');
    };

    The existing change.ktl_req handler in fieldIsRequired then revalidates on calendar picks with no new code there. This one change covers the comments at 18440 (both), 18439, 18446 and 18444.

  3. Re-test the original symptom on a stock form. On a plain Knack date field the calendar already fires change today, so what you saw was most likely the sibling time input: _req treats every sub-input of a date_time field as required, and with time-default None that input renders empty and keeps Submit disabled after the date is picked (comment at 18232). If that is the case, the fix is to decide which sub-inputs _req should count for date_time fields (date only, or date + time depending on the field's time format), not the datepicker at all.

Note on style: using jQuery here is fine. Vanilla JS is a preference in this repo, not a rule.

@CSWinnall

CSWinnall commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator Author

TL;DR for the review above (the seven inline comments are one issue seen from several angles, not seven separate tasks; no need to resolve each one).

The problem in one sentence: jQuery UI's datepicker fires the input's native change event only when there is no onSelect. This PR installs an onSelect on every required date field, so calendar picks stop firing change there, and everything in KTL that listens for change on date inputs (persistent form, KTL.fieldValueChanged, the app onFieldValueChanged callback, Knack's own from/to date copy) silently stops working for those fields.

What to do:

  1. Remove wireDatePickerValidation, the ktlReqDatePickerBound flag and the inputField.each(... wireDatePickerValidation ...) line.

  2. Replace them with one wrapper installed once at KTL load (not per field, not per render), which re-triggers change after any onSelect runs:

    const origSelectDate = $.datepicker._selectDate;
    $.datepicker._selectDate = function (id, dateStr) {
        const inst = this._getInst($(id)[0]);
        origSelectDate.apply(this, arguments);
        if (inst && this._get(inst, 'onSelect')) inst.input.trigger('change');
    };

    The existing change.ktl_req handler in fieldIsRequired then revalidates on calendar picks with no new code there. This one change covers the comments at 18440 (both), 18439, 18446 and 18444.

  3. Re-test the original symptom on a stock form. On a plain Knack date field the calendar already fires change today, so what you saw was most likely the sibling time input: _req treats every sub-input of a date_time field as required, and with time-default None that input renders empty and keeps Submit disabled after the date is picked (comment at 18232). If that is the case, the fix is to decide which sub-inputs _req should count for date_time fields (date only, or date + time depending on the field's time format), not the datepicker at all.

Note on style: using jQuery here is fine. Vanilla JS is a preference in this repo, not a rule.

This is just plain wrong as we had a date only field. We have had to implement this in multiple apps as Knack does not trigger a change on selecting a date.

The reproduction is a date-only field, not date_time. On the tested KTL/Knack runtime, selecting a date does not trigger the field’s change handler. With wireDatePickerValidation removed, the suggested _selectDate wrapper has no effect unless an existing onSelect is present—which it is not in this reproduction. Please identify the existing onSelect you believe is suppressing change, or revise the recommendation to preserve the required-field revalidation path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants