A11y: dedupe ids colliding between advanced/quick style settings (aria_id_unique) - #3368
vivi-the-going-merry[bot] wants to merge 1 commit into
Conversation
The style-editor page's "Advanced Settings" accordion sections and its
"Quick Settings" panel both render into the DOM unconditionally (CSS
toggles which is visible), so four ids were reused between a quick-settings
control and its advanced-settings equivalent for the same underlying
field: frm_field_pad, frm_field_margin, and frm_border_radius. A fourth,
frm_fieldset_color, was reused between the General and Form Title
accordion sections — a copy-paste leftover, since the Form Title instance
actually maps to the title_color field, not a fieldset border color.
Renamed the quick-settings trio with the frm_style_qsettings_ prefix
already used by four sibling controls in the same file, and renamed the
Form Title one to frm_title_color to match its own field and its sibling
frm_title_size/frm_title_margins ids. No id referenced by JS
(getElementById('frm_field_pad')) was touched, so behavior is unchanged.
Closes Strategy11/formidable-pro#6696
vivi-the-going-merry
Bot
added
run analysis
run e2e tests
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Sep 17, 2026 10:46p.m. | Review ↗ |
| JavaScript | |
Sep 17, 2026 10:46p.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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3368 +/- ##
============================================
+ Coverage 28.36% 28.57% +0.21%
- Complexity 9806 9844 +38
============================================
Files 160 160
Lines 32921 33022 +101
============================================
+ Hits 9339 9437 +98
- Misses 23582 23585 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
What was broken
The style-editor page's "Advanced Settings" accordion sections and its "Quick Settings" panel both render into the DOM unconditionally (CSS toggles which is visible), so four ids were reused between a quick-settings control and its advanced-settings equivalent for the same underlying field —
frm_field_pad,frm_field_margin,frm_border_radius— plus a fourth,frm_fieldset_color, reused between the General and Form Title accordion sections (a copy-paste leftover: the Form Title instance actually controlstitle_color, not a fieldset border color). Confirmed against a real CI IBM checker log format: exactly 4 duplicate-id violations (IBM Equal Access rulearia_id_unique), matching this count.What changed
frm_style_qsettings_prefix already used by 4 sibling controls in the same file (classes/views/styles/_quick-settings.php).frm_title_color(classes/views/styles/_form-title.php), matching its own field name and its siblingfrm_title_size/frm_title_marginsids.<label for="...">attributes alongside each renamed id.No id referenced by JS (
js/admin/style.js'sgetElementById('frm_field_pad')) was touched — it already resolved to the (unchanged) accordion element in document order before this fix, since the accordion renders before Quick Settings; behavior there is unchanged. The "advanced settings mirrors quick settings" live sync is keyed on field name, not DOM id, so it's unaffected too.Markup-only — no CSS or JS changes.
How it was verified
Added a PHPUnit test (
tests/phpunit/styles/test_FrmStylesController.php) rendering the real style-editor page markup and asserting each of the 4 originally-colliding ids, plus the 4 renamed ids, appears exactly once — via a small reusableassert_no_duplicate_element_ids()helper added to the sharedFrmUnitTestbase class. Confirmed red (2 occurrences) before the fix, green after, against the real local PHPUnit rig. PHPCS clean.Self-reviewed with 2 parallel Agent-tool lenses (correctness, reuse/simplification) against this diff — both confirmed no issues; correctness lens specifically traced the
getElementByIdquestion above to confirm no behavior change, and one lens's suggestion (also assert the new renamed ids, not just the old ones — guards against a future "fix by deletion" regression) was applied.Closes Strategy11/formidable-pro#6696
🤖 Generated with Claude Code