Fix keyboard accessibility and tab order for inputs with "more icon"#2495
Conversation
- Add tabindex to allowed icon tags in FrmAppHelper - Reposition more icon after input field for correct tab order - Add frm-input-icon class and tabindex="0" for keyboard navigation
- Update getInputForIcon and getIconForInput to handle repositioned icons - Add keyboard event handler for Enter/Space keys on more icons - Remove obsolete focus handler for token proxy inputs
- Pass event parameter through maybeShowInlineModal and showInlineModal functions - Add conditional input focusing to prevent focus conflicts with keyboard navigation - Remove unused showBuilderModal function - Convert arrow function to regular function for better browser compatibility - Improve keyboard interaction handling for Enter/Space key events
- Add visible focus indicator for .frm-input-icon elements - Use primary-500 color for consistent accessibility styling - Improves keyboard navigation visual feedback
- Move smart values modal trigger icons to appear after input fields instead of before - Add frm-input-icon class for consistent styling - Add tabindex="0" to make icons keyboard accessible - Affected files: FrmFieldType.php, sub-field-options.php, default-value-setting.php, value-format.php
Add tabindex="0" to the inline modal icon test case to match the accessibility improvements made to the actual icon implementation.
- Target .frm-show-inline-modal elements specifically instead of generic tabindex selectors - Ensures proper focus management when disabling/enabling toggle groups - Prevents interference with other focusable elements
Add optional chaining operator to safely access classList property when nextElementSibling might be null.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2495 +/- ##
============================================
- Coverage 27.35% 27.33% -0.02%
- Complexity 8710 8727 +17
============================================
Files 140 140
Lines 28750 28854 +104
============================================
+ Hits 7864 7887 +23
- Misses 20886 20967 +81 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughAdds keyboard focusability to inline-modal trigger icons (tabindex and CSS focus styles), permits Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant I as Trigger Icon (.frm-show-inline-modal / .frm-input-icon)
participant JS as maybeShowInlineModal
participant MOD as showInlineModal(icon,input,event)
participant UI as Inline Modal
U->>I: Click or keydown (Enter/Space)
I->>JS: maybeShowInlineModal(event)
JS->>MOD: showInlineModal(this, input?, event)
alt Trigger via Enter/Space
MOD->>UI: Open inline modal
note right of MOD #DCEEF7: Skip moving focus to input when triggered by Enter/Space
else Trigger via click/other
MOD->>UI: Open inline modal
MOD->>UI: Focus related input (if present)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/views/frm-fields/back-end/settings.php (1)
450-454: Unrelated bug: undefined variable $ftyp (typo).This will emit a PHP notice and can break the option label when $ftype is a string.
- FrmHtmlHelper::echo_dropdown_option( - is_array( $ftype ) ? $ftype['name'] : $ftyp, + FrmHtmlHelper::echo_dropdown_option( + is_array( $ftype ) ? $ftype['name'] : $ftype, $fkey === $field['type'], $type_option_params );
🧹 Nitpick comments (11)
resources/scss/admin/components/form/_input-icons.scss (1)
88-90: Use :focus-visible and add outline-offset for better a11y.Prevents showing focus ring on mouse click and improves contrast.
- &:focus { - outline: 1px solid var(--primary-500); - } + &:focus-visible { + outline: 2px solid var(--primary-500); + outline-offset: 2px; + }classes/models/fields/FrmFieldType.php (1)
537-543: Add button semantics and relationships to the trigger icon.Expose intent to AT and wire the control to its modal target.
- FrmAppHelper::icon_by_class( - 'frm_icon_font frm_more_horiz_solid_icon frm-show-inline-modal frm-input-icon', - array( - 'data-open' => $special_default ? 'frm-tax-box-' . $field['id'] : 'frm-smart-values-box', - 'title' => esc_attr__( 'Toggle Options', 'formidable' ), - 'tabindex' => '0', - ) - ); + $target_id = $special_default ? 'frm-tax-box-' . $field['id'] : 'frm-smart-values-box'; + FrmAppHelper::icon_by_class( + 'frm_icon_font frm_more_horiz_solid_icon frm-show-inline-modal frm-input-icon', + array( + 'data-open' => $target_id, + 'role' => 'button', + 'aria-controls' => $target_id, + 'aria-expanded' => 'false', + 'aria-label' => esc_attr__( 'Toggle options', 'formidable' ), + 'tabindex' => '0', + ) + );classes/views/frm-fields/back-end/combo-field/sub-field-options.php (1)
55-62: Mirror button semantics on the sub-field trigger.Aligns with the primary trigger for consistent a11y.
- FrmAppHelper::icon_by_class( - 'frm_icon_font frm_more_horiz_solid_icon frm-show-inline-modal frm-input-icon', - array( - 'data-open' => 'frm-smart-values-box', - 'tabindex' => '0', - ) - ); + FrmAppHelper::icon_by_class( + 'frm_icon_font frm_more_horiz_solid_icon frm-show-inline-modal frm-input-icon', + array( + 'data-open' => 'frm-smart-values-box', + 'role' => 'button', + 'aria-controls' => 'frm-smart-values-box', + 'aria-expanded' => 'false', + 'aria-label' => esc_attr__( 'Toggle options', 'formidable' ), + 'tabindex' => '0', + ) + );js/src/settings-components/components/toggle-group/toggle-group.js (1)
127-129: Preserve/restore prior tab order and mark disabled state.Avoids forcing 0 when re-enabled; adds aria-disabled for clarity.
- element.querySelectorAll( '.frm-show-inline-modal[tabindex]' ).forEach( - inlineModal => inlineModal.tabIndex = isChecked ? -1 : 0 - ); + element.querySelectorAll( '.frm-show-inline-modal[tabindex]' ).forEach(inlineModal => { + if ( isChecked ) { + if ( inlineModal.dataset.prevTabIndex === undefined ) { + inlineModal.dataset.prevTabIndex = String(inlineModal.tabIndex); + } + inlineModal.tabIndex = -1; + inlineModal.setAttribute('aria-disabled', 'true'); + } else { + const prev = inlineModal.dataset.prevTabIndex; + inlineModal.tabIndex = prev !== undefined ? parseInt(prev, 10) : 0; + inlineModal.removeAttribute('aria-disabled'); + delete inlineModal.dataset.prevTabIndex; + } + });classes/views/frm-fields/back-end/default-value-setting.php (1)
26-29: Nice: convert array→string before rendering and move icon after input.Small follow-up: set button semantics on the icon (role/aria-label/controls/expanded) to match other triggers.
classes/views/frm-fields/back-end/value-format.php (2)
11-11: Harden attribute escaping for data-fid.Prefer esc_attr(absint(...)) for consistency with adjacent attrs.
- <input type="text" class="frm_long_input frm_format_opt" value="<?php echo esc_attr( $field['format'] ); ?>" name="field_options[format_<?php echo absint( $field['id'] ); ?>]" id="frm_format_<?php echo absint( $field['id'] ); ?>" data-fid="<?php echo intval( $field['id'] ); ?>" /> + <input type="text" class="frm_long_input frm_format_opt" value="<?php echo esc_attr( $field['format'] ); ?>" name="field_options[format_<?php echo absint( $field['id'] ); ?>]" id="frm_format_<?php echo absint( $field['id'] ); ?>" data-fid="<?php echo esc_attr( absint( $field['id'] ) ); ?>" />
13-19: Add button semantics and ARIA for the interactive SVG (replace title; wire controls/expanded).Same rationale as in settings.php: provide an accessible name and correct semantics.
FrmAppHelper::icon_by_class( 'frm_icon_font frm_more_horiz_solid_icon frm-show-inline-modal frm-input-icon', array( 'data-open' => 'frm-input-mask-box', - 'title' => esc_attr__( 'Toggle Options', 'formidable' ), + 'role' => 'button', + 'aria-label' => esc_attr__( 'Open input mask options', 'formidable' ), + 'aria-haspopup' => 'dialog', + 'aria-controls' => 'frm-input-mask-box', + 'aria-expanded' => 'false', 'tabindex' => '0', ) );Note: Ensure kses allows these attributes as suggested in the helper change above, and that JS toggles aria-expanded and prevents page scroll on Space.
js/src/admin/admin.js (4)
6677-6712: A11y + robustness: toggle aria-expanded and guard input.id; include legacy Spacebar
- While gating focus on keyboard is correct, also update the trigger’s ARIA state when opening/closing the inline modal.
- Guard against missing input.id before replace/setAttribute to avoid runtime errors.
- Include legacy 'Spacebar' to future-proof Space key handling.
-function showInlineModal( icon, input, event ) { +function showInlineModal( icon, input, event ) { const box = document.getElementById( icon.getAttribute( 'data-open' ) ), container = jQuery( icon ).closest( 'p,ul' ), inputTrigger = ( typeof input !== 'undefined' ); if ( container.hasClass( 'frm-open' ) ) { container.removeClass( 'frm-open' ); box.classList.add( 'frm_hidden' ); + icon.setAttribute( 'aria-expanded', 'false' ); } else { if ( ! inputTrigger ) { input = getInputForIcon( icon ); } if ( input !== null ) { if ( ! inputTrigger ) { - const { key } = event ?? {}; - if ( key !== 'Enter' && key !== ' ' ) { + const { key } = event ?? {}; + if ( key !== 'Enter' && key !== ' ' && key !== 'Spacebar' ) { input.focus(); } } container.after( box ); - box.setAttribute( 'data-fills', input.id.replace( '-proxy-input', '' ) ); + if ( input?.id ) { + box.setAttribute( 'data-fills', input.id.replace( '-proxy-input', '' ) ); + } if ( box.id.indexOf( 'frm-calc-box' ) === 0 ) { popCalcFields( box, true ); } } container.addClass( 'frm-open' ); box.classList.remove( 'frm_hidden' ); + icon.setAttribute( 'aria-expanded', 'true' );
8811-8815: Prevent false-positive sibling selectionWhen the icon has frm-input-icon, we assume the previousElementSibling is the input/textarea. Guard the element type to avoid null/undefined usage later (e.g., input.id.replace).
- if ( moreIcon.classList.contains( 'frm-input-icon' ) ) { - return moreIcon.previousElementSibling; - } + if ( moreIcon.classList.contains( 'frm-input-icon' ) ) { + const prev = moreIcon.previousElementSibling; + if ( prev && ( prev.tagName === 'INPUT' || prev.tagName === 'TEXTAREA' ) ) { + return prev; + } + }
8836-8839: Nice adjacency fast-path; add resilient fallback (optional)If markup ever changes, consider also querying the parent for a nearby .frm-input-icon.
if ( input.nextElementSibling?.classList.contains( 'frm-input-icon' ) ) { return input.nextElementSibling; } +const nearby = input.parentNode?.querySelector( '.frm-input-icon' ); +if ( nearby ) { + return nearby; +}
10839-10845: Keyboard: handle legacy 'Spacebar' and align button semantics
- Include 'Spacebar' for older browsers.
- Optionally move Space handling to keyup to better mirror native button behavior (Enter on keydown, Space on keyup).
- if ( key === 'Enter' || key === ' ' ) { + if ( key === 'Enter' || key === ' ' || key === 'Spacebar' ) { event.preventDefault(); maybeShowInlineModal.call( this, event ); }If you opt to mirror native semantics:
- Keep Enter on keydown as-is.
- Add a parallel keyup handler for Space/Spacebar.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
js/formidable-settings-components.js.mapis excluded by!**/*.map,!**/*.map
📒 Files selected for processing (11)
classes/helpers/FrmAppHelper.php(1 hunks)classes/models/fields/FrmFieldType.php(1 hunks)classes/views/frm-fields/back-end/combo-field/sub-field-options.php(1 hunks)classes/views/frm-fields/back-end/default-value-setting.php(1 hunks)classes/views/frm-fields/back-end/settings.php(1 hunks)classes/views/frm-fields/back-end/value-format.php(1 hunks)js/formidable-settings-components.js(1 hunks)js/src/admin/admin.js(5 hunks)js/src/settings-components/components/toggle-group/toggle-group.js(1 hunks)resources/scss/admin/components/form/_input-icons.scss(1 hunks)tests/phpunit/misc/test_FrmAppHelper.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
classes/views/frm-fields/back-end/combo-field/sub-field-options.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4623)icon_by_class(1227-1264)
classes/views/frm-fields/back-end/settings.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4623)icon_by_class(1227-1264)
classes/views/frm-fields/back-end/value-format.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4623)icon_by_class(1227-1264)
classes/views/frm-fields/back-end/default-value-setting.php (1)
classes/models/fields/FrmFieldType.php (3)
default_value_to_string(725-736)show_default_value_field(555-563)display_smart_values_modal_trigger_icon(534-544)
🪛 Biome (2.1.2)
js/formidable-settings-components.js
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Run PHPCS inspection
- GitHub Check: Run ESLint
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (4)
js/formidable-settings-components.js (1)
1-1: Bundled file: ignore lints; keep minified output unlinted.Static-analysis “used before declaration”/“noFunctionAssign” errors stem from the bundler/minifier. Exclude this built artifact from linters and reviews; focus checks on source files (js/src/**).
classes/helpers/FrmAppHelper.php (1)
1294-1295: Allowing tabindex for SVG icons looks good.Matches the new keyboard-focusable triggers; scoped via kses_icon filter. No issues.
tests/phpunit/misc/test_FrmAppHelper.php (1)
364-365: Test update matches new sanitization allowance.Expecting tabindex="0" on SVG is correct.
js/src/admin/admin.js (1)
6672-6675: Pass event into showInlineModal — LGTMGood call passing the event through so focus behavior can depend on activation method.
lauramekaj1
left a comment
lauramekaj1
left a comment
There was a problem hiding this comment.
@shervElmi I tested it and it's working as expected. Thank you!
…ility-more-icon-tab-order
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
js/formidable-settings-components.js (2)
1-1: Restore original tabindex when re-enabling inline modal triggers.In K(...), disabled sections set triggers to tabIndex -1, but on re-enable you hardcode to 0, which can change author-defined order. Store and restore the previous value, and toggle aria-disabled for SRs.
Apply this diff to the logic that updates
.frm-show-inline-modal[tabindex]:- e.querySelectorAll(".frm-show-inline-modal[tabindex]").forEach((function(e){return e.tabIndex=o?-1:0})) + e.querySelectorAll(".frm-show-inline-modal[tabindex]").forEach((function(el){ + if (o) { + if (el.dataset.prevTabIndex === undefined) { + var prev = el.getAttribute("tabindex"); + if (prev !== null) { el.dataset.prevTabIndex = prev; } + } + el.tabIndex = -1; + el.setAttribute("aria-disabled", "true"); + } else { + if (el.dataset.prevTabIndex !== undefined) { + el.setAttribute("tabindex", el.dataset.prevTabIndex); + delete el.dataset.prevTabIndex; + } else { + el.removeAttribute("tabindex"); + } + el.removeAttribute("aria-disabled"); + } + }))
1-1: Make “expand slider group” icon keyboard-activatable.expandSliderGroup binds click on
.frmsvgonly. Add key handlers for Enter/Space for parity.Apply this diff near the click binding:
- n.addEventListener("click",(function(){i.forEach((function(e){e.classList.toggle(t)}))})) + n.addEventListener("click",(function(){i.forEach((function(e){e.classList.toggle(t)}))})); + n.addEventListener("keydown",(function(ev){ + var k = ev.key; + if (k === "Enter" || k === " ") { + ev.preventDefault(); + i.forEach(function(e){ e.classList.toggle(t); }); + } + })) + n.setAttribute("tabindex","0"); + n.setAttribute("role","button"); + n.setAttribute("aria-expanded","false");Update aria-expanded accordingly when toggling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/helpers/FrmAppHelper.php(1 hunks)js/formidable-settings-components.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/helpers/FrmAppHelper.php
🧰 Additional context used
🪛 Biome (2.1.2)
js/formidable-settings-components.js
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Run PHP Syntax inspection (8.3)
🔇 Additional comments (2)
js/formidable-settings-components.js (2)
1-1: LGTM on bundle; functional parity appears intact.This looks like a minified rebuild with no behavioral deltas in this file. Proceeding with a couple of small robustness/accessibility nits below.
Please QA the keyboard flow for a field with the “more” icon:
- Tab order: input -> icon -> next control.
- Pressing Enter/Space on the icon opens the inline modal and returns focus appropriately on close.
1-1: Confirm key activation for.frm-show-inline-modaltriggers exists elsewhere.This bundle doesn’t add keydown handlers for the inline modal triggers. If they’re spans/divs, ensure Enter/Space behaviors are wired in the admin JS that calls
showInlineModal.Manual check:
- Trigger is focusable via tabindex="0".
- Pressing Enter/Space opens the modal.
- Focus returns to trigger on close.
- Change `const { key } = event ?? {};` to `const { key } = event;`
- Event is not expected to be undefined in this context
- Simplifies code as requested in code review feedback
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
js/src/admin/admin.js (2)
10839-10845: Keyboard support: include legacy “Spacebar”.Some browsers report space as “Spacebar”. Minor tweak for broader compatibility.
- const { key } = event; - if ( key === 'Enter' || key === ' ' ) { + const { key } = event; + if ( key === 'Enter' || key === ' ' || key === 'Spacebar' ) {
8812-8815: Be defensive when the icon isn’t immediately adjacent to the input.If markup changes add a wrapper,
previousElementSiblingmay not be the input. Fall back to existing search logic.- if ( moreIcon.classList.contains( 'frm-input-icon' ) ) { - return moreIcon.previousElementSibling; - } + if ( moreIcon.classList.contains( 'frm-input-icon' ) ) { + const prev = moreIcon.previousElementSibling; + if ( prev && ( prev.tagName === 'INPUT' || prev.tagName === 'TEXTAREA' ) ) { + return prev; + } + // fall through to existing logic below + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/src/admin/admin.js(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (3)
js/src/admin/admin.js (3)
6672-6675: Passing the triggering event and preventing default is correct.This preserves existing click behavior and enables key‑aware handling downstream. Looks good.
8836-8839: Icon lookup for new DOM order looks good.Early return when the right-adjacent node is the input icon is a clean, fast path.
6677-6712: Guard against undefinedeventto avoid runtime errors; handle legacy “Spacebar”.If
showInlineModalis ever called without anevent,const { key } = event;will throw. Make this robust and include the olderSpacebarkey value.Run this to confirm all call sites pass an
event:Apply within this function:
-function showInlineModal( icon, input, event ) { +function showInlineModal( icon, input, event ) { @@ - if ( ! inputTrigger ) { - const { key } = event; - if ( key !== 'Enter' && key !== ' ' ) { - input.focus(); - } - } + if ( ! inputTrigger ) { + const key = event?.key; + const isKeyboardToggle = key === 'Enter' || key === ' ' || key === 'Spacebar'; + if ( ! isKeyboardToggle ) { + input.focus(); + } + }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/cypress/e2e/Forms/formPageDataValidation.cy.js (1)
24-33: Trim H1 text before comparison to reduce flakinessPrevents failures from incidental whitespace changes.
Apply this diff:
- const headingText = $h1.text().toLowerCase(); + const headingText = $h1.text().trim().toLowerCase();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/cypress/e2e/Forms/formPageDataValidation.cy.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (1)
tests/cypress/e2e/Forms/formPageDataValidation.cy.js (1)
32-32: Added allowed heading — LGTMThe new heading string extends coverage for current marketing copy.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/cypress/e2e/Applications/validateApplicationsPage.cy.js (1)
22-31: Normalize comparison and dedupe phrases via an arrayMinor resilience improvement: trim/normalize whitespace and check against an array to reduce brittleness and duplication.
Apply:
- cy.get( 'h1' ).should( $h1 => { - const text = $h1.text().toLowerCase(); - expect( text ).to.satisfy( t => - t.includes( 'the only wordpress form maker & application builder plugin' ) || - t.includes( 'upgrade today to unlock the full power of formidable forms' ) || - t.includes( 'the most advanced wordpress form builder' ) || - t.includes( 'more than just a wordpress form builder' ) || - t.includes( 'get more done in less time with better wordpress forms' ) || - t.includes( 'power your wordpress site like never before' ) - ); - } ); + const allowedH1s = [ + 'the only wordpress form maker & application builder plugin', + 'upgrade today to unlock the full power of formidable forms', + 'the most advanced wordpress form builder', + 'more than just a wordpress form builder', + 'get more done in less time with better wordpress forms', + 'power your wordpress site like never before', + ]; + cy.get( 'h1' ).should( $h1 => { + const normalized = $h1.text().toLowerCase().replace(/\s+/g, ' ').trim(); + expect( allowedH1s.some( p => normalized.includes( p ) ) ).to.be.true; + } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/cypress/e2e/Applications/validateApplicationsPage.cy.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: Run PHP Syntax inspection (8.3)
🔇 Additional comments (1)
tests/cypress/e2e/Applications/validateApplicationsPage.cy.js (1)
29-30: LGTM — allowed H1 variants are consistent across testsBoth Applications and Forms tests include "power your wordpress site like never before" (tests/cypress/e2e/Forms/formPageDataValidation.cy.js:32); no changes required.
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
Thanks Sherv!
I pushed some small commits to get the e2e tests passing again because our site copy keeps changing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
js/src/admin/admin.js (3)
6677-6712: Harden event handling and add ARIA toggles for better a11y.Two small improvements:
- Avoid a potential destructure error if showInlineModal is ever called without an event; include Spacebar for older browsers.
- Toggle aria-controls/aria-expanded on the trigger so screen readers announce state.
Suggested patch:
-function showInlineModal( icon, input, event ) { +function showInlineModal( icon, input, event = null ) { const box = document.getElementById( icon.getAttribute( 'data-open' ) ), container = jQuery( icon ).closest( 'p,ul' ), inputTrigger = ( typeof input !== 'undefined' ); if ( container.hasClass( 'frm-open' ) ) { container.removeClass( 'frm-open' ); box.classList.add( 'frm_hidden' ); + if ( icon && icon.setAttribute ) { + icon.setAttribute( 'aria-expanded', 'false' ); + } } else { if ( ! inputTrigger ) { input = getInputForIcon( icon ); } if ( input !== null ) { if ( ! inputTrigger ) { - const { key } = event; - if ( key !== 'Enter' && key !== ' ' ) { + const key = event && 'key' in event ? event.key : undefined; + if ( key !== 'Enter' && key !== ' ' && key !== 'Spacebar' ) { input.focus(); } } container.after( box ); box.setAttribute( 'data-fills', input.id.replace( '-proxy-input', '' ) ); if ( box.id.indexOf( 'frm-calc-box' ) === 0 ) { popCalcFields( box, true ); } } container.addClass( 'frm-open' ); box.classList.remove( 'frm_hidden' ); + if ( icon && icon.setAttribute ) { + icon.setAttribute( 'aria-controls', box.id ); + icon.setAttribute( 'aria-expanded', 'true' ); + } /** * @since 6.4.1 */ wp.hooks.doAction( 'frm_show_inline_modal', box, icon ); } }
10839-10845: Broaden space key handling for older browsers.Add Spacebar to cover legacy key names.
- if ( key === 'Enter' || key === ' ' ) { + if ( key === 'Enter' || key === ' ' || key === 'Spacebar' ) {
6714-6720: Optional: keep ARIA state in sync when closing via the “dismiss” button.When closing from the inline modal’s dismiss control, also set aria-expanded="false" on the trigger.
Outside this hunk, update dismissInlineModal as follows:
function dismissInlineModal( e ) { /*jshint validthis:true */ e.preventDefault(); this.parentNode.classList.add( 'frm_hidden' ); const trigger = document.querySelector( `[data-open="${ this.parentNode.id }"]` ); if ( trigger ) { trigger.closest( '.frm-open' )?.classList.remove( 'frm-open' ); trigger.setAttribute( 'aria-expanded', 'false' ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/src/admin/admin.js(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Run PHP Syntax inspection (8.3)
🔇 Additional comments (3)
js/src/admin/admin.js (3)
6672-6675: Good: event context is now passed to the modal opener.Passing the event into showInlineModal enables key-aware focus handling. Looks right.
8811-8815: Nice: supports icon-after-input placement.Checking for .frm-input-icon and returning previousElementSibling correctly maps the trigger back to its input after the DOM reorder.
8836-8839: Symmetric lookup from input to icon looks good.Prefers the adjacent .frm-input-icon first, falling back to previous logic. Solid.
Crabcyborg
deleted the
fix/keyboard-accessibility-more-icon-tab-order
branch
…ore-icon-tab-order Fix keyboard accessibility and tab order for inputs with "more icon"
3 participants
This PR fixes keyboard accessibility issues with form inputs that include a "more icon" functionality. Previously, users navigating with keyboard could not properly tab through these inputs due to incorrect DOM order and missing keyboard event handlers.
The fix repositions the more icon after the input field (instead of before) and adds proper keyboard navigation support, ensuring users can access all functionality using only the keyboard.
Related Issue
Fixes https://github.com/Strategy11/formidable-pro/issues/5971
Add-ons Changes