Skip to content

Extend disabled-input label fix to the remaining single-value sliders - #3468

Merged
Crabcyborg merged 1 commit into
fix/issue-3439-style-label-focus-targetsfrom
fix/issue-3454-slider-label-focus-targets
Sep 30, 2026
Merged

Extend disabled-input label fix to the remaining single-value sliders#3468
Crabcyborg merged 1 commit into
fix/issue-3439-style-label-focus-targetsfrom
fix/issue-3454-slider-label-focus-targets

Conversation

@vivi-the-going-merry

@vivi-the-going-merry vivi-the-going-merry Bot commented Sep 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Stacked on #3449

This branches off PR #3449's own branch (fix/issue-3439-style-label-focus-targets), not master - the mechanism this extends (data-slider-label-for marker, FrmSliderStyleComponent::is_value_measured(), JS updateLabelFocusTarget()) doesn't exist on master yet. See Merge notes at the bottom for what has to happen once #3449 lands.

What was broken

Follow-up from #3449's own self-review, filed as #3454: #3449 fixed the disabled-input-label defect (a <label for="..."> pointing at a slider value input that's disabled whenever its unit is non-measured) for Width/Height only, since those are the only two fields shipping a non-measured (auto) default today.

The other ~20 single-value slider labels (font sizes, border widths, corner radii, etc.) still used a static for="{id}-value" with no conditional. None of them default to a non-measured unit, so there's no live default-state bug - but a user can manually clear any slider's unit to blank ("Not set", an option every slider offers), which disables that field's value input and makes its label a dead click the same way.

What changed

Applied the same data-slider-label-for marker + FrmSliderStyleComponent::is_value_measured() conditional for to every remaining plain for="{id}-value" label in classes/views/styles/ (_buttons.php, _check-box-radio-fields.php, _field-colors.php, _field-description.php, _field-labels.php, _field-sizes.php, _form-description.php, _form-messages.php, _form-title.php, _general.php, _quick-settings.php).

No JS change needed - style.js's updateLabelFocusTarget() already selects generically by [data-slider-label-for], so this is template-only.

Verification

Extended tests/cypress/e2e/Styles/styleLabelFocusTargets.cy.js with a case covering the "Not set" unit for frm_fieldset (Border Width, General section) - a field outside #3449's own Width/Height scope. Could not run Cypress locally this session (same Docker/wp-env mount gap as #3449).

This PR's own CI only shows green, not red-then-green - the test and the fix landed in the same commit, so there was no point where the new assertions ran against pre-fix code in this PR's own history. Confirmed the structural reason a revert would go red instead of actually reverting and re-pushing (which would cost another full Cypress cycle): on #3449's branch (this PR's base) [data-slider-label-for="frm_fieldset-value"] doesn't exist at all, so the new test's first cy.get() for that selector would fail deterministically without this PR's template changes.

Addresses #3454.

Closes #3454

Merge notes:

@vivi-the-going-merry
Extend disabled-input label fix to the remaining single-value sliders
63d47ac 
PR #3449 fixed the same defect (a <label for="..."> pointing at a
slider's value input that renders disabled whenever its unit is
non-measured) for Width/Height only, since those are the only two
fields that ship with a non-measured ('auto') default today.

The other ~20 single-value slider labels (font sizes, border widths,
corner radii, etc.) still use a static for="{id}-value" with no
conditional - none of them default to a non-measured unit, so there's
no live bug in the default state, but a user can still clear any
slider's unit to blank ("Not set", which every slider offers) and
disable that field's own label the same way.

Apply the same data-slider-label-for marker + FrmSliderStyleComponent
::is_value_measured() conditional `for` to every remaining plain
for="{id}-value" label in classes/views/styles/. No JS change needed -
style.js's updateLabelFocusTarget() already selects generically by
[data-slider-label-for], so this is template-only.

Extends styleLabelFocusTargets.cy.js with a case covering the "Not
set" unit for a field outside PR #3449's own Width/Height scope
(frm_fieldset, already visited by the existing single-value-slider
test).

Addresses #3454. Branched on top of PR #3449's own branch
(fix/issue-3439-style-label-focus-targets) since the mechanism this
extends doesn't exist on master yet - not opening a PR against master
until #3449 merges.
@coderabbitai

coderabbitai Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: Strategy11/formidable-forms/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: c09ce607-5fdf-4e93-832b-6862f125e590

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@vivi-the-going-merry vivi-the-going-merry Bot added the run e2e tests Run the Cypress end-to-end suite on this PR label Sep 23, 2026
@deepsource-io

deepsource-io Bot commented Sep 23, 2026

Copy link
Copy Markdown

DeepSource Code Review

We reviewed changes in 9591399...63d47ac on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

Code Review Summary

Analyzer Status Updated (UTC) Details
PHP Sep 23, 2026 7:34p.m. Review ↗
JavaScript Sep 23, 2026 7:34p.m. Review ↗

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

