Skip to content

fix: exact field-key match in trigger_display_if to prevent false positives - #898

Open
thisismyurl wants to merge 1 commit into
alleyinteractive:mainfrom
thisismyurl:fix/display-if-exact-field-name-match
Open

fix: exact field-key match in trigger_display_if to prevent false positives#898
thisismyurl wants to merge 1 commit into
alleyinteractive:mainfrom
thisismyurl:fix/display-if-exact-field-name-match

Conversation

@thisismyurl

Copy link
Copy Markdown

What

trigger_display_if used name.match( displaySrc ) to decide whether a
changed input should trigger visibility changes on display_if siblings.
String.prototype.match() treats its argument as a regex and matches
substrings, so any trigger input whose full HTML name attribute contains
displaySrc as a substring incorrectly fires the display logic.

Reproducer (from #873)

$custom_meta_group = new Fieldmanager_Group([
    'name'           => 'custom_meta_group',  // contains "meta" as a substring
    'serialize_data' => false,
    'children'       => [
        'meta'         => new Fieldmanager_Checkbox(['checked_value' => 'meta']),
        'text_field'   => new Fieldmanager_Textfield(['display_if' => ['src' => 'meta', 'value' => 'meta']]),
        'subject_prefix' => new Fieldmanager_Select([...]),
    ],
]);

When subject_prefix changes, its input name is
custom_meta_group[subject_prefix]. The old code evaluates:

'custom_meta_group[subject_prefix]'.match('meta') != null  // → true!

…because "meta" is a substring of "custom_meta_group". This causes
text_field to be shown or hidden whenever subject_prefix changes,
even though text_field should only react to meta.

Fix

Replace the partial regex match with an exact bracket-key check:

// Before
if ( name && name.match( $( this ).data( 'display-src' ) ) != null ) {

// After
var displaySrc = $( this ).data( 'display-src' );
if ( name && ( name === displaySrc || name.indexOf( '[' + displaySrc + ']' ) !== -1 ) ) {
  • name === displaySrc handles top-level Fieldmanager fields whose
    input name is the bare field key (no bracket notation).
  • name.indexOf( '[' + displaySrc + ']' ) !== -1 handles nested fields
    where the key is wrapped in brackets (e.g. group[meta]). The
    surrounding brackets ensure [meta] cannot match [meta_info] or
    [custom_meta_group].

Testing

  1. Set up the group from the reproducer above.
  2. Change subject_prefix — confirm text_field visibility does not change.
  3. Check the meta checkbox — confirm text_field shows/hides correctly.
  4. Run the existing JS test suite: npm test (or equivalent).

Fixes #873


Development and testing assisted by AI. All code reviewed and verified manually.

…itives

The previous name.match(displaySrc) call treated displaySrc as a regex
and did a substring search against the trigger input's full HTML name
attribute (e.g. custom_meta_group[meta]). This caused display_if to fire
for unrelated triggers whenever displaySrc appeared as a substring of the
group name or another field name.

Replace the partial match with an exact-key check:
- name === displaySrc  (top-level fields with no bracket wrapping)
- name.indexOf('[' + displaySrc + ']') !== -1  (nested fields)

This ensures only the field whose key exactly matches display-src can
trigger visibility changes.

Fixes alleyinteractive#873
Copilot AI review requested due to automatic review settings June 9, 2026 13:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates the “display-if” field matching logic in the FieldManager initializer to use explicit string comparisons rather than regex matching.

Changes:

  • Replaces name.match(...) regex-based matching with exact-match and bracketed-substring matching.
  • Caches display-src in a local variable to avoid repeated jQuery data lookups.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread js/fieldmanager.js
Comment on lines +299 to +300
var displaySrc = $( this ).data( 'display-src' );
if ( name && ( name === displaySrc || name.indexOf( '[' + displaySrc + ']' ) !== -1 ) ) {
Comment thread js/fieldmanager.js
Comment on lines +299 to +300
var displaySrc = $( this ).data( 'display-src' );
if ( name && ( name === displaySrc || name.indexOf( '[' + displaySrc + ']' ) !== -1 ) ) {
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.

display_if incorrectly hide siblings when matching their display-src values

2 participants