Forms: conditional logic (3/5) — apply in the browser - #50978
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! |
5a56544 to
cf7083a
Compare
cf7083a to
e422f9d
Compare
Wire the resolver into the front-end form so fields appear and disappear as the visitor answers, and skip hidden fields when validating. Visibility is memoised per form rather than globally: two forms on one page have their own rules and answers, and a single shared cache let one form's result satisfy the other's lookup. Hidden fields use `display: none` as the base state, so a visitor with JavaScript or animations off still gets the right form. The reveal animation is additive, scoped to conditional fields, and inside `@media not (prefers-reduced-motion)`. Checkbox normalisation keys off the control's actual type rather than the field's declared one, so a consent field -- which renders as a checkbox but declares 'consent' -- no longer reads as checked after the visitor unchecks it. The context is also reconciled from the DOM on a bfcache restore, since neither input nor change fires when the browser puts the visitor's values back, and focus moves to the nearest visible control when the field being filled in is hidden by a cascade.
e422f9d to
36afd07
Compare
…nditional-logic-frontend
kraftbj
left a comment
kraftbj
left a comment
There was a problem hiding this comment.
Reviewed the browser layer on its own. The pure helper is in good shape — reading every field value before the memo check keeps the signal subscriptions honest, the per-form key genuinely closes the cross-form leak (with a Jest case pinning it), and the three validation call sites all filter through the same helper so the "is this hidden" decision can't drift between them. display: none is the right hiding primitive: it takes the field out of both the tab order and the a11y tree, and novalidate on the form means a leftover required on a hidden control can't produce the "invalid form control is not focusable" dead end. Reduced motion disables the transition rather than shortening it, and nothing depends on transitionend firing.
Two things inline. Note that none of this code is reachable on this branch — the attributes and context that drive it arrive in 50979 and 50980 — so anything wanting manual verification has to be checked from the server branch or later.
| */ | ||
| if ( typeof window !== 'undefined' ) { | ||
| window.addEventListener( 'pageshow', event => { | ||
| if ( ! event.persisted ) { |
There was a problem hiding this comment.
This guard is inverted with respect to the problem the docblock describes, and I think it means the handler can never fire usefully.
persisted === true means the browser reused the whole document including the JS heap. The interactivity context is the same object graph it was at freeze time, and every DOM value is the one the last handler already wrote into it — there's nothing to repair. The case the comment describes, where the browser "puts the visitor's values straight back into the DOM" while the context holds stale values, is session-history form restoration on a fresh document: Back without bfcache, or a reload. That fires pageshow with persisted === false and returns here on line 44.
The skipped case, concretely: visitor answers a select "Are you a member? → Yes", field B appears, they navigate away and press Back without bfcache. The document is re-parsed, fields comes back empty and re-registers from the server-rendered fieldValue, then the browser restores the select to "Yes" in the DOM. Text fields self-correct because they carry data-wp-bind--value='state.getFieldValue', but select, radio and checkbox have no value binding — so the DOM says "Yes" and the context says "". The client resolves B hidden, new FormData( form ) posts member=Yes anyway, and the server resolves B visible and required and rejects the submission for a field the visitor never saw.
Second half, on line 51: even when it does run, input reaches almost nothing. Counting the wiring in class-contact-form-field.php on this branch — radio (1516, 1555), checkbox (1634), explicit consent (1662), checkbox-multiple (2137, 2168), select (2209), image-select (2581) and rating (3199) are all data-wp-on--change exclusively. data-wp-on--input is only on the free-text controls. So a synthetic input is a no-op for every choice-style field, which is the set most likely to be a conditional trigger.
Dropping the persisted guard and dispatching both event types covers it: [ 'input', 'change' ].forEach( type => element.dispatchEvent( new Event( type, { bubbles: true } ) ) ). Worth scoping the selector to forms that actually carry conditional logic so flag-off sites pay nothing.
Neither half is currently covered by a test, which is probably why both survived — a jsdom case dispatching pageshow with persisted: false against a restored select would catch it.
| const candidates = form | ||
| ? Array.from( form.querySelectorAll( 'input, select, textarea, button' ) ) | ||
| : []; | ||
| const next = candidates.find( |
There was a problem hiding this comment.
This does the opposite of what the comment two lines up says. querySelectorAll returns document order and .find() takes the first match, with nothing referencing ref's position — so if the hidden field is the fifth of eight, focus goes to field one. A visitor keyboard-navigating a long form gets sent back to a field they filled in eight steps ago, and tabbing forward walks them through everything they already completed.
Three smaller things in the same block:
- The fallback on line 898 is inert.
<form>has notabindex, soform.focus()is a successful call that does nothing and focus drops to<body>— the exact outcome this callback exists to prevent.?.focus?.()hides that rather than surfacing it. offsetParent !== nullis still true for a sibling field mid-exit, because thedisplaytransition added in this PR runs 120ms withallow-discrete. A second field hiding in the same cascade can be picked as the target and then vanish under the visitor.- Disabled controls aren't excluded, and
.focus()on one is a no-op with the same end state.
Walking forward from ref rather than over the whole form would settle the main issue — something like slicing candidates from findIndex( el => ref.contains( el ) ) + 1 before the .find(), falling back to the last preceding candidate. Plus :not([disabled]), excluding anything already carrying the hidden class, and giving the fallback something real like the submit button.
Checked the mount behavior while I was here and it's fine: data-wp-watch is a useSignalEffect so it does fire on mount, but activeElement is <body> on load and the ref.contains guard returns early, so it won't move focus on page load. The one hole is loading with a #field-id fragment pointing inside a field that starts hidden.
…nditional-logic-frontend
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.
…nditional-logic-frontend
2 participants
Fixes FORMS-747
Proposed changes
Part 3 of 5 splitting #50938. Wires the resolver into the front-end form so fields appear and disappear as the visitor answers, and skips hidden fields when validating.
display: noneas the base state, so a visitor with JavaScript or animations off still gets a correct form. The reveal animation is purely additive, scoped to conditional fields, and inside@media not (prefers-reduced-motion).Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
The server does not stamp conditional markup until part 4, so the easiest way to exercise this alone is the unit tests:
jetpack test js packages/forms—conditional-visibility.test.jscovers the cascade, the per-form isolation, and the hidden-field validation skip.For a live check, enable the flag and use a form whose fields already carry
data-jp-conditional(or wait for part 4, which emits it):