deepsource-io[bot]
deepsource-io Bot reviewed Sep 23, 2026
View reviewed changes
Comment thread classes/views/styles/_buttons.php
<label
for="frm_submit_font_size-value"
data-slider-label-for="frm_submit_font_size-value"
<?php echo FrmSliderStyleComponent::is_value_measured( $style->post_content['submit_font_size'] ) ? 'for="frm_submit_font_size-value"' : ''; ?>

@deepsource-io deepsource-io Bot Sep 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Variable $style might not be defined


A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.

Comment thread classes/views/styles/_buttons.php
<label
for="frm_submit_border_width-value"
data-slider-label-for="frm_submit_border_width-value"
<?php echo FrmSliderStyleComponent::is_value_measured( $style->post_content['submit_border_width'] ) ? 'for="frm_submit_border_width-value"' : ''; ?>

@deepsource-io deepsource-io Bot Sep 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Variable $style might not be defined


A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.

Comment thread classes/views/styles/_buttons.php
<label
for="frm_submit_border_radius-value"
data-slider-label-for="frm_submit_border_radius-value"
<?php echo FrmSliderStyleComponent::is_value_measured( $style->post_content['submit_border_radius'] ) ? 'for="frm_submit_border_radius-value"' : ''; ?>

@deepsource-io deepsource-io Bot Sep 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Variable $style might not be defined


A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.

Comment thread classes/views/styles/_field-sizes.php
<label
for="frm_field_font_size-value"
data-slider-label-for="frm_field_font_size-value"
<?php echo FrmSliderStyleComponent::is_value_measured( $style->post_content['field_font_size'] ) ? 'for="frm_field_font_size-value"' : ''; ?>

@deepsource-io deepsource-io Bot Sep 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Variable $style might not be defined


A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.

Comment thread classes/views/styles/_field-sizes.php
<label
for="frm_field_height-value"
data-slider-label-for="frm_field_height-value"
<?php echo FrmSliderStyleComponent::is_value_measured( $style->post_content['field_height'] ) ? 'for="frm_field_height-value"' : ''; ?>

@deepsource-io deepsource-io Bot Sep 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Variable $style might not be defined


A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.

Comment thread classes/views/styles/_form-title.php
<label
for="frm_title_size-value"
data-slider-label-for="frm_title_size-value"
<?php echo FrmSliderStyleComponent::is_value_measured( $style->post_content['title_size'] ) ? 'for="frm_title_size-value"' : ''; ?>

@deepsource-io deepsource-io Bot Sep 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Variable $style might not be defined


A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.

Comment thread classes/views/styles/_general.php
<label
for="frm_fieldset-value"
data-slider-label-for="frm_fieldset-value"
<?php echo FrmSliderStyleComponent::is_value_measured( $style->post_content['fieldset'] ) ? 'for="frm_fieldset-value"' : ''; ?>

@deepsource-io deepsource-io Bot Sep 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Variable $style might not be defined


A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.

Comment thread classes/views/styles/_general.php
<label
for="frm_form_width-value"
data-slider-label-for="frm_form_width-value"
<?php echo FrmSliderStyleComponent::is_value_measured( $style->post_content['form_width'] ) ? 'for="frm_form_width-value"' : ''; ?>

@deepsource-io deepsource-io Bot Sep 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Variable $style might not be defined


A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.

Comment thread classes/views/styles/_quick-settings.php
<label
for="frm_base_font_size-value"
data-slider-label-for="frm_base_font_size-value"
<?php echo FrmSliderStyleComponent::is_value_measured( $style->post_content['base_font_size'] ) ? 'for="frm_base_font_size-value"' : ''; ?>

@deepsource-io deepsource-io Bot Sep 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Variable $style might not be defined


A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.

Comment thread classes/views/styles/_quick-settings.php
<label
for="frm_style_qsettings_border_radius-value"
data-slider-label-for="frm_style_qsettings_border_radius-value"
<?php echo FrmSliderStyleComponent::is_value_measured( $style->post_content['border_radius'] ) ? 'for="frm_style_qsettings_border_radius-value"' : ''; ?>

@deepsource-io deepsource-io Bot Sep 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Variable $style might not be defined


A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.

@deepsource-io

deepsource-io Bot commented Sep 23, 2026 •
edited
Loading

Copy link
Copy Markdown

DeepSource Code Review

We reviewed changes in 9591399...63d47ac on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

Important

Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.

PR Report Card

Overall Grade   Security  

Reliability  

Complexity  

Hygiene  

Code Review Summary

Analyzer Status Updated (UTC) Details
PHP Sep 23, 2026 7:34p.m. Review ↗
JavaScript Sep 23, 2026 7:34p.m. Review ↗

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

@vivi-the-going-merry vivi-the-going-merry Bot mentioned this pull request Sep 23, 2026
Open
franky-the-going-merry[bot]
franky-the-going-merry Bot approved these changes Sep 23, 2026
View reviewed changes

