PRO-959: Add edac_get_landmark_types() as single source of truth for landmark detection - #1745
Conversation
…landmark detection Localizes pageScanner's LANDMARK_TAGS/LANDMARK_ROLES/CONDITIONAL_LANDMARK_TAGS/ CONDITIONAL_LANDMARK_ROLES from a new PHP helper instead of hardcoding them in both the scanner and downstream consumers (e.g. the pro plugin's filter UI). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds two PHP helper functions ( ChangesLandmark types configuration pipeline
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces dynamic landmark detection rules by passing canonical landmark types from PHP to the frontend page scanner via localized script data and iframe scan options. The review feedback suggests improving localization in includes/helper-functions.php by using WordPress translation functions instead of hardcoding ucfirst(), and preventing potential runtime errors in src/editorApp/checkPage.js by using optional chaining when accessing window.edac_editor_app.maxAltLength.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function edac_get_landmark_filter_options(): array { | ||
| $types = edac_get_landmark_types(); | ||
|
|
||
| $values = array_unique( | ||
| array_map( | ||
| 'strtolower', | ||
| array_merge( | ||
| $types['tags'], | ||
| $types['roles'], | ||
| $types['conditionalTags'], | ||
| $types['conditionalRoles'] | ||
| ) | ||
| ) | ||
| ); | ||
|
|
||
| return array_map( | ||
| static function ( $value ) { | ||
| return [ | ||
| 'value' => $value, | ||
| 'label' => ucfirst( $value ), | ||
| ]; | ||
| }, | ||
| array_values( $values ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The landmark labels (e.g., 'Main', 'Navigation', 'Banner') are user-facing strings displayed in the Issues Explorer's Landmark filter in the pro plugin. Hardcoding ucfirst( $value ) prevents these labels from being translated.
Using a translation mapping array with WordPress translation functions (__()) ensures these strings can be localized properly.
function edac_get_landmark_filter_options(): array {
$types = edac_get_landmark_types();
$values = array_unique(
array_map(
'strtolower',
array_merge(
$types['tags'],
$types['roles'],
$types['conditionalTags'],
$types['conditionalRoles']
)
)
);
$labels = [
'main' => __( 'Main', 'accessibility-checker' ),
'navigation' => __( 'Navigation', 'accessibility-checker' ),
'banner' => __( 'Banner', 'accessibility-checker' ),
'contentinfo' => __( 'Contentinfo', 'accessibility-checker' ),
'complementary' => __( 'Complementary', 'accessibility-checker' ),
'region' => __( 'Region', 'accessibility-checker' ),
'article' => __( 'Article', 'accessibility-checker' ),
'form' => __( 'Form', 'accessibility-checker' ),
'section' => __( 'Section', 'accessibility-checker' ),
'header' => __( 'Header', 'accessibility-checker' ),
'footer' => __( 'Footer', 'accessibility-checker' ),
'nav' => __( 'Navigation', 'accessibility-checker' ),
'aside' => __( 'Aside', 'accessibility-checker' ),
];
return array_map(
static function ( $value ) use ( $labels ) {
return [
'value' => $value,
'label' => $labels[ $value ] ?? ucfirst( $value ),
];
},
array_values( $values )
);
}| iframeDocument.defaultView.scanOptions = { | ||
| maxAltLength: window.edac_editor_app.maxAltLength, | ||
| ...( landmarkTypes && { | ||
| landmarkTags: landmarkTypes.tags, | ||
| landmarkRoles: landmarkTypes.roles, | ||
| conditionalLandmarkTags: landmarkTypes.conditionalTags, | ||
| conditionalLandmarkRoles: landmarkTypes.conditionalRoles, | ||
| } ), | ||
| }; |
There was a problem hiding this comment.
To prevent potential runtime TypeError exceptions if window.edac_editor_app is undefined or null, use optional chaining (?.) when accessing maxAltLength.
| iframeDocument.defaultView.scanOptions = { | |
| maxAltLength: window.edac_editor_app.maxAltLength, | |
| ...( landmarkTypes && { | |
| landmarkTags: landmarkTypes.tags, | |
| landmarkRoles: landmarkTypes.roles, | |
| conditionalLandmarkTags: landmarkTypes.conditionalTags, | |
| conditionalLandmarkRoles: landmarkTypes.conditionalRoles, | |
| } ), | |
| }; | |
| iframeDocument.defaultView.scanOptions = { | |
| maxAltLength: window?.edac_editor_app?.maxAltLength, | |
| ...( landmarkTypes && { | |
| landmarkTags: landmarkTypes.tags, | |
| landmarkRoles: landmarkTypes.roles, | |
| conditionalLandmarkTags: landmarkTypes.conditionalTags, | |
| conditionalLandmarkRoles: landmarkTypes.conditionalRoles, | |
| } ), | |
| }; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/editorApp/checkPage.js (1)
109-123: ⚡ Quick winUse conditional spread for
maxAltLengthto matchlandmarkTypespattern.Line 115 unconditionally includes
maxAltLengthin thescanOptionsobject, even when it might be falsy. While the PHP code ensuresmaxAltLengthis always at least 1, using a conditional spread would make the code more robust and consistent with howlandmarkTypesis handled.♻️ Refactor to use conditional spread
const injectIframe = ( previewUrl, postID ) => { + const maxAltLength = window?.edac_editor_app?.maxAltLength; + const landmarkTypes = window?.edac_editor_app?.landmarkTypes; - if ( iframeDocument ) { - const landmarkTypes = window?.edac_editor_app?.landmarkTypes; - - if ( window?.edac_editor_app?.maxAltLength || landmarkTypes ) { + if ( iframeDocument && ( maxAltLength || landmarkTypes ) ) { - // Pass scan overrides into the iframe's own window so pageScanner.bundle.js - // (injected below, into this same iframe document) can read them. - iframeDocument.defaultView.scanOptions = { - maxAltLength: window.edac_editor_app.maxAltLength, - ...( landmarkTypes && { - landmarkTags: landmarkTypes.tags, - landmarkRoles: landmarkTypes.roles, - conditionalLandmarkTags: landmarkTypes.conditionalTags, - conditionalLandmarkRoles: landmarkTypes.conditionalRoles, - } ), - }; - } + // Pass scan overrides into the iframe's own window so pageScanner.bundle.js + // (injected below, into this same iframe document) can read them. + iframeDocument.defaultView.scanOptions = { + ...( maxAltLength && { maxAltLength } ), + ...( landmarkTypes && { + landmarkTags: landmarkTypes.tags, + landmarkRoles: landmarkTypes.roles, + conditionalLandmarkTags: landmarkTypes.conditionalTags, + conditionalLandmarkRoles: landmarkTypes.conditionalRoles, + } ), + }; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/editorApp/checkPage.js` around lines 109 - 123, In the scanOptions object assignment, the maxAltLength property is unconditionally included even when it might be falsy. Refactor it to use a conditional spread operator pattern, similar to how landmarkTypes is handled. Wrap the maxAltLength property assignment in a conditional spread using the same pattern: ...( window.edac_editor_app.maxAltLength && { maxAltLength: window.edac_editor_app.maxAltLength, } ) to make the code more robust and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/editorApp/checkPage.js`:
- Around line 109-123: In the scanOptions object assignment, the maxAltLength
property is unconditionally included even when it might be falsy. Refactor it to
use a conditional spread operator pattern, similar to how landmarkTypes is
handled. Wrap the maxAltLength property assignment in a conditional spread using
the same pattern: ...( window.edac_editor_app.maxAltLength && { maxAltLength:
window.edac_editor_app.maxAltLength, } ) to make the code more robust and
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d66de6c3-fa3a-4c84-befd-bdb4e0d6c489
📒 Files selected for processing (4)
admin/class-enqueue-admin.phpincludes/helper-functions.phpsrc/editorApp/checkPage.jssrc/pageScanner/index.js
|
Closing — the proposed single-source-of-truth approach needs rework (edac_get_landmark_filter_options shouldn't live in the free plugin, it's pro-only UI formatting). Leaving PRO-959 open to redo. |
1 participant
Summary
LANDMARK_TAGS,LANDMARK_ROLES,CONDITIONAL_LANDMARK_TAGS,CONDITIONAL_LANDMARK_ROLESinsrc/pageScanner/index.js) were hardcoded inline, with the pro plugin maintaining its own separate, incomplete, out-of-sync copy for its Landmark filter dropdown.edac_get_landmark_types()inincludes/helper-functions.phpas the canonical PHP source, plusedac_get_landmark_filter_options()which flattens it into{ value, label }pairs for filter UIs.pageScanner/index.jsnow reads its detection constants fromwindow.scanOptions(matching the existingmaxAltLengthoverride pattern) with the same hardcoded values kept only as a fallback for contexts that don't localizescanOptions(e.g. standalone test runs).edac_get_landmark_types()into the editor scan'swp_localize_scriptpayload (edac_editor_app.landmarkTypes) and intocheckPage.js's iframescanOptionsinjection, alongside the existingmaxAltLengthoverride.Linear: https://linear.app/equalize-digital/issue/PRO-959/landmark-filter-options-replace-hardcoded-lists-with-a-single-source
Companion PR in accessibility-checker-pro consumes
edac_get_landmark_filter_options()for the Issues Explorer's Landmark filter.Test plan
<nav>,<aside>,role="navigation", androle="complementary"elements; confirm the savedlandmarkvalues match the expected tag/role strings (unchanged behavior — same hardcoded defaults, now sourced via localization)wp_localize_scriptdata present (e.g. direct script load without the editor flow) still works via the fallback constantsSummary by CodeRabbit