Forms: Add 'Other' option support to radio fields #46461
Conversation
enejb
added
[Type] Enhancement
github-actions
bot
added
[Block] Contact Form
|
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. |
|
Some UI questions.
Todo/check.
|
| Are you an Automattician? The PR will need to be tested on WordPress.com. This comment will be updated with testing instructions as soon the build is complete. |
There was a problem hiding this comment.
Pull request overview
This PR adds "Other" option support to radio fields in Jetpack Forms, enabling users to provide custom text when selecting an "Other" option. The implementation spans frontend interactivity, backend processing, validation, ARIA accessibility, and comprehensive test coverage.
Key Changes:
- Adds editor UI controls for enabling/configuring "Other" options on radio fields
- Implements frontend handling with automatic focus management and input clearing
- Adds backend validation to ensure custom text is provided when "Other" is selected on required fields
- Stores metadata (is_other_option, other_label, other_user_value) for flexible rendering
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
Feedback_Fields_Test.php |
Adds comprehensive test for "Other" option form submission and storage |
view.js |
Implements frontend state management and event handlers for "Other" option interaction |
grunion.scss |
Adds styling for "Other" text input with visibility control and disabled states |
class-feedback.php |
Adds processing logic to detect and extract "Other" option metadata from submissions |
class-contact-form-plugin.php |
Preserves isOther attribute from option blocks for server-side rendering |
class-contact-form-field.php |
Implements validation and HTML rendering for "Other" text input with ARIA attributes |
settings/index.js |
Adds allowOther and otherLabel block attributes |
option/index.js |
Adds isOther attribute to option blocks |
option/edit.js |
Adds toolbar button to mark options as "Other" with visual styling |
field-single-choice/edit.js |
Adds toggle control to enable/disable "Other" option with automatic option block management |
editor.scss |
Adds italic styling for "Other" options in the editor |
changelog |
Documents the new feature addition |
| $field .= "<input id='" . esc_attr( $radio_id ) . "' type='radio' name='" . esc_attr( $id ) . "' value='" . esc_attr( $radio_value ) . "' data-wp-on--change='actions.onOtherRadioChange' data-other-label='" . esc_attr( $option_label ) . "' " . $class . checked( $option_label, $value, false ) . ' ' . ( $required ? "required aria-required=\'true\'" : '' ) . '/> '; | ||
| $has_option_is_other_id = esc_attr( $radio_id ); | ||
| } else { | ||
| $field .= "<input id='" . esc_attr( $radio_id ) . "' type='radio' name='" . esc_attr( $id ) . "' value='" . esc_attr( $radio_value ) . "' data-wp-on--change='actions.onFieldChange' " . $class . checked( $option_label, $value, false ) . ' ' . ( $required ? "required aria-required=\'true\'" : '' ) . '/> '; |
There was a problem hiding this comment.
The lines 1362 and 1365 are extremely long (over 200 characters each), making them difficult to read and maintain. Consider breaking these HTML concatenations into multiple lines for better readability.
| $field .= "<input id='" . esc_attr( $radio_id ) . "' type='radio' name='" . esc_attr( $id ) . "' value='" . esc_attr( $radio_value ) . "' data-wp-on--change='actions.onOtherRadioChange' data-other-label='" . esc_attr( $option_label ) . "' " . $class . checked( $option_label, $value, false ) . ' ' . ( $required ? "required aria-required=\'true\'" : '' ) . '/> '; | |
| $has_option_is_other_id = esc_attr( $radio_id ); | |
| } else { | |
| $field .= "<input id='" . esc_attr( $radio_id ) . "' type='radio' name='" . esc_attr( $id ) . "' value='" . esc_attr( $radio_value ) . "' data-wp-on--change='actions.onFieldChange' " . $class . checked( $option_label, $value, false ) . ' ' . ( $required ? "required aria-required=\'true\'" : '' ) . '/> '; | |
| $field .= "<input id='" . esc_attr( $radio_id ) . "' " | |
| . "type='radio' name='" . esc_attr( $id ) . "' " | |
| . "value='" . esc_attr( $radio_value ) . "' " | |
| . "data-wp-on--change='actions.onOtherRadioChange' " | |
| . "data-other-label='" . esc_attr( $option_label ) . "' " | |
| . $class | |
| . checked( $option_label, $value, false ) | |
| . ' ' | |
| . ( $required ? "required aria-required=\'true\'" : '' ) | |
| . '/> '; | |
| $has_option_is_other_id = esc_attr( $radio_id ); | |
| } else { | |
| $field .= "<input id='" . esc_attr( $radio_id ) . "' " | |
| . "type='radio' name='" . esc_attr( $id ) . "' " | |
| . "value='" . esc_attr( $radio_value ) . "' " | |
| . "data-wp-on--change='actions.onFieldChange' " | |
| . $class | |
| . checked( $option_label, $value, false ) | |
| . ' ' | |
| . ( $required ? "required aria-required=\'true\'" : '' ) | |
| . '/> '; |
| id='" . $other_text_id . "' | ||
| name='" . esc_attr( $id ) . "-other-text' | ||
| type='text' | ||
| class='grunion-field' |
There was a problem hiding this comment.
The class 'grunion-field' is applied to the "Other" text input but should likely include 'jetpack-other-text-input' as well for consistency with the wrapper's class name and the CSS rules defined for this element.
| class='grunion-field' | |
| class='grunion-field jetpack-other-text-input' |
| <BlockControls> | ||
| <ToolbarGroup> | ||
| <ToolbarButton | ||
| onClick={ () => setAttributes( { isOther: ! isOther } ) } | ||
| className={ isOther ? 'is-pressed' : undefined } | ||
| > | ||
| { __( 'Other', 'jetpack-forms' ) } | ||
| </ToolbarButton> | ||
| </ToolbarGroup> | ||
| </BlockControls> |
There was a problem hiding this comment.
The "Other" toolbar button is shown for all field types (checkbox, radio) but the functionality is only implemented for radio fields. This could confuse users who see the button on non-radio field options where it won't work. Consider conditionally displaying the toolbar button only for radio field types.
| <BlockControls> | |
| <ToolbarGroup> | |
| <ToolbarButton | |
| onClick={ () => setAttributes( { isOther: ! isOther } ) } | |
| className={ isOther ? 'is-pressed' : undefined } | |
| > | |
| { __( 'Other', 'jetpack-forms' ) } | |
| </ToolbarButton> | |
| </ToolbarGroup> | |
| </BlockControls> | |
| { type === 'radio' && ( | |
| <BlockControls> | |
| <ToolbarGroup> | |
| <ToolbarButton | |
| onClick={ () => setAttributes( { isOther: ! isOther } ) } | |
| className={ isOther ? 'is-pressed' : undefined } | |
| > | |
| { __( 'Other', 'jetpack-forms' ) } | |
| </ToolbarButton> | |
| </ToolbarGroup> | |
| </BlockControls> | |
| ) } |
| public function test_radio_field_with_other_option() { | ||
| $form_id = Utility::get_form_id(); | ||
|
|
||
| // Create form submission with separate radio value and text input value | ||
| // This simulates the actual POST data from the form | ||
| $_post_data = Utility::get_post_request( | ||
| array( | ||
| 'favoritecolor' => 'Other', // Radio button value | ||
| 'favoritecolor-other-text' => 'Purple with green stripes', // Text input value | ||
| ), | ||
| 'g' . $form_id | ||
| ); | ||
|
|
||
| // Create options data with an "Other" option | ||
| $optionsdata = Contact_Form::esc_shortcode_val( | ||
| wp_json_encode( | ||
| array( | ||
| array( | ||
| 'label' => 'Red', | ||
| ), | ||
| array( | ||
| 'label' => 'Blue', | ||
| ), | ||
| array( | ||
| 'label' => 'Other', | ||
| 'isOther' => true, | ||
| ), | ||
| ), | ||
| JSON_UNESCAPED_SLASHES | JSON_HEX_TAG | JSON_HEX_AMP | ||
| ) | ||
| ); | ||
|
|
||
| $shortcode = "[contact-field type='radio' label='Favorite Color' allowOther='1' options='Red,Blue,Other' optionsdata='{$optionsdata}' /]"; | ||
|
|
||
| $form = new Contact_Form( | ||
| array( | ||
| 'title' => 'Test Form', | ||
| ), | ||
| $shortcode | ||
| ); | ||
|
|
||
| $response = Feedback::from_submission( $_post_data, $form ); | ||
| $feedback_post_id = $response->save(); | ||
| $saved_response = Feedback::get( $feedback_post_id ); | ||
|
|
||
| // Get the field | ||
| $fields = $response->get_fields(); | ||
| $this->assertNotEmpty( $fields, 'Fields should not be empty' ); | ||
|
|
||
| $field = reset( $fields ); // Get first field | ||
| $this->assertInstanceOf( Feedback_Field::class, $field, 'Field should be a Feedback_Field instance' ); | ||
|
|
||
| // Test that the value is preserved | ||
| $this->assertEquals( 'Other: Purple with green stripes', $field->get_value(), 'Value should be preserved as submitted' ); | ||
|
|
||
| // Test that metadata is set correctly | ||
| $meta = $field->get_meta(); | ||
| $this->assertTrue( $meta['is_other_option'], 'is_other_option should be true' ); | ||
| $this->assertEquals( 'Other', $meta['other_label'], 'other_label should be "Other"' ); | ||
| $this->assertEquals( 'Purple with green stripes', $meta['other_user_value'], 'other_user_value should contain custom text' ); | ||
|
|
||
| // Test saved response | ||
| $saved_fields = $saved_response->get_fields(); | ||
| $saved_field = reset( $saved_fields ); | ||
|
|
||
| $this->assertEquals( 'Other: Purple with green stripes', $saved_field->get_value(), 'Saved value should match' ); | ||
| $saved_meta = $saved_field->get_meta(); | ||
| $this->assertTrue( $saved_meta['is_other_option'], 'Saved is_other_option should be true' ); | ||
| $this->assertEquals( 'Purple with green stripes', $saved_meta['other_user_value'], 'Saved other_user_value should match' ); | ||
| } |
There was a problem hiding this comment.
The test doesn't cover the validation scenario where the "Other" option is selected on a required field but no custom text is provided. This is a critical validation path mentioned in the PR description and implemented in the validation logic (lines 434-438 of class-contact-form-field.php). Consider adding a test case that verifies the validation error is triggered when custom text is missing.
| field.isOtherSelected = true; | ||
| field.otherLabel = otherLabel; |
There was a problem hiding this comment.
The code directly mutates the field object properties (field.isOtherSelected, field.otherLabel) rather than using a proper state update mechanism. This breaks immutability principles and could lead to issues with state management, reactivity, and debugging. Consider using a proper state update function or immutable update patterns.
| /* "Other" option text input styles */ | ||
| .jetpack-other-text-input-wrapper { | ||
| display: none; | ||
| margin-left: 1.5em; |
There was a problem hiding this comment.
The CSS uses physical properties (margin-left) instead of logical properties (margin-inline-start). For RTL language support and better internationalization, consider using logical properties as per the project's coding guidelines.
| <BlockControls> | ||
| <ToolbarGroup> | ||
| <ToolbarButton | ||
| onClick={ () => setAttributes( { isOther: ! isOther } ) } | ||
| className={ isOther ? 'is-pressed' : undefined } | ||
| > | ||
| { __( 'Other', 'jetpack-forms' ) } | ||
| </ToolbarButton> | ||
| </ToolbarGroup> | ||
| </BlockControls> |
There was a problem hiding this comment.
The "Other" toolbar button is shown for all field types (checkbox, radio) but the functionality is only implemented for radio fields. This could confuse users who see the button on non-radio field options where it won't work. Consider conditionally displaying the toolbar button only for radio field types.
| " . $other_input_styles . " | ||
| placeholder='" . esc_attr( __( 'Please specify...', 'jetpack-forms' ) ) . "' | ||
| value='' | ||
| aria-labelledby='" . $other_label_id . "' |
There was a problem hiding this comment.
The text input has aria-labelledby pointing to a label with class 'screen-reader-text', but the input is inside a region that's hidden (display: none) until the "Other" option is selected. When the region becomes visible with aria-live='polite', the screen reader might not properly announce the input's purpose. Consider adding an aria-describedby attribute that references the parent radio button's label to provide better context about what option this text input relates to.
| aria-labelledby='" . $other_label_id . "' | |
| aria-labelledby='" . $other_label_id . "' | |
| aria-describedby='" . esc_attr( $has_option_is_other_id ) . "' |
| // For radio fields, check if value uses an "Other" pattern | ||
| let isOtherSelected = false; | ||
| let otherLabel = null; | ||
| if ( type === 'radio' && value && value.includes( ': ' ) ) { | ||
| // Check if this might be an "Other" value (has the pattern "Label: text") | ||
| const colonIndex = value.indexOf( ': ' ); | ||
| const possibleLabel = value.substring( 0, colonIndex ); | ||
| // Simple heuristic: if the label part is short (< 30 chars), it might be an "Other" label | ||
| if ( possibleLabel.length < 30 ) { |
There was a problem hiding this comment.
The heuristic for detecting "Other" options in radio fields is fragile. The code checks if a value contains ": " and if the label part is less than 30 characters to determine if it's an "Other" option. This could lead to false positives - any radio option value that happens to contain a colon followed by text with a short prefix would be incorrectly identified as an "Other" option. Consider using a more reliable mechanism, such as explicitly checking against known "Other" labels from the field configuration or using a metadata flag.
| // For radio fields, check if value uses an "Other" pattern | |
| let isOtherSelected = false; | |
| let otherLabel = null; | |
| if ( type === 'radio' && value && value.includes( ': ' ) ) { | |
| // Check if this might be an "Other" value (has the pattern "Label: text") | |
| const colonIndex = value.indexOf( ': ' ); | |
| const possibleLabel = value.substring( 0, colonIndex ); | |
| // Simple heuristic: if the label part is short (< 30 chars), it might be an "Other" label | |
| if ( possibleLabel.length < 30 ) { | |
| // For radio fields, check if value uses an explicit "Other" pattern | |
| let isOtherSelected = false; | |
| let otherLabel = null; | |
| if ( type === 'radio' && typeof value === 'string' && value.includes( ': ' ) ) { | |
| const colonIndex = value.indexOf( ': ' ); | |
| const possibleLabel = value.substring( 0, colonIndex ); | |
| // Only treat as "Other" when the label explicitly matches a known "Other" option. | |
| if ( possibleLabel === 'Other' ) { |
| field.isOtherSelected = false; | ||
| field.otherLabel = null; | ||
|
|
There was a problem hiding this comment.
The code directly mutates the field object properties (field.isOtherSelected, field.otherLabel) rather than using a proper state update mechanism. This breaks immutability principles and could lead to issues with state management, reactivity, and debugging. Consider using a proper state update function or immutable update patterns.
| field.isOtherSelected = false; | |
| field.otherLabel = null; | |
| const updatedField = { | |
| ...field, | |
| isOtherSelected: false, | |
| otherLabel: null, | |
| }; | |
| context.fields = { | |
| ...context.fields, | |
| [ fieldId ]: updatedField, | |
| }; |
Code Coverage SummaryCoverage changed in 7 files. Only the first 5 are listed here.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
simison
force-pushed
the
add/other-option-to-dropdown
branch
from
3085195 to
0d958f6
Compare
| }, | ||
| otherLabel: { | ||
| type: 'string', | ||
| default: 'Other', |
| allowOther: { | ||
| type: 'boolean', | ||
| default: false, | ||
| }, | ||
| otherLabel: { | ||
| type: 'string', | ||
| default: 'Other', | ||
| }, |
There was a problem hiding this comment.
Wondering if we could simplify by not having other options at the field level, and only have isOther at the inner option block? Once that's present, the other text field gets rendered right below it?
| &.is-other { | ||
| font-style: italic; | ||
| } |
There was a problem hiding this comment.
Let's instead allow italic & bold styling for option field (can be separate PR) so that this is something users can customize, instead of us dictating the fixed style.
In the theme I tested this didn't even work likely due theme override.
| if ( type === 'radio' && value && value.includes( ': ' ) ) { | ||
| // Check if this might be an "Other" value (has the pattern "Label: text") | ||
| const colonIndex = value.indexOf( ': ' ); | ||
| const possibleLabel = value.substring( 0, colonIndex ); | ||
| // Simple heuristic: if the label part is short (< 30 chars), it might be an "Other" label | ||
| if ( possibleLabel.length < 30 ) { | ||
| isOtherSelected = true; | ||
| otherLabel = possibleLabel; | ||
| } |
There was a problem hiding this comment.
The heuristic for detecting "Other" values based on a colon separator and a 30-character label length is fragile. This could incorrectly identify legitimate radio values that happen to contain a colon and short text before it (e.g., "Time: Morning", "Status: Active"). This detection should rely on more explicit metadata or field attributes rather than pattern matching the value string.
| if ( ! empty( $option['isOther'] ) ) { | ||
| $field .= "<input id='" . esc_attr( $radio_id ) . "' type='radio' name='" . esc_attr( $id ) . "' value='" . esc_attr( $radio_value ) . "' data-wp-on--change='actions.onOtherRadioChange' data-other-label='" . esc_attr( $option_label ) . "' " . $class . checked( $option_label, $value, false ) . ' ' . ( $required ? "required aria-required=\'true\'" : '' ) . '/> '; | ||
| $has_option_is_other_id = esc_attr( $radio_id ); | ||
| } else { | ||
| $field .= "<input id='" . esc_attr( $radio_id ) . "' type='radio' name='" . esc_attr( $id ) . "' value='" . esc_attr( $radio_value ) . "' data-wp-on--change='actions.onFieldChange' " . $class . checked( $option_label, $value, false ) . ' ' . ( $required ? "required aria-required=\'true\'" : '' ) . '/> '; | ||
| } |
There was a problem hiding this comment.
The input elements are concatenated as very long single lines (lines 1362 and 1365), making the code difficult to read and maintain. Consider breaking these into multi-line strings with proper indentation for better readability.
| if ( ! empty( $option['isOther'] ) ) { | |
| $field .= "<input id='" . esc_attr( $radio_id ) . "' type='radio' name='" . esc_attr( $id ) . "' value='" . esc_attr( $radio_value ) . "' data-wp-on--change='actions.onOtherRadioChange' data-other-label='" . esc_attr( $option_label ) . "' " . $class . checked( $option_label, $value, false ) . ' ' . ( $required ? "required aria-required=\'true\'" : '' ) . '/> '; | |
| $has_option_is_other_id = esc_attr( $radio_id ); | |
| } else { | |
| $field .= "<input id='" . esc_attr( $radio_id ) . "' type='radio' name='" . esc_attr( $id ) . "' value='" . esc_attr( $radio_value ) . "' data-wp-on--change='actions.onFieldChange' " . $class . checked( $option_label, $value, false ) . ' ' . ( $required ? "required aria-required=\'true\'" : '' ) . '/> '; | |
| } | |
| $on_change = 'actions.onFieldChange'; | |
| if ( ! empty( $option['isOther'] ) ) { | |
| $on_change = 'actions.onOtherRadioChange'; | |
| $has_option_is_other_id = esc_attr( $radio_id ); | |
| } | |
| $required_attr = $required ? " required aria-required='true'" : ''; | |
| $input_attributes = "id='" . esc_attr( $radio_id ) . "'"; | |
| $input_attributes .= " type='radio'"; | |
| $input_attributes .= " name='" . esc_attr( $id ) . "'"; | |
| $input_attributes .= " value='" . esc_attr( $radio_value ) . "'"; | |
| $input_attributes .= " data-wp-on--change='" . $on_change . "'"; | |
| if ( ! empty( $option['isOther'] ) ) { | |
| $input_attributes .= " data-other-label='" . esc_attr( $option_label ) . "'"; | |
| } | |
| $input_attributes .= ' ' . $class; | |
| $input_attributes .= checked( $option_label, $value, false ); | |
| $input_attributes .= $required_attr; | |
| $field .= '<input ' . $input_attributes . ' /> '; |
| $custom_text = trim( $custom_text ); | ||
|
|
||
| // For required fields, ensure custom text is not empty | ||
| if ( $this->get_attribute( 'required' ) && empty( $custom_text ) ) { |
There was a problem hiding this comment.
The validation logic at line 435 checks if the field is required using $this->get_attribute( 'required' ), but this could be inconsistent with the $isRequired context used elsewhere in validation. Consider using a consistent method to check the required status throughout the validation block.
| if ( $this->get_attribute( 'required' ) && empty( $custom_text ) ) { | |
| if ( $isRequired && empty( $custom_text ) ) { |
| id='" . $other_text_id . "' | ||
| name='" . esc_attr( $id ) . "-other-text' | ||
| type='text' | ||
| class='grunion-field' |
There was a problem hiding this comment.
The text input element is missing the jetpack-other-text-input class that's defined in the CSS (grunion.scss). The class attribute should include both grunion-field jetpack-other-text-input to ensure both the general field styles and the specific "Other" input styles are applied correctly.
| class='grunion-field' | |
| class='grunion-field jetpack-other-text-input' |
|
|
||
| // Get the text input value | ||
| const fieldset = event.target.closest( 'fieldset' ); | ||
| const otherTextInput = fieldset?.querySelector( '.jetpack-other-text-input' ); |
There was a problem hiding this comment.
The querySelector .jetpack-other-text-input in lines 396 and 421 won't find any elements because the rendered HTML uses class grunion-field instead. This will cause the text input clearing and focusing features to fail. The class name should match what's rendered in the PHP (class-contact-form-field.php line 1434).
| const otherTextInput = fieldset?.querySelector( '.jetpack-other-text-input' ); | |
| const otherTextInput = fieldset?.querySelector( '.grunion-field' ); |
| Significance: minor | ||
| Type: added | ||
|
|
||
| Add 'Other' option support for radio fields with custom text input, including ARIA accessibility, proper validation, and metadata storage for form submissions |
There was a problem hiding this comment.
The changelog filename "add-other-option-to-dropdown" mentions "dropdown" but the feature is actually for radio fields, not dropdown/select fields. The filename should be "add-other-option-to-radio-fields" or similar to accurately reflect the feature scope.
| Add 'Other' option support for radio fields with custom text input, including ARIA accessibility, proper validation, and metadata storage for form submissions | |
| Add 'Other' option support for dropdown fields with custom text input, including ARIA accessibility, proper validation, and metadata storage for form submissions |
| if ( ! empty( $option['isOther'] ) && $this->get_attribute( 'allowother' ) ) { | ||
| $field .= $this->render_other_input_field( $radio_id, $required, $id, $this->field_styles ); | ||
| } |
There was a problem hiding this comment.
In the else branch (lines 1374-1410), the $option variable is a string after being processed by strip_tags() on line 1378. Attempting to access $option['isOther'] as an array on line 1405 will cause a PHP error. This code path is for legacy forms that don't use optionsdata. Since the "Other" functionality requires optionsdata to work properly, this check will always fail and should be removed from this branch.
| private function render_other_input_field( $has_option_is_other_id, $required, $id, $field_styles ) { | ||
| $other_text_id = esc_attr( $has_option_is_other_id ) . '-other-text'; | ||
| $other_label_id = esc_attr( $has_option_is_other_id ) . '-other-label'; | ||
| $other_label_text = __( 'Please specify…', 'jetpack-forms' ); | ||
| $aria_required_attr = $required ? "aria-required='true'" : ''; | ||
|
|
||
| // Prepare styles for the text input to match other form fields | ||
| $other_input_styles = ! empty( $field_styles ) ? " style='" . esc_attr( $field_styles ) . "' " : ''; | ||
|
|
||
| // Render text input wrapper with aria-live for screen reader announcements | ||
| $field = "<div class='jetpack-other-text-input-wrapper' data-wp-class--is-visible='state.isOtherSelected' role='region' aria-live='polite'>"; | ||
|
|
||
| // Add a visually-hidden label for screen readers | ||
| $field .= "<label id='" . $other_label_id . "' for='" . $other_text_id . "' class='screen-reader-text'>" . esc_html( $other_label_text ) . '</label>'; | ||
|
|
||
| $field .= "<input | ||
| id='" . $other_text_id . "' | ||
| name='" . esc_attr( $id ) . "-other-text' | ||
| type='text' | ||
| class='grunion-field' | ||
| " . $other_input_styles . " | ||
| placeholder='" . esc_attr( __( 'Please specify…', 'jetpack-forms' ) ) . "' | ||
| value='' | ||
| aria-labelledby='" . $other_label_id . "' | ||
| " . $aria_required_attr . " | ||
| data-wp-on--input='actions.onOtherTextInput' | ||
| data-wp-bind--disabled='!state.isOtherSelected' | ||
| data-wp-class--has-value='state.hasFieldValue' />"; | ||
| $field .= '</div>'; | ||
| return $field; | ||
| } |
There was a problem hiding this comment.
The indentation is inconsistent - the function body starts with tabs but should align with the rest of the class methods. The opening brace should be at the same indentation level as the 'private' keyword, and the function body should be indented one level from there.
| <ToolbarGroup> | ||
| <ToolbarButton | ||
| onClick={ () => setAttributes( { isOther: ! isOther } ) } | ||
| className={ isOther ? 'is-pressed' : undefined } |
There was a problem hiding this comment.
The className logic is inconsistent between the standalone (line 84) and non-standalone (line 136) branches. In the standalone branch, clsx is used with a conditional object { 'is-pressed': isOther }, while in the non-standalone branch it uses a ternary expression. For consistency and code maintainability, both should use the same pattern, preferably clsx with the conditional object pattern.
| className={ isOther ? 'is-pressed' : undefined } | |
| className={ clsx( { 'is-pressed': isOther } ) } |
| class='grunion-field' | ||
| " . $other_input_styles . " | ||
| placeholder='" . esc_attr( __( 'Please specify…', 'jetpack-forms' ) ) . "' | ||
| value='' |
There was a problem hiding this comment.
When an "Other" option has a saved value like "Other: custom text", the text input field needs to be pre-populated with "custom text" so users can see and edit their previous response. Currently, the text input always renders with value='' (line 1451). The code should extract the custom text portion from $value when rendering the form with a saved "Other" response and populate it in the text input field.
| * @param string $has_option_is_other_id The ID of the "Other" option. | ||
| * @param bool $required Whether the main field is required. | ||
| * @param string $id The base ID of the main field. | ||
| * @param string $field_styles The styles to apply to the text input field. | ||
| * | ||
| * @return string The HTML for the "Other" text input field. | ||
| */ | ||
| private function render_other_input_field( $has_option_is_other_id, $required, $id, $field_styles ) { | ||
| $other_text_id = esc_attr( $has_option_is_other_id ) . '-other-text'; | ||
| $other_label_id = esc_attr( $has_option_is_other_id ) . '-other-label'; |
There was a problem hiding this comment.
The variable name 'has_option_is_other_id' is confusing and doesn't clearly communicate its purpose. It actually represents the radio button ID for the "Other" option, not whether the option exists. Consider renaming it to something clearer like 'other_option_id' or 'other_radio_id'.
| * @param string $has_option_is_other_id The ID of the "Other" option. | |
| * @param bool $required Whether the main field is required. | |
| * @param string $id The base ID of the main field. | |
| * @param string $field_styles The styles to apply to the text input field. | |
| * | |
| * @return string The HTML for the "Other" text input field. | |
| */ | |
| private function render_other_input_field( $has_option_is_other_id, $required, $id, $field_styles ) { | |
| $other_text_id = esc_attr( $has_option_is_other_id ) . '-other-text'; | |
| $other_label_id = esc_attr( $has_option_is_other_id ) . '-other-label'; | |
| * @param string $other_option_id The ID of the "Other" option. | |
| * @param bool $required Whether the main field is required. | |
| * @param string $id The base ID of the main field. | |
| * @param string $field_styles The styles to apply to the text input field. | |
| * | |
| * @return string The HTML for the "Other" text input field. | |
| */ | |
| private function render_other_input_field( $other_option_id, $required, $id, $field_styles ) { | |
| $other_text_id = esc_attr( $other_option_id ) . '-other-text'; | |
| $other_label_id = esc_attr( $other_option_id ) . '-other-label'; |
simison
force-pushed
the
add/other-option-to-dropdown
branch
from
db34e2f to
551ae97
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
projects/packages/forms/src/contact-form/class-contact-form-plugin.php:686
- The 'allowother' attribute is never set on the field when processing block attributes. While individual options have 'isOther' preserved in optionsdata, the field-level 'allowother' attribute that is checked in validation (line 390) and rendering (line 1360) is never populated. This means the "Other" option functionality will not work properly. Add logic in block_attributes_to_shortcode_attributes() to set 'allowother' to true when any option has isOther set to true.
// Preserve isOther attribute from the option block so
// server-side rendering can attach special handlers.
if ( ! empty( $option['attrs']['isOther'] ) ) {
$option_data['isOther'] = true;
}
if ( isset( $option_attrs['class'] ) ) {
$option_data['class'] = $option_attrs['class'] . ' wp-block-jetpack-option';
} else {
$option_data['class'] = 'wp-block-jetpack-option';
}
if ( isset( $option_attrs['style'] ) ) {
$option_data['style'] = $option_attrs['style'];
}
$options[] = $option_label; // Legacy shortcode attribute in case filters are using it.
$options_data[] = $option_data;
}
}
$atts['options'] = implode( ',', $options );
$atts['optionsdata'] = \wp_json_encode( $options_data, JSON_UNESCAPED_SLASHES | JSON_HEX_AMP );
| // For radio fields, check if value uses an "Other" pattern | ||
| let isOtherSelected = false; | ||
| let otherLabel = null; | ||
| if ( type === 'radio' && value && value.includes( ': ' ) ) { | ||
| // Check if this might be an "Other" value (has the pattern "Label: text") | ||
| const colonIndex = value.indexOf( ': ' ); | ||
| const possibleLabel = value.substring( 0, colonIndex ); | ||
| // Simple heuristic: if the label part is short (< 30 chars), it might be an "Other" label | ||
| if ( possibleLabel.length < 30 ) { | ||
| isOtherSelected = true; | ||
| otherLabel = possibleLabel; |
There was a problem hiding this comment.
The heuristic for detecting "Other" values (checking if label before ": " is < 30 characters) is fragile and could lead to false positives. For example, legitimate radio values like "Product: Widget" or "Color: Blue" could be misinterpreted as "Other" responses. Consider using a more reliable approach, such as checking against the actual options data to determine if an option has isOther set, or storing an explicit flag when "Other" is selected.
| // For radio fields, check if value uses an "Other" pattern | |
| let isOtherSelected = false; | |
| let otherLabel = null; | |
| if ( type === 'radio' && value && value.includes( ': ' ) ) { | |
| // Check if this might be an "Other" value (has the pattern "Label: text") | |
| const colonIndex = value.indexOf( ': ' ); | |
| const possibleLabel = value.substring( 0, colonIndex ); | |
| // Simple heuristic: if the label part is short (< 30 chars), it might be an "Other" label | |
| if ( possibleLabel.length < 30 ) { | |
| isOtherSelected = true; | |
| otherLabel = possibleLabel; | |
| // For radio fields, determine if an "Other" option is selected based on the field options. | |
| let isOtherSelected = false; | |
| let otherLabel = null; | |
| if ( | |
| type === 'radio' && | |
| typeof value === 'string' && | |
| extra && | |
| Array.isArray( extra.options ) | |
| ) { | |
| const matchingOtherOption = extra.options.find( | |
| option => | |
| option && | |
| option.isOther && | |
| typeof option.label === 'string' && | |
| value.startsWith( option.label + ': ' ) | |
| ); | |
| if ( matchingOtherOption ) { | |
| isOtherSelected = true; | |
| otherLabel = matchingOtherOption.label; |
| otherPlaceholder: { | ||
| type: 'string', | ||
| default: __( 'Please specify…', 'jetpack-forms' ), | ||
| }, |
There was a problem hiding this comment.
The otherPlaceholder attribute is defined in the block schema but is never actually used in the frontend rendering. The frontend always uses the hardcoded string "Please specify…" from the translation function. Consider either removing the otherPlaceholder attribute if customization isn't needed, or pass it through to the frontend rendering in render_other_input_field().
| otherPlaceholder: { | |
| type: 'string', | |
| default: __( 'Please specify…', 'jetpack-forms' ), | |
| }, |
|
|
||
| /* "Other" option text input styles */ | ||
| .jetpack-other-text-input-wrapper { | ||
| margin-left: 1.5em; |
There was a problem hiding this comment.
The margin-left property should use the logical property margin-inline-start instead to support RTL languages. This ensures the margin is applied correctly in both LTR and RTL layouts.
simison
force-pushed
the
add/other-option-to-dropdown
branch
from
551ae97 to
7561d62
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
projects/packages/forms/src/contact-form/class-contact-form-plugin.php:686
- The
allowOtherattribute is never set on the field during block-to-shortcode conversion. While the code preserves theisOtherattribute for individual options (lines 666-668), it doesn't set theallowOtherfield-level attribute that is required by the validation logic (line 390 in class-contact-form-field.php) and the feedback processing logic (line 425 in class-feedback.php). This means the "Other" option validation and processing will not work correctly since$allow_otherwill always be null/false. The code should set$atts['allowOther'] = truewhen any option hasisOtherset to true.
foreach ( $option_blocks as $option ) {
$option_label = trim( $option['attrs']['label'] ?? '' );
if ( $option_label ) {
$option_attrs = self::get_block_support_classes_and_styles( 'jetpack/option', $option['attrs'] );
$option_data = array( 'label' => $option_label );
// Preserve isOther attribute from the option block so
// server-side rendering can attach special handlers.
if ( ! empty( $option['attrs']['isOther'] ) ) {
$option_data['isOther'] = true;
}
if ( isset( $option_attrs['class'] ) ) {
$option_data['class'] = $option_attrs['class'] . ' wp-block-jetpack-option';
} else {
$option_data['class'] = 'wp-block-jetpack-option';
}
if ( isset( $option_attrs['style'] ) ) {
$option_data['style'] = $option_attrs['style'];
}
$options[] = $option_label; // Legacy shortcode attribute in case filters are using it.
$options_data[] = $option_data;
}
}
$atts['options'] = implode( ',', $options );
$atts['optionsdata'] = \wp_json_encode( $options_data, JSON_UNESCAPED_SLASHES | JSON_HEX_AMP );
| // we have an isOther option so we don't render the duplicate | ||
| // appended "Other" radio later. | ||
| if ( ! empty( $option['isOther'] ) ) { | ||
| $field .= "<input id='" . esc_attr( $radio_id ) . "' type='radio' name='" . esc_attr( $id ) . "' value='" . esc_attr( $radio_value ) . "' data-wp-on--change='actions.onOtherRadioChange' data-other-label='" . esc_attr( $option_label ) . "' " . $class . checked( $option_label, $value, false ) . ' ' . ( $required ? "required aria-required=\'true\'" : '' ) . '/> '; |
There was a problem hiding this comment.
The checked state logic uses checked( $option_label, $value, false ), which compares the option label (e.g., "Other") with the field value. However, when a user has previously submitted an "Other" option with custom text, the value will be in the format "Other: custom text" (e.g., "Other: Purple with green stripes"). This means the exact match will fail and the "Other" radio button won't be pre-selected when editing a form or viewing a previously submitted value. The comparison should check if the value either equals the label OR starts with the label followed by ": " for "Other" options.
| $field .= "<input id='" . esc_attr( $radio_id ) . "' type='radio' name='" . esc_attr( $id ) . "' value='" . esc_attr( $radio_value ) . "' data-wp-on--change='actions.onOtherRadioChange' data-other-label='" . esc_attr( $option_label ) . "' " . $class . checked( $option_label, $value, false ) . ' ' . ( $required ? "required aria-required=\'true\'" : '' ) . '/> '; | |
| $field .= "<input id='" . esc_attr( $radio_id ) . "' type='radio' name='" . esc_attr( $id ) . "' value='" . esc_attr( $radio_value ) . "' data-wp-on--change='actions.onOtherRadioChange' data-other-label='" . esc_attr( $option_label ) . "' " . $class . checked( true, ( $value === $option_label || ( is_string( $value ) && 0 === strpos( $value, $option_label . ': ' ) ) ), false ) . ' ' . ( $required ? "required aria-required=\'true\'" : '' ) . '/> '; |
| // For radio fields, check if value uses an "Other" pattern | ||
| let isOtherSelected = false; | ||
| let otherLabel = null; | ||
| if ( type === 'radio' && value && value.includes( ': ' ) ) { | ||
| // Check if this might be an "Other" value (has the pattern "Label: text") | ||
| const colonIndex = value.indexOf( ': ' ); | ||
| const possibleLabel = value.substring( 0, colonIndex ); | ||
| // Simple heuristic: if the label part is short (< 30 chars), it might be an "Other" label | ||
| if ( possibleLabel.length < 30 ) { | ||
| isOtherSelected = true; | ||
| otherLabel = possibleLabel; | ||
| } | ||
| } |
There was a problem hiding this comment.
The heuristic used to detect "Other" options (checking if label part is < 30 characters) is fragile and could lead to false positives. For example, if a user legitimately enters a value like "Category: Sports" for a non-Other field, it would incorrectly be detected as an Other option. This logic should instead check against the actual field configuration using the allowother attribute and optionsdata to determine if the field has an Other option, rather than relying on a simple string pattern match.
| id='" . $other_text_id . "' | ||
| name='" . esc_attr( $id ) . "-other-text' | ||
| type='text' | ||
| class='grunion-field' |
There was a problem hiding this comment.
The JavaScript code is trying to select elements with the class .jetpack-other-text-input, but the actual input element rendered in render_other_input_field only has the class grunion-field (line 1448). This mismatch means that the input field will never be found by the JavaScript selectors on lines 396 and 421 in view.js, preventing the "Other" text input from being cleared or focused properly. The input element needs to also include the class jetpack-other-text-input.
| class='grunion-field' | |
| class='grunion-field jetpack-other-text-input' |
github-actions
bot
added
the
[Plugin] Jetpack
Introduces an 'Other' option for radio fields, allowing users to enter custom text. Includes editor UI controls, ARIA accessibility, validation, metadata storage, frontend handling, and comprehensive tests for correct processing and storage of 'Other' responses.
simison
force-pushed
the
add/other-option-to-dropdown
branch
from
fc76e32 to
a018102
Compare
| otherTextInput.value = ''; | ||
| } |
There was a problem hiding this comment.
Direct DOM manipulation with otherTextInput.value = '' (line 398) bypasses React's state management and the WordPress Interactivity API. This can cause inconsistencies between the actual DOM state and the application state. Consider using the Interactivity API's state management or triggering a proper input event to update the value through the appropriate handlers.
| font-size: var(--jetpack--contact-form--font-size, 16px); | ||
|
|
||
| &:focus { | ||
| outline: 2px solid; |
There was a problem hiding this comment.
The focus outline style uses outline: 2px solid; without specifying a color, which means it will inherit the currentColor. This could result in poor contrast if the text color doesn't provide sufficient contrast against the background. Consider explicitly setting an outline color or using a color with good contrast, such as outline-color: var(--jetpack--contact-form--focus-color, #0073aa); to ensure accessibility compliance with WCAG contrast requirements.
| outline: 2px solid; | |
| outline: 2px solid; | |
| outline-color: var(--jetpack--contact-form--focus-color, #0073aa); |
| value={ isFocusedOtherPlaceholder ? otherPlaceholder : '' } | ||
| placeholder={ otherPlaceholder } |
There was a problem hiding this comment.
The input behavior is confusing: the value is cleared on blur (line 194 shows empty string when not focused), but the placeholder shows the stored value. This creates a poor user experience where users can't see what they typed after unfocusing, and they have to refocus to edit. Consider keeping the value visible at all times by using value={ otherPlaceholder } instead of conditionally clearing it.
| value={ isFocusedOtherPlaceholder ? otherPlaceholder : '' } | |
| placeholder={ otherPlaceholder } | |
| value={ otherPlaceholder } | |
| placeholder={ otherPlaceholder || __( 'Please specify…', 'jetpack-forms' ) } |
| // Render text input wrapper with aria-live for screen reader announcements | ||
| $field = "<div class='jetpack-other-text-input-wrapper' data-wp-class--is-visible='state.isOtherSelected' role='region' aria-live='polite'>"; |
There was a problem hiding this comment.
Using aria-live='polite' on a wrapper that toggles visibility can cause screen readers to announce content changes unexpectedly. When the "Other" option is selected and the text input becomes visible, screen readers will announce the entire region contents. This could be disruptive. Consider removing the aria-live attribute and relying on the focus management (which already moves focus to the input) to provide the necessary feedback to screen reader users.
| // Render text input wrapper with aria-live for screen reader announcements | |
| $field = "<div class='jetpack-other-text-input-wrapper' data-wp-class--is-visible='state.isOtherSelected' role='region' aria-live='polite'>"; | |
| // Render text input wrapper; rely on focus management instead of aria-live for announcements | |
| $field = "<div class='jetpack-other-text-input-wrapper' data-wp-class--is-visible='state.isOtherSelected' role='region'>"; |
| $field .= '</span>'; | ||
| $field .= '</label>'; | ||
| if ( ! empty( $option['isOther'] ) ) { | ||
| $placeholder = ! empty( $option['otherPlaceholder'] ) ? $option['otherPlaceholder'] : ''; |
There was a problem hiding this comment.
The $placeholder value from $option['otherPlaceholder'] is passed directly to render_other_input_field() and then output using esc_attr() at line 1450. However, the placeholder value could contain malicious content if it's not properly sanitized when the option block data is created. Ensure that otherPlaceholder is sanitized when the options data is initially processed or add sanitization here before passing to the render function.
| $placeholder = ! empty( $option['otherPlaceholder'] ) ? $option['otherPlaceholder'] : ''; | |
| $placeholder = ! empty( $option['otherPlaceholder'] ) ? sanitize_text_field( wp_unslash( $option['otherPlaceholder'] ) ) : ''; |
| * | ||
| * @return string The HTML for the "Other" text input field. | ||
| */ | ||
| private function render_other_input_field( $has_option_is_other_id, $required, $id, $field_styles, $placeholder ) { |
There was a problem hiding this comment.
The parameter name $has_option_is_other_id is unclear and misleading. This parameter receives a radio button's ID (like 'favoritecolor-other'), not a boolean indicating whether an option "has" the "isOther" property. Consider renaming to $radio_id or $option_id to better reflect what the parameter actually contains.
Resolves FORMS-136
Introduces an 'Other' option for radio fields, allowing users to enter custom text. Includes editor UI controls, ARIA accessibility, validation, metadata storage, frontend handling, and comprehensive tests for correct processing and storage of 'Other' responses.
See
Screen.Recording.2026-01-06.at.9.52.24.AM.mov
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No
Testing instructions:
vendor/bin/phpunit --filter test_radio_field_with_other_option