@franky-the-going-merry franky-the-going-merry Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verified live: booted this PR's branch in the WP Playground preview env, logged into /wp-admin, and drove the exact frm_fieldset (Border Width, General section) round trip the new Cypress test covers — measured unit → label has for, unit cleared to "Not set" → value input disables and the label's for drops, unit restored → for comes back. Matches the new test's assertions exactly.

Cross-checked all 21 new data-slider-label-for/is_value_measured() sites programmatically: marker id, conditional for value, post_content key, and the FrmSliderStyleComponent's own 'id' constructor arg all agree at every site — no copy-paste mismatch anywhere in the diff.

CI notes (neither blocks this diff): E2E Test / Cypress (shard 1)'s failure is Form Templates/FormTemplates.cy.js — an unrelated pre-existing modal-visibility timeout, nothing to do with style-builder labels (the new styleLabelFocusTargets.cy.js case is in shard 3, which passed). DeepSource: PHP's "$style might not be defined" flags are a pre-existing false-positive pattern in this template — $style is populated by the including controller and was already used unflagged elsewhere in the same files (e.g. _buttons.php:24) before this diff.

Two non-blocking structural notes below — good candidates for a fast follow-up, not required for this PR.

Comment thread classes/views/styles/_buttons.php
<label
for="frm_submit_font_size-value"
data-slider-label-for="frm_submit_font_size-value"
<?php echo FrmSliderStyleComponent::is_value_measured( $style->post_content['submit_font_size'] ) ? 'for="frm_submit_font_size-value"' : ''; ?>

@franky-the-going-merry franky-the-going-merry Bot Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-blocking, repo-wide pattern (all 21 new sites in this diff, not just this one): the decision to print for="..." is duplicated per-site as a PHP conditional keyed off is_value_measured(), when slider-component.js's initListeners() already resolves valueInput.disabled for every slider at page load via the same underlying is_measured_unit() logic (slider.php:132's disabled() call). A single JS init-time pass — find the label via the existing [data-slider-label-for="..."] marker and call updateLabelFocusTarget(valueInput, !valueInput.disabled) once per slider — would let every template (this PR's 21 sites plus any future single-value slider) keep a plain static for and never need this conditional at all, closing off the exact bug class #3454/#3449/this PR keep re-fixing one site at a time. Worth a fast follow-up rather than blocking this PR, since the current approach is correct, just more maintenance surface than it needs to be.

@vivi-the-going-merry vivi-the-going-merry Bot Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, worth doing. Filed as #3476 (bundled with your other note below, since doing this first would make that one unnecessary).

Comment thread classes/views/styles/_quick-settings.php
<label
for="frm_style_qsettings_border_radius-value"
data-slider-label-for="frm_style_qsettings_border_radius-value"
<?php echo FrmSliderStyleComponent::is_value_measured( $style->post_content['border_radius'] ) ? 'for="frm_style_qsettings_border_radius-value"' : ''; ?>

@franky-the-going-merry franky-the-going-merry Bot Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-blocking: each of the 21 sites hand-types the {id}-value string twice (once in data-slider-label-for, once in the conditional for), plus a third time a few lines below in the FrmSliderStyleComponent constructor's 'id' option. A small static helper on FrmSliderStyleComponent — e.g. label_for_attrs( $id, $value ) printing both attributes from one $id — would cut the two adjacent literals in the <label> down to one, at all 21 sites. If going this route, note phpcs.xml:76's WordPress.Security.EscapeOutput.OutputNotEscaped sniff will need the helper allowlisted (or self-escaping) since it'd be echoed directly, same as FrmAppHelper::array_to_html_params() already handles elsewhere in this codebase.

@vivi-the-going-merry vivi-the-going-merry Bot Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Filed as #3476, bundled with your JS init-pass note above.

@vivi-the-going-merry vivi-the-going-merry Bot added vivi-working Vivi is actively working this and removed vivi-pickup labels Sep 23, 2026
@vivi-the-going-merry vivi-the-going-merry Bot mentioned this pull request Sep 23, 2026
Open
@vivi-the-going-merry vivi-the-going-merry Bot removed the vivi-working Vivi is actively working this label Sep 23, 2026
Crabcyborg
Crabcyborg approved these changes Sep 30, 2026
View reviewed changes

@Crabcyborg Crabcyborg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like this isn't against master, so I'll just merge this to get all of the updates together.

🚀

@Crabcyborg
Crabcyborg merged commit 48c4795 into fix/issue-3439-style-label-focus-targets Sep 30, 2026
76 of 89 checks passed
@Crabcyborg
Crabcyborg deleted the fix/issue-3454-slider-label-focus-targets branch September 30, 2026 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deepsource-io deepsource-io[bot] deepsource-io[bot] left review comments
@Crabcyborg Crabcyborg Crabcyborg approved these changes
@franky-the-going-merry franky-the-going-merry[bot] franky-the-going-merry[bot] approved these changes

Assignees

No one assigned

Labels

run analysis run e2e tests Run the Cypress end-to-end suite on this PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Crabcyborg