From aa4902fa7cd03e92f9e41a18a2e34ab27a87acb5 Mon Sep 17 00:00:00 2001 From: "vivi-the-going-merry[bot]" <308115520+vivi-the-going-merry[bot]@users.noreply.github.com> Date: Thu, 17 Sep 2026 16:40:52 -0600 Subject: [PATCH] A11y: dedupe ids colliding between advanced/quick style settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- classes/views/styles/_form-title.php | 6 +-- classes/views/styles/_quick-settings.php | 18 ++++---- tests/phpunit/base/FrmUnitTest.php | 19 ++++++++ .../styles/test_FrmStylesController.php | 46 +++++++++++++++++++ 4 files changed, 77 insertions(+), 12 deletions(-) diff --git a/classes/views/styles/_form-title.php b/classes/views/styles/_form-title.php index 3a322f1453..be28362bdc 100644 --- a/classes/views/styles/_form-title.php +++ b/classes/views/styles/_form-title.php @@ -4,8 +4,8 @@ } ?>
-
@@ -14,7 +14,7 @@ class="frm-style-item-heading">get_field_name( 'title_color' ), $style->post_content['title_color'], array( - 'id' => 'frm_fieldset_color', + 'id' => 'frm_title_color', 'action_slug' => 'title_color', ) ); diff --git a/classes/views/styles/_quick-settings.php b/classes/views/styles/_quick-settings.php index 82b4716d1c..af08997ba6 100644 --- a/classes/views/styles/_quick-settings.php +++ b/classes/views/styles/_quick-settings.php @@ -118,8 +118,8 @@ class="frm-style-item-heading">
-
@@ -128,7 +128,7 @@ class="frm-style-item-heading">post_content['field_margin'], array( - 'id' => 'frm_field_margin', + 'id' => 'frm_style_qsettings_field_margin', 'max_value' => 100, 'will_change' => array( $frm_style->get_field_name( 'field_margin' ), @@ -139,8 +139,8 @@ class="frm-style-item-heading">
-
@@ -149,7 +149,7 @@ class="frm-style-item-heading">post_content['field_pad'], array( - 'id' => 'frm_field_pad', + 'id' => 'frm_style_qsettings_field_pad', 'max_value' => 100, 'will_change' => array( $frm_style->get_field_name( 'field_pad' ), @@ -206,8 +206,8 @@ class="frm-style-item-heading">
-
@@ -216,7 +216,7 @@ class="frm-style-item-heading">post_content['border_radius'], array( - 'id' => 'frm_border_radius', + 'id' => 'frm_style_qsettings_border_radius', 'max_value' => 50, 'will_change' => array( $frm_style->get_field_name( 'border_radius' ), diff --git a/tests/phpunit/base/FrmUnitTest.php b/tests/phpunit/base/FrmUnitTest.php index 1dad220136..4cf62b0cab 100644 --- a/tests/phpunit/base/FrmUnitTest.php +++ b/tests/phpunit/base/FrmUnitTest.php @@ -843,4 +843,23 @@ protected function use_frm_role( $role ) { break; } } + + /** + * Assert that none of the given HTML element ids appears more than once in some + * rendered HTML (aria_id_unique — a duplicate id breaks any ARIA property that + * references it, since the reference can no longer resolve to a single element). + * + * @since x.x + * + * @param string $html + * @param array $ids + * + * @return void + */ + protected function assert_no_duplicate_element_ids( $html, $ids ) { + foreach ( $ids as $id ) { + $count = preg_match_all( '/\bid=["\']' . preg_quote( $id, '/' ) . '["\']/', $html ); + $this->assertSame( 1, $count, 'Expected exactly one element with id "' . $id . '"' ); + } + } } diff --git a/tests/phpunit/styles/test_FrmStylesController.php b/tests/phpunit/styles/test_FrmStylesController.php index d4061d4bdb..0224aee62b 100644 --- a/tests/phpunit/styles/test_FrmStylesController.php +++ b/tests/phpunit/styles/test_FrmStylesController.php @@ -48,6 +48,52 @@ private function get_custom_stylesheet() { return $stylesheet_urls; } + /** + * The styler edit page's "Quick Settings" panel and its "Advanced Settings" + * accordion sections both render into the DOM unconditionally (only one is + * shown at a time via CSS), so an id reused between a quick-settings control + * and its advanced-settings equivalent collides and breaks any ARIA property + * that references it (aria_id_unique). + * + * @covers FrmStylesController::render_style_page + */ + public function test_render_style_page_has_no_duplicate_ids() { + $this->set_current_user_to_1(); + + // render_style_page() reads $_GET to decide the view ('edit' vs 'list'); a leftover + // 'form'/'style_id' from another test would silently switch this to the list view. + $_GET = array(); + + $form_id = $this->factory->form->create(); + $form = FrmForm::getOne( $form_id ); + $frm_style = new FrmStyle( 'default' ); + $active_style = $frm_style->get_one(); + + ob_start(); + $this->run_private_method( + array( 'FrmStylesController', 'render_style_page' ), + array( $active_style, $form, $active_style ) + ); + $html = ob_get_clean(); + + $this->assert_no_duplicate_element_ids( + $html, + array( + 'frm_field_pad', + 'frm_field_margin', + 'frm_border_radius', + 'frm_fieldset_color', + // The renamed quick-settings/form-title ids themselves, so a future edit that + // deletes one of these elements (instead of just re-duplicating its id) still + // fails loudly here. + 'frm_style_qsettings_field_pad', + 'frm_style_qsettings_field_margin', + 'frm_style_qsettings_border_radius', + 'frm_title_color', + ) + ); + } + /** * @covers FrmStylesController::save_style * @covers FrmStyle::update