Forms: add conditional logic to form fields - #50938
Conversation
enejb
added
Enhancement
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 26 files. Only the first 5 are listed here.
14 files are newly checked for coverage. Only the first 5 are listed here.
Full summary · PHP report · JS report Coverage check overridden by
I don't care about code coverage for this PR
|
There was a problem hiding this comment.
Pull request overview
Adds first-pass conditional logic support to Jetpack Forms fields (editor UI + front-end visibility + server-side enforcement), gated behind a single jetpack_forms_conditional_logic_enable flag to keep behavior consistent while the feature is tested.
Changes:
- Introduces a shared conditional-logic vocabulary + evaluator in TypeScript (editor/front-end) and a mirrored PHP evaluator for validation and storage.
- Updates front-end interactivity and field rendering to hide conditionally-invisible fields, and updates PHP validation/storage to skip/strip hidden fields.
- Adds comprehensive PHP + JS test coverage, including a parity test to prevent TS/PHP drift, plus a changelog entry.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| projects/packages/forms/tests/php/contact-form/Feedback_Conditional_Logic_Test.php | Integration coverage for stripping hidden fields during feedback creation. |
| projects/packages/forms/tests/php/contact-form/Conditional_Logic_Validation_Test.php | Integration coverage for validation behavior (required + hidden regression). |
| projects/packages/forms/tests/php/contact-form/Conditional_Logic_Test.php | Unit tests for PHP evaluator, mirroring JS cases. |
| projects/packages/forms/tests/php/contact-form/Conditional_Logic_Parity_Test.php | Enforces TS/PHP parity (operators, type tables, cascade cap). |
| projects/packages/forms/tests/php/contact-form/Conditional_Logic_Feature_Flag_Test.php | Verifies the feature flag gates editor/runtime/enforcement together. |
| projects/packages/forms/tests/js/blocks/shared/conditional-logic/register.test.js | Tests editor filter registration + field coverage. |
| projects/packages/forms/tests/js/blocks/shared/conditional-logic/panel.test.jsx | Tests panel behavior and rule-building UX. |
| projects/packages/forms/tests/js/blocks/shared/conditional-logic/operator-labels.test.js | Ensures every operator is labelled and labels are non-empty. |
| projects/packages/forms/tests/js/blocks/shared/conditional-logic/field-types.test.js | Verifies block/type/operator/value-input mappings for all field blocks. |
| projects/packages/forms/tests/js/blocks/shared/conditional-logic/field-options.test.js | Tests option extraction across the various field option storage schemes. |
| projects/packages/forms/tests/js/blocks/shared/conditional-logic/evaluate.test.js | Unit tests for JS evaluator + cascade behavior. |
| projects/packages/forms/src/modules/form/view.js | Adds per-field derived state to compute conditional visibility in the interactivity store. |
| projects/packages/forms/src/contact-form/css/grunion.scss | Adds a CSS class to fully hide conditionally-hidden field wrappers. |
| projects/packages/forms/src/contact-form/class-feedback.php | Strips hidden fields from stored feedback data server-side. |
| projects/packages/forms/src/contact-form/class-contact-form.php | Emits conditional logic context; caches resolved visibility; skips validation for hidden fields. |
| projects/packages/forms/src/contact-form/class-contact-form-plugin.php | Serializes the conditionalLogic block attribute into a shortcode-compatible attribute. |
| projects/packages/forms/src/contact-form/class-contact-form-field.php | Decodes conditionallogic and binds conditional hiding to field wrapper via interactivity. |
| projects/packages/forms/src/contact-form/class-conditional-logic.php | New PHP conditional-logic evaluator + cascade resolver. |
| projects/packages/forms/src/class-jetpack-forms.php | Adds the single feature gate (jetpack_forms_conditional_logic_enable). |
| projects/packages/forms/src/blocks/shared/settings/index.js | Adds a shared conditionalLogic attribute default for form field blocks. |
| projects/packages/forms/src/blocks/shared/conditional-logic/util/operator-labels.ts | Editor-only translated operator labels. |
| projects/packages/forms/src/blocks/shared/conditional-logic/util/field-types.ts | Shared operator/type tables and helpers (block + shortcode type mapping). |
| projects/packages/forms/src/blocks/shared/conditional-logic/util/field-options.ts | Normalizes selectable options across choice-style fields. |
| projects/packages/forms/src/blocks/shared/conditional-logic/util/evaluate.ts | New TS evaluator + cascade resolver used in editor/front-end. |
| projects/packages/forms/src/blocks/shared/conditional-logic/register.jsx | Registers the editor.BlockEdit filter to inject the panel into all field blocks. |
| projects/packages/forms/src/blocks/shared/conditional-logic/hooks/use-subject-fields.js | Discovers sibling fields and ensures stable IDs for condition subjects. |
| projects/packages/forms/src/blocks/shared/conditional-logic/editor.scss | Styles for the inspector panel UI. |
| projects/packages/forms/src/blocks/shared/conditional-logic/controls/index.js | Control registry (phase 1: Field Value). |
| projects/packages/forms/src/blocks/shared/conditional-logic/controls/field-value/edit.jsx | Rule builder UI for field-value conditions. |
| projects/packages/forms/src/blocks/shared/conditional-logic/constants.js | Default + normalization helper for the attribute shape. |
| projects/packages/forms/src/blocks/shared/conditional-logic/components/panel.jsx | Inspector panel wrapper and top-level logic management. |
| projects/packages/forms/src/blocks/shared/conditional-logic/components/panel-header.jsx | Copy/paste/reset utilities and clipboard validation. |
| projects/packages/forms/src/blocks/field-text/edit.jsx | Clarifying docblock note that the conditional panel is injected via filter. |
| projects/packages/forms/src/blocks/contact-form/class-contact-form-block.php | Exposes the feature flag to the editor via registered features. |
| projects/packages/forms/src/blocks/contact-form/child-blocks.js | Side-effect import to ensure editor filter registration is loaded. |
| projects/packages/forms/changelog/try-conditional-form-fields | Changelog entry for the new (flagged) conditional-logic feature. |
| // BaseControl still applies a bottom margin here even | ||
| // with __nextHasNoMarginBottom, which pushed the two | ||
| // selectors out of line with each other. | ||
| .components-base-control, | ||
| .components-base-control__field { | ||
| margin-block-end: 0; | ||
| } |
There was a problem hiding this comment.
Are they inheriting the margin from some odd stylesheet or what's going on there? They shouldn't have margin.
There was a problem hiding this comment.
Good question, and the answer is unsatisfying: both selectors already pass __nextHasNoMarginBottom, but BaseControl still emits the bottom margin in this context.
It isn't cosmetic — that margin is what made the two selectors sit visibly out of line with each other, which was reported as a bug during testing. So I've kept the override but added a comment explaining why it exists rather than leaving it looking arbitrary:
// Both selectors pass __nextHasNoMarginBottom, but
// BaseControl still emits a bottom margin here, which
// left the two boxes visibly out of line. Dropping it
// is what makes the row sit flush.If there's a correct prop or component I should be reaching for instead, I'd rather use that than override — happy to swap it out.
| @@ -0,0 +1,348 @@ | |||
| import { Button, Notice, SelectControl, TextControl } from '@wordpress/components'; | |||
There was a problem hiding this comment.
Recommend trying directly with Button and Notice from @wordpress/ui, easier when not needing to migrate later.
There was a problem hiding this comment.
Switched — Button now comes from @wordpress/ui, and the remove control uses IconButton, which looks like the right component for an icon-only action since it takes label for both the tooltip and assistive tech.
That detail mattered: swapping to a plain Button first silently dropped the button's accessible name, and two tests caught it.
Notice I've left on @wordpress/components for now — the rule-level notices rely on status="warning" and isDismissible, and I couldn't confirm the equivalents without the design-system docs to hand. Happy to move it in a follow-up if those map across.
github-actions
Bot
added
the
[Plugin] Jetpack
|
Thanks for the review — all of it is addressed in 335d485. Design system. The panel now uses
One I pushed back on, the Also fixed: CI was red on One caveat worth stating: I didn't have the design-system documentation available while making these changes, so the component props and token names came from the installed |
|
Reviewed the code first, then built the branch onto a fresh JN site with the flag on to check the two things reading can't settle. Test formsTwo forms, both with one rule: show "Please explain" when "Reason" is "Other", and "Please explain" is required.
Every test below picks Reason = "Sales", so the rule does not match and the conditional field is hidden, as intended. Block markup for Form A<!-- wp:jetpack/contact-form -->
<div class="wp-block-jetpack-contact-form">
<!-- wp:jetpack/field-select {"id":"reason","label":"Reason","options":["Sales","Other"],"required":true} /-->
<!-- wp:jetpack/field-text {"id":"explain","label":"Please explain","required":true,"conditionalLogic":{"enabled":true,"action":"show","logicalOperator":"any","controls":{"fieldValue":{"rules":[{"field":"reason","operator":"is","value":"Other"}]}}}} /-->
<!-- wp:jetpack/field-telephone {"id":"phone","label":"Phone"} /-->
<!-- wp:button {"tagName":"button","type":"submit"} -->
<div class="wp-block-button"><button type="submit" class="wp-block-button__link wp-element-button">Send</button></div>
<!-- /wp:button -->
</div>
<!-- /wp:jetpack/contact-form -->From reading
From testing on a live site1. Confirmed, blocker. Form A, Reason = "Sales" — "Please explain" is correctly hidden. Fill Phone, click Send: nothing happens, no request goes out, and the form shows "Please fill out the form correctly. • Please explain: This field is required." The visitor is told to fill a field that is not on the page, and the error links to an anchor they cannot reach. Testing step 8 does not pass. 
2. The PHP half is fine. Same submission, but sent with 3. Confirmed, but wider than expected. Form B with the conditional field hidden: its wrapper keeps a 300px slot in the row, so
4. Divergences confirmed with a WorDBless test driving both real paths on the same submission — three separate mechanisms:
5. Separate image-select bug. The rule builder offers option labels, but the input's value is a JSON blob ( Suggested order
Also, low severity: the parse-time Nice: the parity test pinning the JS↔PHP operator vocabulary, fail-open on circular rules, and one flag gating editor, render and submit. |
|
Thanks — this was a genuinely useful review. All of it is addressed on this branch and mirrored into #50976–#50980, which have been rebuilt on current trunk. BlockingLegacy-path validation. Confirmed: Should fixStorage and validation resolving separately. Storage now calls Hidden values reaching the integrations. Stripped once in Dates. Went with the full fix rather than the fallback: the subject's Consent. Keyed off Also fixedInset-label wrap (the runtime and the first-paint stamp now agree on the element via Image-select — took your second option and dropped Parity test. You were right that it pinned vocabulary rather than behaviour. There's now a shared table of Two things your review turned up indirectlyThe rules never survived a shortcode round-trip. Rewriting That's very likely CGastrell's submit-blocking report, and it wasn't a stale build — |
53a7647 to
77464b8
Compare
Any field can be shown or hidden based on another field's answer. Disabled by default: the whole feature sits behind the `forms-conditional-logic` flag, registered with the jetpack-feature-flags package, while it is in testing. Five pieces, reviewable separately as #50976-#50980: - the shared vocabulary both sides speak -- the comparison behaviour each field type has, the operators it offers, the value input each operator needs -- plus a `conditional_logic` declaration on each field block, beside `form_editor` and outside the registered block settings so it cannot collide with core block metadata - the resolver, written once in JS and once in PHP, because the browser decides what the visitor sees while the server decides what it accepts, and the browser's answer cannot be trusted at submit time. Resolution is a fixed point, since a field's visibility can depend on a field that is itself conditional; a cycle fails open, because hiding a field the visitor cannot reveal is a dead end - the front-end runtime, which shows and hides fields as the visitor answers and skips hidden fields when validating - the server side: hidden fields are skipped by validation, so a required field the visitor cannot see can never block submission; initial visibility is resolved during render so a hidden field never flashes into view; and a hidden field's answer reaches nothing downstream - the editor panel, added by an `editor.BlockEdit` filter rather than per-block wiring, loaded through a lazy boundary so a site with the feature off never fetches it Rebased onto trunk by content rather than by replaying commits: the branch had 36 commits against 103 of drift, and the conflicts landed in intermediate states that later work removed outright, so replaying them would have produced commits that never existed in a working form. The tree here is byte-identical to the verified pre-rebase branch merged with trunk.
The attribute kept its rules in a map keyed by condition kind:
controls: { fieldValue: { rules: [ ... ] } }
A map cannot express "any of these AND all of those", so supporting more than
one grouping later meant reshaping what is already stored. It is an array now:
logicalOperator: 'any', // combines the groups
groups: [
{ logicalOperator: 'all', rules: [ ... ] },
]
Each group reduces its own rules with its own operator, and the groups reduce
with the top-level one. Both evaluators handle several groups already, even
though the V1 panel writes exactly one -- if only the storage changed, the
second group would still arrive needing an evaluator change, and the migration
would only have been deferred. With one group the outer reduction is a no-op,
so behaviour is unchanged.
Rules now carry their own type, so further condition kinds -- query string,
user role, date and time -- become new rule types inside a group rather than
another reshape. That was what the `controls` map was reserving space for, and
per-rule is the version of it that composes with grouping. An evaluator that
meets a type it does not know ignores that rule, so a form saved by a newer
editor degrades to its remaining conditions instead of breaking.
The panel is unchanged to look at: it edits one group, and its Any/All selector
is now that group's operator rather than the top-level one. Removing the last
condition drops the empty group rather than leaving a hollow one behind. The
control registry is gone -- its only job was keying the map that no longer
exists, and the panel renders the group's rules directly.
No migration path: the feature has never shipped enabled, so nothing stored
anywhere uses the old shape. Test forms built on a flagged site during review
will need their conditions re-added.
cc299bd to
97dde97
Compare
The shared behaviour table declared its data provider with only the PHPUnit attribute. The PHP 7.2 and 7.3 matrix runs on a PHPUnit old enough to ignore attributes, so the provider never ran and the test was called with no arguments. Adding the annotation alongside the attribute is what the Jetpack.PHPUnit.Attributes sniff was warning about, and it keeps both PHPUnit generations reading the same table. Reading the fixture now throws when it cannot be parsed rather than falling through. Phan flagged the unguarded array access, and the guard it wants is the one the test needs anyway: a fixture that fails to load would otherwise turn every case into a silent pass, which is the one failure mode a cross-language parity table must not have. Also documents the $format parameter the date fix added to three methods, and fixes two alignment nits and a brace placement.
PHPCS treats the double-arrow misalignment as a warning, and this repo's phpcs run exits non-zero on warnings, so it fails the build.
A rating was declared as a `number`, and every rule against one was silently dead. The field submits `selected/max` -- `4/5`, not `4` -- so is_numeric() and Number() both refuse it, the pair comes back unparseable, and every numeric operator returns false. "Show when the rating is at least 4" hid its field permanently, and the answer was then dropped at storage. Only is_empty and is_not_empty worked, because they never reach the numeric comparison. The same failure as image-select, and it was hiding in the same place: the type table said the comparison was numeric, and nothing checked what the field actually submits. Rating is its own type key now rather than a special case inside `number`, because two things differ, not one. The submitted value needs unpacking before it can be compared -- only the submitted side, since the rule stores a bare number. And the values worth offering are the field's own scale, so the rule builder lists 1..max from the block's `max` attribute instead of a free number box that would happily accept 6 stars out of 5. The operators are the numeric set, which is why sharing the key looked reasonable. Four cases in the shared behaviour table, so both evaluators are pinned to the same answers; they failed in both languages before this.
The rule builder lived in a column about 280px wide. Three controls per condition do not fit across it, so each condition stacked into a bordered card, and three or four of those were taller than the viewport -- on top of the action and match selectors and a hint line. The multi-group storage would have made it worse: a second group means two nested lists in that column. The inspector now keeps a summary and a button, and the rules are edited in a wide dialog. That follows jetpack-integration-controls, which already keeps a summary in the inspector and pushes its list into a Modal. With the width available, a condition is one row -- subject, comparison, value, remove -- reading as a sentence rather than a labelled card. Both selectors sit inside the sentence, so the whole rule reads back as "Show this field when all of these match". A long list becomes aligned columns instead of a stack. The summary is why the inspector keeps a panel rather than a bare button: an author can see whether a field is conditional, and roughly why, without opening anything. It states the action, the match mode and the count. Edits still commit straight to the block attribute. There is no draft state and no Save button, so undo remains the editor's own and the rules have one source of truth, matching every other inspector control. Nothing else moves: the stored shape, both evaluators, all the PHP, the isSelected gate, and the lazy boundary are untouched. The dialog renders inside the already-lazy panel chunk, so a site with the feature off still fetches none of it -- verified against a clean build, where editor.js contains no trace of the dialog. Two test changes worth noting. Most of the rule-editing tests needed only the dialog opened first, since they drive the same components. The two that queried through the render container now query the dialog instead, because a Modal portals to the end of the document rather than nesting in what render() returns.
Three changes to the conditional-logic dialog. The heading goes back to the arrangement it had in the inspector: the action and match selectors side by side, with the clause that finishes the sentence on its own line beneath. Folding both into a single inline sentence read tidily in a mockup but left the two controls competing with the prose between them; side by side they are the sentence, and the line underneath finishes it. Each condition now sits on its own tinted surface. In the inspector a border did that work, but a bordered card in a wide dialog reads as a box around nothing -- the tint separates the rows without drawing another outline. The remove control uses the trash icon rather than a cross. The dialog's own close button is a cross, and two crosses on screen doing different things is one too many.
…o content The controls in a condition row all sit on one line, so the row centres them rather than aligning to the top. The remove button also loses the top offset it carried: that offset existed to line it up against top-aligned controls, and with the row centred it was the thing knocking it out of line. The two heading selectors size to their own labels instead of splitting the row. They are words in a sentence, and stretching them across a wide dialog made a short phrase like "if any" span half the width, which stopped the line reading as a sentence at all -- the problem was only invisible in the inspector because there was no width to spread into. Also rewraps the comments in this file: SCSS here is checked at 80 columns rather than the 100 the JavaScript uses, and the rewrite had been running past it. My commits were using --no-verify, so the pre-commit stylelint never said so. The stacked branches are unaffected.
… with one Four changes to the conditional-logic dialog. The inspector button matches Manage integrations -- the components Button at secondary, not the design-system one. Two buttons opening two dialogs from the same inspector should not look like different kinds of thing. The builder opens with a condition waiting to be filled in, instead of an empty pane and an Add button. That row is not written to the block until a field is chosen, so opening the dialog does not mark the post as changed. A condition that names no subject, or gives no value where its operator needs one, is skipped by both evaluators. That was invisible: the field simply did not react, with nothing on screen saying why. Finished conditions now carry a tick at the head of the row, an unfinished one says it will be ignored, and Add condition waits until the current one says something -- otherwise an author can stack up rules that quietly do nothing. The judgement lives in one place and mirrors what the evaluators skip, so the editor and the runtime cannot drift on what counts as a condition. Only a started condition complains. An untouched row is empty, not wrong, and the builder now opens with one of those.
…on add The Edit conditions button centres in the panel rather than sitting against the left edge, so it reads as the panel's action rather than a stray control. The unfinished-condition message lines up with the control it is about. It was starting at the container edge while the controls start after the status column, which left it hanging under the tick. It now repeats that column as an empty spacer instead of computing an offset, so the two stay aligned if the column's width ever changes. Adding a condition moves focus to its field selector. A new row appears empty and the first thing to do with it is choose a subject; it also tells a screen-reader user the row is there at all. Only the row the button just made takes focus, so opening the dialog does not pull focus out of the editor. The focus test needed a harness holding real state. The existing one passes a jest.fn() as setAttributes, so an added condition never comes back as props and the second row never renders -- the assertion would have been about the mock rather than the component.
A field with conditions now carries a button beside Required: an eye when the rules show it, an eye with a slash when they hide it. Its tooltip is the same sentence the inspector summary uses, and pressing it opens the rule builder. It answers "what is special about this field?" for an author looking at the canvas rather than the sidebar, which is where the panel's summary is no help. Rendered from the same filter as the panel rather than from the blocks. The Required control is rendered in three places -- the shared field controls, and the consent and checkbox blocks, which build their own -- so adding a sibling there would have meant three edits and a fourth whenever another block goes its own way. The filter already covers every field block. Only shown once conditions exist: on a field without any there is no state to report, and a button on every field block in the form is noise. Both conditional-logic suites mock @wordpress/block-editor, so both needed BlockControls adding. One of them fails at import rather than in a test, which reads as an unrelated suite breaking.
…xist Matches how Required marks a field: the is-pressed class, which the toolbar renders as an inverted icon. Two toolbar buttons reporting a field's state should look like the same kind of thing. Note this is always on today, because the button only renders once conditions exist -- there is no un-inverted state to see. Keeping the condition explicit rather than hardcoding the class is what makes an always-visible version a one-line change if the button should sit in the toolbar the way Required does, present whether or not it applies.
…supports it The button was appearing only once rules existed, which made it useless for finding the feature -- an author had to already know it was there. It now sits in the toolbar for any block that supports conditional logic, the way Required does, and inverts once the field carries conditions. That also makes it a way in: pressing it opens the builder whether or not rules exist, so conditional logic is reachable from the canvas rather than only from the sidebar. Its tooltip is "Add conditional logic" while the field has none. It cannot be "Conditional logic": that is the inspector panel's title, so a query for a button of that name matches both, which is worth avoiding in the accessibility tree as well as in the tests.
Four changes to the rule builder. Clear all conditions removes them in one go, sitting opposite Add rather than beside it so the two are not mistaken for each other. It appears only once there is something to clear. Clearing does not immediately offer another empty row. The builder normally opens with one waiting, but doing that straight after a clear looks like the clear failed -- an author who wants a row presses Add. Add condition is now absent rather than disabled while the current condition is unfinished. A disabled button plus a line explaining why is more to read than a button that is simply not there yet, so the explanatory line goes with it. The inspector's Edit conditions button spans the panel instead of centring, so it reads as the panel's action rather than a control floating in the middle.
Adding a condition was blocked while the current one was unfinished, which made it impossible to add a second one mid-thought -- a normal way to work, and the bug reported here. Add condition is always available now. What the gate was protecting against is instead said per row. A complete condition is badged Active; one that is incomplete is badged Inactive and carries the reason: no field chosen, a field that has since been deleted, or a missing value. That is more useful than the gate was, because it names the row at fault rather than blocking the whole builder. The reason is set on the badge as well as in its tooltip. A tooltip renders nothing until hovered, so on its own it leaves the reason unreachable by keyboard and unread by a screen reader. The tick is gone. It told an author nothing they could act on, and now that the badge carries both states there is no need for a second signal.
…o content The badge was flexing like a fourth column, taking room from the three selectors that carry the condition's meaning. It now takes only the width its own text needs, and sits after the remove control at the end of the row. Widths default to content for everything in the row, with the three selectors sharing out what is left. Anything added to the row later will size to its content rather than starting to stretch on its own.
The line after the selectors read "of the following conditions are met:", which finished the sentence but left the starting state unsaid. A field with a show rule is hidden until something reveals it, and one with a hide rule is visible until something hides it -- an author had to infer that. It now states the default, and follows the action: "This field is hidden by default, until the following conditions are met:", or visible for a hide rule.
The icons were the wrong way round. A show rule means the field is hidden until something reveals it, so the toolbar showed an open eye for a field that starts hidden. The icon now reports the state an author actually sees on the canvas before any condition is met: hidden for a show rule, visible for a hide rule. A field with no conditions reads as visible, which the plain action check would have got wrong -- the action defaults to `show`, so an untouched field would have claimed to be hidden. That question is now named rather than decided inline. The toolbar icon and the builder's opening line both report the same thing, so having one predicate for it is what stops them disagreeing. Tested against the predicate rather than the rendered icon. Reading an SVG path out of the DOM meant manual cleanup and direct node access, both of which the testing-library rules reject, and it pinned the shape of an icon rather than the meaning behind it.
Badges at the end of the row said Active or Inactive in words. An icon at the head says the same thing in less space, and puts the state where it can be read straight down the left edge of a long list rather than hunted for at the end of each line. A green check means the condition will be acted on; an amber caution means it will be skipped. The tooltip carries the reason, phrased as the thing to do about it: choose a field, give it a value, or replace one whose field has since been deleted. The reason is set on the icon as well as in its tooltip. A tooltip renders nothing until hovered, so on its own it leaves the only explanation of why a rule will not fire unreachable by keyboard and unread by a screen reader.
panel.test.jsx mocks useSubjectFields and useEnsureFieldId so it can drive the rule builder without a block editor. That left what they actually implement untested: which fields are offered as subjects, and what id a chosen one is given. The id assignment is the part worth pinning -- getting it wrong silently repoints a rule at another field, or renames a field that may already have responses stored against its old id. Covers minting an id from a label, keeping an explicit one, de-duplicating against ids already in the form, the empty-slug fallback, and a missing field. For useSubjectFields: sibling fields listed, the panel's own excluded, id-less fields kept, step numbering, the label fallbacks, and a field outside any form. The panel suite gains the one assertion it could not make with a fixture that had no id of its own -- that the panel's own field id reaches the uniqueness check. Without it an unnamed sibling whose label slugifies the same way is handed the owner's id. Both guards verified by breaking them: dropping the used-id list fails the de-duplication tests, and dropping ownFieldId fails the panel one. Equivalent ground to kraftbj's tests on #50980, written against the UI as it now stands rather than cherry-picked -- the panel those assertions drove has since moved into a dialog.
The status icons were reading as near-black. The unsuffixed success and warning foreground tokens are text-on-light colours -- rgb(0,41,0) and rgb(46,25,0), dark enough that neither icon looked like its status. The `-weak` variants are the ones with the hue: green for a condition that will be acted on, amber for one that will be skipped. Caught by looking at a real screenshot; both spellings compile and pass stylelint, so nothing else would have flagged it.
Code reviewFound 1 issue:
jetpack/projects/packages/forms/src/contact-form/class-conditional-logic.php Lines 385 to 388 in 5898fc1 The JS mirror has the same behaviour, so the two evaluators agree with each other and only disagree with the UI: Either make the evaluators genuinely skip a rule whose operator needs a value it was not given, or change the editor copy so it stops promising something the runtime does not do. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
The editor said an unfinished condition would be ignored. Both evaluators substituted an empty string for the missing value and evaluated it for real, so the amber icon and its "Give this condition a value." tooltip were promising something that did not happen. Evaluating such a rule is worse than useless. `does_not_contain ''` is true of every value, so a half-written rule quietly forced its field visible; `is ''` matched whenever the subject happened to be blank, firing by accident. Either way a condition the editor marked inert was deciding what the visitor saw. An operator that compares against something, given nothing to compare against, now returns null in both languages -- the same "ignore this rule" the evaluators already use for a subject field that no longer exists. isRuleComplete loses its exception for subjects that render no value input. An operator needing a value it cannot be given is exactly as inert as one the author simply has not filled in, and both evaluators now skip both; keeping the exception would have put the icon back at odds with what happens. Zero is a value, not a missing one. Both sides compare the stringified form, so a rule against 0 is unaffected -- covered in the shared table, along with the two operators above. Also reattaches two docblocks that earlier commits on this branch separated from their functions by inserting a new method in between, one of which phpcs was failing on.
|
Fixed in bf8fd6d. Went with making the evaluators honour what the editor says, rather than softening the copy — a rule the author hasn't finished shouldn't be deciding what a visitor sees. An operator that compares against something, given nothing to compare against, now returns
Four rows added to the shared behaviour table, so both evaluators are pinned to the same answers:
Verified by removing the guard again and watching two of those rows fail. |
The production build failed: "msgid argument is not a string literal: __(x?'Edit conditions':'Add conditions','jetpack-forms')". Two identically shaped __() calls in a ternary get folded by the minifier into one call whose msgid is an expression, and an expression cannot be extracted for translation. Restructuring does not help -- an if/else, and separate object properties, both get folded back into the same call. The package already solves this by giving one branch a third argument, which __() ignores at runtime but the minifier cannot merge across. Same fix here, in the two places this branch introduced, with a comment so the stray argument does not read as a typo. Also realigns the double arrows in the conditional-logic test fixtures. This repo's phpcs run exits non-zero on warnings, and the scripted edit that moved those tests onto the groups shape left them misaligned.
kraftbj
added
the
I don't care about code coverage for this PR
See FORMS-744
Proposed changes
Adds conditional logic to Jetpack form fields: any field can be shown or hidden based on
another field's answer.
Disabled by default. The whole feature sits behind the
forms-conditional-logicfeatureflag, registered with the
automattic/jetpack-feature-flagspackage, while we test it. Note this is that package's first consumer.
Rules are edited in a dialog rather than the inspector. Three controls per condition do not fit
a ~280px column without stacking into a card each, and a handful of those outgrows the viewport.
A condition that names no field, or gives no value where its operator needs one, is skipped by
both evaluators. That used to be invisible — the field simply did not react. The icon at the
head of each row now says which conditions will be acted on, and the tooltip on an amber one
says what to do about it.
The inspector keeps a summary and a button, so a field's behaviour is readable without opening
anything. The block toolbar carries the same state next to Required: an eye, crossed out
when the field starts hidden.
editor.BlockEditfilteradds the panel to each
jetpack/field-*block that declares a comparison behaviour — 18 ofthe 19. A filter rather than per-block wiring because the field blocks share no single
inspector component (four build their own), and new field types inherit it automatically.
Each block declares its own
conditional_logic.typebesideform_editor, so support can beturned on one block at a time.
their own options, numeric fields get
>/</≥/≤, date and time get before/after,checkboxes get is/is-not-checked, and a rating offers its own scale. Entries are labelled
with their block type —
Name (Name field)— so fields are distinguishable without a label.rules, resolved to a fixed point. On a cycle the field is left visible: a stray value in a
response is recoverable, a silently discarded answer is not.
validation, storage, the notification email and the integrations from one map, so they cannot
disagree about whether a field was shown.
Storage
An array of groups, not a map keyed by condition kind. A map cannot express "any of these AND
all of those", so supporting more than one grouping later would have meant reshaping what is
already stored. Both evaluators handle several groups today even though the UI writes exactly
one — otherwise the second group would still arrive needing an evaluator change.
Rules carry their own
type, so further condition kinds (query string, user role, date andtime) become new rule types inside a group rather than another reshape. A rule of an unknown
type is ignored, so a form saved by a newer editor degrades to its remaining conditions.
Those extra condition kinds are deliberately out of scope: they are decided once at render, but
a submission does not carry the request context that produced it, so PHP cannot re-derive them
at submit time without a signed render-time payload. That needs its own design.
Related product discussion/links
Does this pull request change what data or activity we track or use?
No new tracking. It does change what is stored in a form response: fields hidden by conditional
logic are excluded from the stored response, the notification email, and the payload passed to
integrations — which is the point of the feature. Nothing is recorded that was not submitted.
Testing instructions
Conditional logic is off by default. Enable the flag with a mu-plugin:
Editor
checkbox and a radio field.
the block toolbar beside Required — both on every field type, including radio and
checkbox-multiple. (Image-select is deliberately excluded; see below.)
is greater than/is at least;confirm it re-offers 1–3;
is checkedand shows no value box.and the field should behave as if the rule were not there.
half-written rule must not block adding another.
editor (Jetpack → Forms), which load different bundles.
Front end
Choose "Other", fill B, then change A away. Both B and C should hide — C must not survive on
B's leftover value.
Submission should go through. Make it visible and submit empty — it should now block.
dd/mm/yyand build a rule on it. The browser and the storedresponse must agree. Worth doing with your machine's timezone away from UTC.
must disappear, and its answer must not be stored.
Flag off
validates normally, and nothing is stripped from responses. With DevTools → Network filtered
to
conditional-logic, nothing should be fetched: the rule builder lives in a lazily-loadedchunk that is never requested when the flag is off.
Notes for reviewers
Behaviour worth a second opinion
Most fields have no
id: the renderer derives one from the label at output time, and a rulepointing at a derived id would stop matching the moment someone edited that label. On a form
that already has responses, this changes that field's response key going forward.
choice, not the label the rule builder offers, so a rule against it could never match.
Comparing the decoded label in all three evaluators is the real fix, but that is value-shape
handling in exactly the place the two evaluators already drift.
too much is recoverable in a way that one showing too little is not.
Fixed on this branch
field they could not see.
conditional field, an invalid email or an out-of-allow-list choice were accepted and stored.
[and a bare
<broke the value out of the attribute, dropping the condition while leaving thefield required.
prefilled field the visitor cleared could be validated as visible and then have its answer
dropped as hidden.
grunion_after_feedback_post_inserted— the last of which MailPoet reads for consent.Date.parse()reads a bareYYYY-MM-DDas UTC and
mm/dd/yyas local, whilestrtotime()reads both as site-local. Add/mm/yyfield parsed on neither side. Both now parse explicitly using the field's own format.
selected/max, e.g.4/5, not a number.in, and had their answers discarded server-side.
circular and failed open.
outer wrapper.
should have been revealed.
Implementation notes
field-single-choice/isjetpack/field-radio,field-multiple-choice/isjetpack/field-checkbox-multiple. A hand-written type table got both wrong, soblock-names.test.jsderives names from source rather than restating them.dist/blocks/editor.jsanddist/form-editor/jetpack-form-editor.js,and the Forms editor loads both. Registration is guarded with
hasFilter, sinceaddFilterdoes not de-duplicate by namespace.
subject list, and mounting it on every field made that walk run per field on every store
change.
other pins comparison behaviour from a table both suites read — which is where the two
evaluators actually drift. The date bug above passed the vocabulary test.
automattic/jetpack-feature-flags. Adding the dependencycascaded into
projects/plugins/jetpack/composer.lock, committed here since CI runs--frozen-lockfile.