feat: add Structure Map panel to frontend highlighter - #1678
feat: add Structure Map panel to frontend highlighter#1678SteveJonesDev wants to merge 7 commits into
Conversation
Adds a new Structure view tab alongside Issues in the frontend highlighter panel, providing a HeadingsMap-style overview of the page's heading hierarchy and landmark regions. - Headings tab: flat list with H1–H6 level badges, indentation via CSS custom property, live level-skip detection, and cross-reference against DB issues - Landmarks tab: nested tree reflecting DOM containment, built with compareDocumentPosition - Full ARIA tablist/tab/tabpanel pattern with roving tabindex and keyboard navigation (ArrowLeft/Right/Home/End) for both the view tabs and structure sub-tabs - Clicking a heading or landmark applies the existing visual highlight (outline + dark-blue label badge + smooth scroll) and clears any previously highlighted element - Fixed all: unset display regression on ul/li by adding display: block !important to structure list rules Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a multi-tab Structure view (Headings, Landmarks, Tab Order) to the front-end highlighter panel, implements accessible tab semantics and keyboard navigation, introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Panel as Highlight Panel (Tabs)
participant StructureMap as structureMap.js
participant DOM as Page DOM & Overlay
User->>Panel: Click or keyboard-select Structure tab
Panel->>Panel: update aria-selected/tabIndex<br/>toggle .edac-view--structure
Panel->>StructureMap: renderHeadingsPanel / renderLandmarksPanel / renderTabOrderPanel (container, callbacks)
StructureMap->>DOM: query headings / landmarks / focusable elements
StructureMap->>DOM: render list items in panel (with labels, warnings)
User->>Panel: Click item in panel
Panel->>StructureMap: item callback (onHeadingClick/onLandmarkClick/onTabOrderClick)
StructureMap->>DOM: scrollIntoView or focus target element
Panel->>DOM: applyHeadingHighlight / applyLandmarkHighlight / applyTabOrderHighlight
DOM->>DOM: draw overlay (SVG lines, badges) / apply highlight classes
User->>Panel: Close panel or switch tab
Panel->>DOM: remove overlay / cleanup highlight classes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. 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 a "Structure" view to the accessibility checker, allowing users to inspect the document's heading hierarchy and landmark regions via a new tabbed interface. The implementation includes logic to detect skipped heading levels, nested landmarks, and associated accessibility issues. Feedback focuses on optimizing performance by caching the structure map instead of rebuilding it on every tab switch, ensuring highlight labels remain correctly positioned during page scrolls, and correctly parsing multiple IDs in aria-labelledby attributes to comply with ARIA specifications.
| if ( index === 1 ) { | ||
| this.buildStructureMap(); | ||
| } |
There was a problem hiding this comment.
The structure map is completely rebuilt every time the "Structure" tab is selected. This involves querying the entire DOM for headings and landmarks and re-creating all associated DOM nodes and event listeners. For large pages, this can cause noticeable lag. Consider caching the rendered structure and only rebuilding it when the page is rescanned or when the structure view is first opened.
| landmarkLabel.style.left = ( rect.left + window.scrollX ) + 'px'; | ||
| landmarkLabel.style.top = ( rect.top + window.scrollY ) + 'px'; |
There was a problem hiding this comment.
The highlight labels for landmarks and headings are positioned using static absolute coordinates calculated at the moment of the click. If the user scrolls the page after the highlight is applied, the label badge will remain fixed at its original position while the highlighted element moves, leading to misalignment. Since the existing issue tooltips in this application use autoUpdate from Floating UI to handle this, a similar approach should be considered here for consistency and accuracy.
| const labelledby = el.getAttribute( 'aria-labelledby' ); | ||
| if ( labelledby ) { | ||
| const labelEl = document.getElementById( labelledby ); | ||
| if ( labelEl ) { | ||
| return labelEl.textContent.trim(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of getLandmarkLabel only supports a single ID in the aria-labelledby attribute. However, the ARIA specification allows aria-labelledby to contain a space-separated list of multiple IDs. Using document.getElementById on the entire attribute value will fail if multiple IDs are present.
| const labelledby = el.getAttribute( 'aria-labelledby' ); | |
| if ( labelledby ) { | |
| const labelEl = document.getElementById( labelledby ); | |
| if ( labelEl ) { | |
| return labelEl.textContent.trim(); | |
| } | |
| } | |
| const labelledby = el.getAttribute( 'aria-labelledby' ); | |
| if ( labelledby ) { | |
| return labelledby | |
| .split( /\s+/ ) | |
| .map( ( id ) => document.getElementById( id )?.textContent.trim() ) | |
| .filter( Boolean ) | |
| .join( ' ' ); | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a832d62f8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| headingLabel.style.left = ( rect.left + window.scrollX ) + 'px'; | ||
| headingLabel.style.top = ( rect.top + window.scrollY ) + 'px'; | ||
|
|
||
| document.body.appendChild( headingLabel ); |
There was a problem hiding this comment.
applyHeadingHighlight appends a persistent .edac-heading-label node to document.body, but the existing reset paths (showIssue() → removeSelectedClasses() and panelClose()) never call removeHeadingHighlights(). In practice, if a user clicks a heading in Structure view and then closes the panel or navigates issues, the dark-blue heading badge remains stuck on the page, which is a visible UI regression introduced by this feature.
Useful? React with 👍 / 👎.
| if ( index === 1 ) { | ||
| this.buildStructureMap(); |
There was a problem hiding this comment.
The Structure map is only rendered when switchView(1) runs. If a user opens Structure view before highlightAjax() resolves, renderStructureMap is built with this.issues || [] and never refreshed after this.issues is populated, so heading items miss DB-backed error flags until the user manually switches away and back. This race is reachable on slower responses and breaks the intended issue cross-referencing in the new panel.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/frontendHighlighterApp/index.js (2)
259-266: Consider caching or diffing to avoid unnecessary re-renders.
buildStructureMapis called every time the user switches to the Structure view (line 255). If the DOM hasn't changed, this re-queries all headings/landmarks and rebuilds the entire panel. For pages with many headings, this could cause noticeable delays.Consider adding a simple cache check (e.g., compare heading count or a generation timestamp) to skip re-rendering when the structure hasn't changed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontendHighlighterApp/index.js` around lines 259 - 266, buildStructureMap currently re-queries and calls renderStructureMap every time the Structure view is opened, causing unnecessary re-renders; add a lightweight cache check inside buildStructureMap that compares a small fingerprint (e.g., heading/landmark count, a generation timestamp, or shallow hash of this.panelStructure and this.issues) against a stored previousFingerprint and return early if unchanged, otherwise update previousFingerprint and call renderStructureMap(this.panelStructure, this.issues, el => this.applyLandmarkHighlight(el), el => this.applyHeadingHighlight(el)); this minimizes DOM work while preserving current behavior when the structure actually changes.
1768-1821: Extract duplicated inline style CSS to a shared constant.
applyLandmarkHighlightandapplyHeadingHighlightcontain nearly identical inline CSS strings (~12 lines each). Extracting this to a shared constant would reduce duplication and ensure consistent styling.Proposed refactor
// At module level or within the class const HIGHLIGHT_LABEL_STYLE = ` position: absolute; background: `#072446`; color: white; padding: 4px 8px; font-size: 12px; font-weight: bold; border-radius: 3px; z-index: 99998; pointer-events: none; font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, sans-serif; line-height: 1; box-shadow: 0 2px 4px rgba(0,0,0,0.2); `; // Then in both methods: landmarkLabel.style.cssText = HIGHLIGHT_LABEL_STYLE; // and headingLabel.style.cssText = HIGHLIGHT_LABEL_STYLE;Also applies to: 1832-1881
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontendHighlighterApp/index.js` around lines 1768 - 1821, The inline CSS string used to style label badges is duplicated in applyLandmarkHighlight and applyHeadingHighlight; extract that string to a single shared constant (e.g., HIGHLIGHT_LABEL_STYLE) at module-level or as a static/class-level constant and replace both uses so both landmarkLabel.style.cssText and headingLabel.style.cssText reference HIGHLIGHT_LABEL_STYLE, ensuring the constant contains the exact CSS currently assigned and is imported/visible to the class scope.src/frontendHighlighterApp/structureMap.js (1)
22-35:aria-labelledbycan contain multiple space-separated IDs — only the first is used here.Per the ARIA spec,
aria-labelledbycan reference multiple IDs (space-separated). This implementation only uses the first element found. While this works for most cases, consider handling multiple IDs for full spec compliance.Proposed enhancement for multi-ID support
function getLandmarkLabel( el ) { const labelledby = el.getAttribute( 'aria-labelledby' ); if ( labelledby ) { - const labelEl = document.getElementById( labelledby ); - if ( labelEl ) { - return labelEl.textContent.trim(); - } + const ids = labelledby.split( /\s+/ ); + const labelParts = ids + .map( ( id ) => document.getElementById( id )?.textContent?.trim() ) + .filter( Boolean ); + if ( labelParts.length ) { + return labelParts.join( ' ' ); + } } const ariaLabel = el.getAttribute( 'aria-label' ); if ( ariaLabel ) { return ariaLabel.trim(); } return ''; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontendHighlighterApp/structureMap.js` around lines 22 - 35, The getLandmarkLabel function currently reads only a single ID from aria-labelledby; change it to support multiple space-separated IDs by splitting the labelledby string on whitespace, iterating each id, fetching each element with document.getElementById, collecting each element.textContent (ignoring missing elements), joining collected texts with a single space and trimming the result before returning; keep the existing aria-label fallback and final empty-string return unchanged to preserve behavior when no labels are found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/frontendHighlighterApp/sass/app.scss`:
- Around line 1012-1017: The rule for .edac-structure-item-text uses the
deprecated declaration `word-break: break-word`; replace that declaration with
`overflow-wrap: break-word !important` (optionally keep a modern `word-break`
fallback like `normal` if desired) so the element retains the intended wrap
behavior while avoiding the deprecated value.
In `@src/frontendHighlighterApp/structureMap.js`:
- Around line 104-108: The Structure tab can render before element references
are set, so in buildStructureMap() (called from switchView()) change the
issue-matching logic to fall back to selector comparison when i.element is
undefined: instead of only checking i.element === el, also check i.selector (or
i.selectorText) against the DOM node (e.g., use el.matches(i.selector) or
compare data attributes) so hasDbIssue becomes true if either the resolved
element matches or the selector matches; update the check in the loop over
headingEls and ensure you reference the issues array and each issue's .element
and .selector properties (and keep the existing null-safe handling).
---
Nitpick comments:
In `@src/frontendHighlighterApp/index.js`:
- Around line 259-266: buildStructureMap currently re-queries and calls
renderStructureMap every time the Structure view is opened, causing unnecessary
re-renders; add a lightweight cache check inside buildStructureMap that compares
a small fingerprint (e.g., heading/landmark count, a generation timestamp, or
shallow hash of this.panelStructure and this.issues) against a stored
previousFingerprint and return early if unchanged, otherwise update
previousFingerprint and call renderStructureMap(this.panelStructure,
this.issues, el => this.applyLandmarkHighlight(el), el =>
this.applyHeadingHighlight(el)); this minimizes DOM work while preserving
current behavior when the structure actually changes.
- Around line 1768-1821: The inline CSS string used to style label badges is
duplicated in applyLandmarkHighlight and applyHeadingHighlight; extract that
string to a single shared constant (e.g., HIGHLIGHT_LABEL_STYLE) at module-level
or as a static/class-level constant and replace both uses so both
landmarkLabel.style.cssText and headingLabel.style.cssText reference
HIGHLIGHT_LABEL_STYLE, ensuring the constant contains the exact CSS currently
assigned and is imported/visible to the class scope.
In `@src/frontendHighlighterApp/structureMap.js`:
- Around line 22-35: The getLandmarkLabel function currently reads only a single
ID from aria-labelledby; change it to support multiple space-separated IDs by
splitting the labelledby string on whitespace, iterating each id, fetching each
element with document.getElementById, collecting each element.textContent
(ignoring missing elements), joining collected texts with a single space and
trimming the result before returning; keep the existing aria-label fallback and
final empty-string return unchanged to preserve behavior when no labels are
found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32726dbc-150e-459a-8f79-987b40252b43
📒 Files selected for processing (3)
src/frontendHighlighterApp/index.jssrc/frontendHighlighterApp/sass/app.scsssrc/frontendHighlighterApp/structureMap.js
| .edac-structure-item-text { | ||
| color: #1e1e1e !important; | ||
| word-break: break-word !important; | ||
| flex: 1 !important; | ||
| text-align: left !important; | ||
| } |
There was a problem hiding this comment.
Use overflow-wrap: break-word instead of deprecated word-break: break-word.
The break-word value for word-break is deprecated. Use overflow-wrap: break-word for the intended behavior.
Proposed fix
.edac-structure-item-text {
color: `#1e1e1e` !important;
- word-break: break-word !important;
+ overflow-wrap: break-word !important;
flex: 1 !important;
text-align: left !important;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .edac-structure-item-text { | |
| color: #1e1e1e !important; | |
| word-break: break-word !important; | |
| flex: 1 !important; | |
| text-align: left !important; | |
| } | |
| .edac-structure-item-text { | |
| color: `#1e1e1e` !important; | |
| overflow-wrap: break-word !important; | |
| flex: 1 !important; | |
| text-align: left !important; | |
| } |
🧰 Tools
🪛 Stylelint (17.9.0)
[error] 1014-1014: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontendHighlighterApp/sass/app.scss` around lines 1012 - 1017, The rule
for .edac-structure-item-text uses the deprecated declaration `word-break:
break-word`; replace that declaration with `overflow-wrap: break-word
!important` (optionally keep a modern `word-break` fallback like `normal` if
desired) so the element retains the intended wrap behavior while avoiding the
deprecated value.
| for ( const el of headingEls ) { | ||
| const level = parseInt( el.tagName[ 1 ] ); | ||
| const hasDbIssue = issues?.some( ( i ) => i.element === el ) ?? false; | ||
| const isSkip = prevLevel > 0 && level > prevLevel + 1; | ||
| prevLevel = level; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that issues array is populated with element references before buildStructureMap is called
# Check the flow in index.js
# Find where issues.element is set
rg -n "issues\[.*\]\.element\s*=" src/frontendHighlighterApp/index.js
# Check the order of operations - when is buildStructureMap called vs when are elements populated
rg -n -A5 "buildStructureMap|\.element\s*=" src/frontendHighlighterApp/index.js | head -60Repository: equalizedigital/accessibility-checker
Length of output: 608
🏁 Script executed:
# Get context around where elements are populated (line 1031)
sed -n '1020,1045p' src/frontendHighlighterApp/index.js
# Get context around where buildStructureMap is called (line 255)
sed -n '245,270p' src/frontendHighlighterApp/index.js
# Check if there's any async behavior that might affect ordering
rg -n "async|await|Promise|setTimeout|requestAnimationFrame" src/frontendHighlighterApp/index.js | head -20Repository: equalizedigital/accessibility-checker
Length of output: 1700
🏁 Script executed:
# Get context around line 255 to see the full function
sed -n '240,280p' src/frontendHighlighterApp/index.js
# Get the function names - what function contains each code section?
rg -n "^\s*(async\s+)?(\w+)\s*\(" src/frontendHighlighterApp/index.js | grep -E "(1020|1031|255|259)"
# Check if highlightAjax has a callback that might be tied to buildStructureMap
rg -n -B5 -A15 "highlightAjax()" src/frontendHighlighterApp/index.js | head -40Repository: equalizedigital/accessibility-checker
Length of output: 2703
🏁 Script executed:
# Find the function containing the "if ( index === 1 )" code
sed -n '200,260p' src/frontendHighlighterApp/index.js | head -40
# Search for where highlightAjax is called and what happens with its promise
rg -n -B10 -A10 "highlightAjax()" src/frontendHighlighterApp/index.js | head -80Repository: equalizedigital/accessibility-checker
Length of output: 3656
🏁 Script executed:
# Find the function containing line 1022
sed -n '990,1050p' src/frontendHighlighterApp/index.js
# Search for where switchView is called to understand the flow
rg -n "switchView" src/frontendHighlighterApp/index.jsRepository: equalizedigital/accessibility-checker
Length of output: 2114
Race condition exists — Structure tab can be clicked before element references are fully populated.
The code flow shows panelOpen() calls highlightAjax().then() to load and populate element references asynchronously. Meanwhile, switchView(index) is triggered directly from tab click handlers, which means a user could click the Structure tab before the AJAX response completes and this.issues[index].element assignments finish. When buildStructureMap() executes before elements are populated, the check issues?.some( ( i ) => i.element === el ) will silently fail because i.element will be undefined.
While the null-coalescing operator (?? false) prevents errors, the reference comparison won't match valid issues that lack element references yet. Adding a fallback comparison using selectors (as suggested) would guard against this timing dependency and ensure issues are matched correctly regardless of when the Structure panel is rendered relative to AJAX completion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontendHighlighterApp/structureMap.js` around lines 104 - 108, The
Structure tab can render before element references are set, so in
buildStructureMap() (called from switchView()) change the issue-matching logic
to fall back to selector comparison when i.element is undefined: instead of only
checking i.element === el, also check i.selector (or i.selectorText) against the
DOM node (e.g., use el.matches(i.selector) or compare data attributes) so
hasDbIssue becomes true if either the resolved element matches or the selector
matches; update the check in the loop over headingEls and ensure you reference
the issues array and each issue's .element and .selector properties (and keep
the existing null-safe handling).
Removes the nested Issues/Structure → Headings/Landmarks two-row tab pattern in favour of a single flat tablist: Issues | Headings | Landmarks. Eliminates the visual confusion of two stacked navigation rows and reduces clicks to reach structure views. - Removed renderStructureMap and its internal sub-tablist from structureMap.js - Exported renderHeadingsPanel and renderLandmarksPanel as top-level named exports - Added #edac-panel-headings and #edac-panel-landmarks tabpanels to the HTML template - Updated switchView, constructor, and init to handle three tabs - Replaced #edac-panel-structure flex-column layout and sub-tab CSS with simpler scrollable panel styles Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…landmark selection
Adds a fourth "Tab Order" tab that lists all focusable elements on the page in keyboard-navigation order, shows a taba11y-style SVG overlay with numbered badges and connecting lines, and highlights elements on click. Excludes all EDAC UI elements and the WP admin bar. Overlay redraws on window resize and is removed when the panel closes or the user switches to another tab. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces the per-element inline style approach with a body class (edac-tab-order-active) that hides all .edac-highlight-btn via CSS. This also covers buttons created after the tab switch (e.g. late AJAX). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Calls removeSelectedClasses, removeHeadingHighlights, and removeTabOrderHighlights at the top of switchView so any active highlight from the previous tab is always reset before the new tab renders. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontendHighlighterApp/index.js (1)
1254-1264:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPanel close path misses heading/tab-order cleanup
On close, you remove generic selected classes and tab-order overlay, but not
.edac-heading-label/.edac-tab-order-labeland their dedicated highlight classes. If the panel is closed while a structure item is active, artifacts can remain on-page.Suggested fix
panelClose() { if ( this.isDocked ) { this.removeDock(); } this.highlightPanel.classList.remove( 'edac-highlight-panel-visible' ); this.panelControls.style.display = 'none'; this.panelToggle.style.display = 'block'; this.removeSelectedClasses(); + this.removeHeadingHighlights(); + this.removeTabOrderHighlights(); this.removeHighlightButtons(); this.removeTabOrderOverlay();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontendHighlighterApp/index.js` around lines 1254 - 1264, The panelClose method leaves .edac-heading-label and .edac-tab-order-label DOM artifacts and their highlight classes behind; update panelClose (after removeTabOrderOverlay()) to also clear any heading/tab-order labels and their specific highlight classes by invoking or adding cleanup logic similar to removeSelectedClasses/removeHighlightButtons (e.g., a new removeHeadingLabels/removeTabOrderLabels helper or extend removeSelectedClasses to query for '.edac-heading-label' and '.edac-tab-order-label' and remove those elements and their dedicated highlight classes), ensuring no structure-item artifacts remain when the panel is closed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/frontendHighlighterApp/index.js`:
- Around line 222-225: The startup sequence applies
highlightLandmark(this.landmarkParameter) then calls switchView(2), but
switchView internally calls removeSelectedClasses() which clears the highlight;
fix by moving the call order (call this.switchView(2) before
this.highlightLandmark(this.landmarkParameter)) so the view switch doesn't wipe
the highlight, or alternatively modify switchView to accept a preserveSelection
flag and skip removeSelectedClasses when preserveSelection is true (update
callers accordingly); reference functions: panelOpen, highlightLandmark,
switchView, and removeSelectedClasses to locate the change.
- Around line 287-310: Wrap each panel builder (buildHeadingsPanel,
buildLandmarksPanel, buildTabOrderPanel) body in a try/catch so DOM assumptions
from renderHeadingsPanel, renderLandmarksPanel, renderTabOrderPanel or
overlayTabOrder don’t hard-fail the highlighter; on catch, clear the target
panel (this.panelHeadings/this.panelLandmarks/this.panelTabOrder), log the error
via an appropriate logger/console with context (which panel failed and the
error), and skip further steps like overlayTabOrder or adding the resize
listener (ensure _tabOrderResizeHandler is not registered if overlay failed).
This will allow graceful degradation when render* or overlayTabOrder throws.
---
Outside diff comments:
In `@src/frontendHighlighterApp/index.js`:
- Around line 1254-1264: The panelClose method leaves .edac-heading-label and
.edac-tab-order-label DOM artifacts and their highlight classes behind; update
panelClose (after removeTabOrderOverlay()) to also clear any heading/tab-order
labels and their specific highlight classes by invoking or adding cleanup logic
similar to removeSelectedClasses/removeHighlightButtons (e.g., a new
removeHeadingLabels/removeTabOrderLabels helper or extend removeSelectedClasses
to query for '.edac-heading-label' and '.edac-tab-order-label' and remove those
elements and their dedicated highlight classes), ensuring no structure-item
artifacts remain when the panel is closed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fe9867a3-98b3-40f3-8d0b-e3123dca75e7
📒 Files selected for processing (3)
src/frontendHighlighterApp/index.jssrc/frontendHighlighterApp/sass/app.scsssrc/frontendHighlighterApp/structureMap.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/frontendHighlighterApp/structureMap.js
| this.panelOpen(); | ||
| this.highlightLandmark( this.landmarkParameter ); | ||
| this.switchView( 2 ); | ||
| } else if ( this.isDocked ) { |
There was a problem hiding this comment.
Landmark deep-link highlight is cleared during startup
At Line 223 you apply landmark highlight, but Line 224 then calls switchView( 2 ), which immediately clears it via removeSelectedClasses(). This breaks the deep-link visual highlight behavior.
Suggested fix
- } else if ( this.landmarkParameter ) {
- this.panelOpen();
- this.highlightLandmark( this.landmarkParameter );
- this.switchView( 2 );
+ } else if ( this.landmarkParameter ) {
+ this.panelOpen();
+ this.switchView( 2 );
+ this.highlightLandmark( this.landmarkParameter );
} else if ( this.isDocked ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.panelOpen(); | |
| this.highlightLandmark( this.landmarkParameter ); | |
| this.switchView( 2 ); | |
| } else if ( this.isDocked ) { | |
| } else if ( this.landmarkParameter ) { | |
| this.panelOpen(); | |
| this.switchView( 2 ); | |
| this.highlightLandmark( this.landmarkParameter ); | |
| } else if ( this.isDocked ) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontendHighlighterApp/index.js` around lines 222 - 225, The startup
sequence applies highlightLandmark(this.landmarkParameter) then calls
switchView(2), but switchView internally calls removeSelectedClasses() which
clears the highlight; fix by moving the call order (call this.switchView(2)
before this.highlightLandmark(this.landmarkParameter)) so the view switch
doesn't wipe the highlight, or alternatively modify switchView to accept a
preserveSelection flag and skip removeSelectedClasses when preserveSelection is
true (update callers accordingly); reference functions: panelOpen,
highlightLandmark, switchView, and removeSelectedClasses to locate the change.
| buildHeadingsPanel() { | ||
| this.panelHeadings.innerHTML = ''; | ||
| renderHeadingsPanel( this.panelHeadings, this.issues || [], ( el ) => this.applyHeadingHighlight( el ) ); | ||
| } | ||
|
|
||
| buildLandmarksPanel() { | ||
| this.panelLandmarks.innerHTML = ''; | ||
| renderLandmarksPanel( this.panelLandmarks, ( el ) => this.applyLandmarkHighlight( el ), this._activeLandmarkEl ); | ||
| } | ||
|
|
||
| buildTabOrderPanel() { | ||
| this.panelTabOrder.innerHTML = ''; | ||
| renderTabOrderPanel( this.panelTabOrder, ( el, stopNumber, type ) => this.applyTabOrderHighlight( el, stopNumber, type ) ); | ||
| this.overlayTabOrder(); | ||
|
|
||
| if ( ! this._tabOrderResizeHandler ) { | ||
| let resizeTimer; | ||
| this._tabOrderResizeHandler = () => { | ||
| clearTimeout( resizeTimer ); | ||
| resizeTimer = setTimeout( () => this.overlayTabOrder(), 150 ); | ||
| }; | ||
| } | ||
| window.addEventListener( 'resize', this._tabOrderResizeHandler ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Guard structure panel rendering to avoid hard-failing the highlighter
The new structure render path (renderHeadingsPanel / renderLandmarksPanel / renderTabOrderPanel + overlay) is unguarded. If any DOM assumption throws, the panel can become unusable instead of degrading gracefully.
Suggested hardening pattern
buildHeadingsPanel() {
- this.panelHeadings.innerHTML = '';
- renderHeadingsPanel( this.panelHeadings, this.issues || [], ( el ) => this.applyHeadingHighlight( el ) );
+ try {
+ this.panelHeadings.innerHTML = '';
+ renderHeadingsPanel( this.panelHeadings, this.issues || [], ( el ) => this.applyHeadingHighlight( el ) );
+ } catch ( error ) {
+ this.panelHeadings.innerHTML = `<p class="edac-structure-empty">${ __( 'Unable to render headings.', 'accessibility-checker' ) }</p>`;
+ }
}As per coding guidelines: “Gracefully handle JavaScript errors to avoid breaking accessibility features.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontendHighlighterApp/index.js` around lines 287 - 310, Wrap each panel
builder (buildHeadingsPanel, buildLandmarksPanel, buildTabOrderPanel) body in a
try/catch so DOM assumptions from renderHeadingsPanel, renderLandmarksPanel,
renderTabOrderPanel or overlayTabOrder don’t hard-fail the highlighter; on
catch, clear the target panel
(this.panelHeadings/this.panelLandmarks/this.panelTabOrder), log the error via
an appropriate logger/console with context (which panel failed and the error),
and skip further steps like overlayTabOrder or adding the resize listener
(ensure _tabOrderResizeHandler is not registered if overlay failed). This will
allow graceful degradation when render* or overlayTabOrder throws.
|
✅ Accessibility Checker build (primary only)
|
1 participant
Adds a new Structure view tab alongside Issues in the frontend highlighter panel, providing a HeadingsMap-style overview of the page's heading hierarchy and landmark regions.
Checklist
Summary by CodeRabbit
New Features
Usability
Style