Forms: conditional logic (5/5) — the editor panel - #50980
Conversation
|
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! |
c0f0b60 to
af5ffb3
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.
af5ffb3 to
d162c28
Compare
The authoring UI: pick an action, pick whether any or all conditions must hold, and build rules against any other field in the form. Added by an `editor.BlockEdit` filter rather than per-block wiring. The field blocks share no single inspector component -- four of them build their own -- so a filter is the only way to cover all of them without touching nineteen edit files, and new field types inherit the panel automatically. It mounts only for the selected block: the panel walks the whole form tree to build its subject list, and mounting it on every field made that walk run per field on every block-editor store change. The panel loads through a lazy boundary, so a site with the feature off never fetches it: the filter is not registered, the component never renders, and the browser never requests the chunk. That keeps roughly 700KB out of the editor bundle that loads on every site. Any field in the form can be a rule's subject, with operators and a value input chosen from that field's declared comparison behaviour. Fields without an explicit id get one assigned when first referenced, checked against the ids already in use including the panel's own field -- otherwise two same-labelled fields could end up sharing an id and the rule would evaluate the wrong one.
d162c28 to
012ee9f
Compare
…itional-logic-editor
…itional-logic-editor
kraftbj
left a comment
kraftbj
left a comment
There was a problem hiding this comment.
Reviewed the panel layer on its own. The round-1 id-collision fix is genuinely reused rather than reimplemented — ownFieldId is folded into usedIds and it goes through the same generateUniqueFormFieldId helper the Name/ID control uses. The hasFilter guard is correct across the two bundles because @wordpress/hooks is externalized to the shared wp.hooks global, and the isSelected gate keeps the block-tree walk to one panel instance. Deleted subjects surface a Notice, label edits don't break rules since they reference id, and i18n and the icon-only control's accessible name are handled.
Not approving yet, on the rating issue — but the fix belongs in 50976 where the declaration lives, so I've filed it there rather than duplicating it here. This PR is what makes it reachable: block-types.js builds the subject map from every block's conditional_logic.type, so Rating shows up in the dropdown looking like any other numeric field, the author sets "equals 4", and it never fires.
Two things inline.
| * @param {object} field - Subject field descriptor. | ||
| * @return {string} A value unique within the dropdown. | ||
| */ | ||
| const selectionValue = field => field.id || `clientId:${ field.clientId }`; |
There was a problem hiding this comment.
Duplicating a field that already has an explicit id gives two dropdown entries with the same value, and both lookups resolve to the first one.
WordPress's Duplicate and paste copy attributes verbatim, id included, and nothing here re-derives a fresh one. walk() lists both blocks, both produce the same selectionValue, and the two fields.find(...) lookups — the subject resolution in handleFieldChange and the one in RuleRow — return the first match regardless of which option the author clicked. So the author can't point a rule at the second copy; the UI silently substitutes the first.
Transient (it resolves once either id is edited, or once PHP's dedupe renames one at render), and not a crash, so I'd take it as a follow-up rather than a blocker. Keying options by clientId and disambiguating the label when two fields share an id would settle it.
While you're in here: choosing an id-less field as a subject writes a permanent id onto a field the author didn't otherwise touch, changing it from render-derived (g{form_id}-{label}) to the JS slug. The comment a few lines down reasons through the risk, but nothing tells the author it happened. A one-line notice near the Field dropdown would cover it, since this panel's whole purpose is being pointed at existing fields on existing forms.
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.
…itional-logic-editor
The attribute kept its rules in a map keyed by condition kind, which 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 of groups now, each combining its own rules with its own operator, combined with each other by 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. With a single group the outer reduction is a no-op, so behaviour is unchanged. Rules now carry their own type, so further condition kinds become new rule types inside a group rather than another reshape. A rule of an unknown kind is ignored, so a form saved by a newer editor degrades to its remaining conditions.
…itional-logic-editor
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, and every numeric operator returns false. "Show when the rating is at least 4" hid its field permanently. Only is_empty and is_not_empty worked, because they never reach the numeric comparison. Rating is its own type key rather than a special case inside `number`, because two things differ. 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 own scale, so the rule builder lists 1..max from the block max attribute instead of a free number box that would accept 6 stars out of 5. The operators are the numeric set, which is why sharing the key looked reasonable.
The id a rule points at is the load-bearing part of the feature: a wrong one silently retargets a rule at another field, or renames a field that may already have responses. That path ran in no test. panel.test.jsx and with-conditional-logic.test.jsx both mocked use-subject-fields.js wholesale, including a hand-rolled copy of the collision logic, and the one assertion meant to catch the regression used arrayContaining against an always-undefined ownFieldId, so deleting the ownFieldId wiring left the suite green. Add use-subject-fields.test.jsx exercising the real useEnsureFieldId against the real generateUniqueFormFieldId: minting a slug id, keeping an explicit id, de-duplicating, and the round-1 owner collision where a sibling's label matches the panel's own field id. It also covers the real useSubjectFields walk (owner exclusion, id-less fields, multi-step numbering). Strengthen panel.test.jsx to assert the exact usedIds array including ownFieldId, so removing ownFieldId from the panel now fails the suite instead of passing.
kraftbj
left a comment
kraftbj
left a comment
There was a problem hiding this comment.
Approving. The rating-field blocker from my last review is fixed, and the id-collision guard now has real coverage.
6597845 adds use-subject-fields.test.jsx, which exercises the actual useEnsureFieldId / generateUniqueFormFieldId path (mint, keep-existing, de-dupe, and the round-1 owner collision) plus the real useSubjectFields walk, and strengthens panel.test.jsx so the uniqueness check asserts the exact usedIds array including ownFieldId. Verified the test does its job: dropping ownFieldId={ attributes.id } from the panel turns the suite red.
Two items I'd treat as potential follow-ups rather than blockers:
- No notice when choosing an id-less field mints a permanent id. The minted bare slug differs from the renderer's derived
g{form_id}-{label}key, so this can change a field's submission key on a form that may already have responses — worth a one-line notice near the Field dropdown. - Duplicating a field (WordPress copies the id verbatim) yields two dropdown entries with the same value, and both lookups resolve to the first. Keying options by clientId would settle it.
Neither blocks this PR.
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.
2 participants
Fixes FORMS-749
Proposed changes
Part 5 of 5 splitting #50938 — the authoring UI. Pick an action, pick whether any or all conditions must hold, and build rules against any other field in the form.
editor.BlockEditfilter rather than per-block wiring. The field blocks share no single inspector component — four of them build their own — so a filter is the only way to cover all of them without touching nineteen edit files, and new field types inherit the panel automatically.Bundle impact
Measured on a clean build of each bundle:
dist/blocks/editor.jsdist/form-editor/jetpack-form-editor.jsMost of that is
@wordpress/icons, which sat in the entry only because the panel reached it statically. Total emitted JS is unchanged — this is about what loads, not what ships. No enqueue change is needed: theeditorentrypoint still lists exactlyeditor.jsplus its stylesheets, and every new chunk isinitial=false, so the webpack runtime fetches them itself.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Enable the flag in a mu-plugin:
With the flag off, open the editor with DevTools → Network filtered to
conditional-logic: it must stay empty, and no panel should appear on any field.