Add contrast information data collection and output for contrast failures - #1589
Add contrast information data collection and output for contrast failures#1589pattonwebz wants to merge 10 commits into
Conversation
📝 WalkthroughWalkthroughThe PR adds support for storing and displaying color contrast metadata alongside accessibility violations. A new Changes
Sequence DiagramsequenceDiagram
participant PS as PageScanner
participant REST as REST API
participant DB as Database
participant AJAX as AJAX Handler
participant FE as Frontend UI
PS->>PS: processViolation()<br/>Detect color_contrast_failure
PS->>PS: Extract colors & ratios<br/>from check.data
PS->>REST: POST violation with<br/>extraData (colors, ratios)
REST->>REST: set_post_scan_results()<br/>Capture extraData parameter
REST->>REST: JSON encode extraData
REST->>DB: Insert_Rule_Data::insert()<br/>Store extra_data column
DB-->>REST: Row inserted/updated
FE->>AJAX: Fetch issue details
AJAX->>DB: SELECT with extra_data
DB-->>AJAX: Return row + extra_data
AJAX->>AJAX: esc_attr(extra_data)
AJAX-->>FE: Issue with data-extra-data
FE->>FE: descriptionOpen()<br/>Check extra_data.fgColor<br/>& extra_data.bgColor
FE->>FE: Render swatches<br/>& contrast ratio display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the accessibility checker by adding support for detailed color contrast data. It includes backend changes for data handling and storage, database schema updates, and frontend improvements for displaying the new data, providing users with more comprehensive information about color contrast issues. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 effectively adds functionality to collect, store, and display detailed color contrast information for accessibility issues. The changes are well-structured, spanning from the database schema and backend logic to the frontend UI. My review identified one area for improvement regarding data sanitization to ensure the new functionality is robust and secure.
| 'rule' => sanitize_text_field( $rule_data['rule'] ), | ||
| 'ruletype' => sanitize_text_field( $rule_data['ruletype'] ), | ||
| 'object' => esc_attr( $rule_data['object'] ), | ||
| 'extra_data' => isset( $rule_data['extra_data'] ) ? sanitize_text_field( $rule_data['extra_data'] ) : null, |
There was a problem hiding this comment.
Using sanitize_text_field on a JSON string is not a robust method for sanitization and can lead to data corruption. If the JSON string contains characters like \u003c or \u003e, they will be stripped or encoded, potentially breaking the JSON structure upon parsing. While this may not affect the current data (color codes and numbers), it's a fragile approach.
A more secure and reliable method would be to decode the JSON string, sanitize each value within the resulting array using appropriate functions (e.g., sanitize_hex_color for colors, floatval for numbers), and then re-encode the array to a JSON string. This would prevent data corruption and add a layer of security against potential injection vulnerabilities, especially since this data is used to generate inline styles on the frontend.
There was a problem hiding this comment.
Pull request overview
Adds end-to-end support for capturing, persisting, and displaying additional color contrast failure details (foreground/background colors and contrast ratios) alongside existing accessibility issue records.
Changes:
- Extend the page scanner to extract contrast check data and attach it to
color_contrast_failureviolations. - Add an
extra_dataJSON field to issue storage (DB schema + insert/update paths) and decode it when returning issue details. - Render contrast swatches/ratio in the Issue Details modal and the frontend highlighter panel when
extra_datais available.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/pageScanner/index.js |
Attaches extraData (contrast details) to contrast-failure violations. |
includes/classes/class-rest-api.php |
Accepts extraData in scan results and decodes extra_data when returning details. |
admin/class-insert-rule-data.php |
Stores optional extra_data JSON for issue records (insert + update). |
admin/class-update-database.php |
Adds extra_data column to the custom issues table schema. |
accessibility-checker.php |
Bumps EDAC_DB_VERSION to trigger schema update. |
admin/class-frontend-highlight.php |
Includes and decodes extra_data in frontend highlighter AJAX issue payloads. |
src/frontendHighlighterApp/index.js |
Displays contrast swatches/ratio in the highlighter panel. |
src/frontendHighlighterApp/sass/app.scss |
Styles for the new highlighter contrast UI. |
src/issueModal/components/IssueDetailsModal.js |
Displays contrast swatches/ratio in the issue details modal. |
admin/class-ajax.php |
Includes extra_data in details queries and outputs it as a data attribute on issue rows. |
| // Color contrast data | ||
| if ( matchingObj.extra_data?.fgColor && matchingObj.extra_data?.bgColor ) { | ||
| const { fgColor, bgColor, contrastRatio, expectedContrastRatio } = matchingObj.extra_data; | ||
| content += ` | ||
| <div class="edac-highlight-panel-description-contrast"> | ||
| <div class="edac-highlight-panel-description-contrast-swatches"> | ||
| <div class="edac-highlight-panel-description-contrast-swatch"> | ||
| <div class="edac-highlight-panel-description-contrast-swatch-color" style="background-color: ${ fgColor }; color: ${ bgColor };">Aa</div> | ||
| <div class="edac-highlight-panel-description-contrast-swatch-label">${ __( 'Foreground', 'accessibility-checker' ) }<br>${ fgColor }</div> | ||
| </div> | ||
| <div class="edac-highlight-panel-description-contrast-swatch"> | ||
| <div class="edac-highlight-panel-description-contrast-swatch-color" style="background-color: ${ bgColor }; color: ${ fgColor };">Aa</div> | ||
| <div class="edac-highlight-panel-description-contrast-swatch-label">${ __( 'Background', 'accessibility-checker' ) }<br>${ bgColor }</div> | ||
| </div> | ||
| </div> | ||
| <div class="edac-highlight-panel-description-contrast-ratio"> | ||
| ${ __( 'Contrast ratio:', 'accessibility-checker' ) } <strong>${ contrastRatio }:1</strong> (${ __( 'required:', 'accessibility-checker' ) } ${ expectedContrastRatio }) | ||
| </div> | ||
| </div> | ||
| `; |
There was a problem hiding this comment.
fgColor/bgColor/ratios are interpolated into an HTML string (including a style attribute) and then assigned via innerHTML. Because extra_data ultimately comes from scan results sent over the network and stored in the DB, this is an XSS risk if those fields are ever tampered with. Prefer building these nodes with createElement + textContent (and set styles only after validating allowed color formats), or at minimum escape the interpolated values before concatenating HTML.
| 'rule' => $rule, | ||
| 'ruletype' => $ruletype, | ||
| 'object' => esc_attr( $rule_obj ), | ||
| 'extra_data' => $extra_data ? wp_json_encode( $extra_data ) : null, | ||
| 'recordcheck' => 1, |
There was a problem hiding this comment.
$extra_data is JSON-encoded and stored with only sanitize_text_field() applied later, which does not validate structure or constrain values. Since this data originates from a client-provided REST payload, please whitelist expected keys and strictly sanitize/cast values (e.g., validate colors against a safe regex, cast ratios/font sizes to float/int) before encoding and persisting.
|
|
||
| $extra_data = isset( $violation['extraData'] ) && is_array( $violation['extraData'] ) ? $violation['extraData'] : null; | ||
|
|
||
| ( new Insert_Rule_Data() )->insert( $post, $actual_rule_id, $impact, $html, $landmark, $landmark_selector, $selectors, $extra_data ); |
There was a problem hiding this comment.
When accepting extraData from the REST request, it's only checked for is_array() and then passed through to DB storage. To prevent persisting untrusted/oversized payloads (and to support safe frontend rendering), validate that extraData contains only the expected scalar fields for contrast issues and sanitize/cast each value before calling insert().
| if ( isset( $result['extra_data'] ) && null !== $result['extra_data'] ) { | ||
| $result['extra_data'] = json_decode( $result['extra_data'], true ); |
There was a problem hiding this comment.
json_decode() is applied to extra_data without checking decode success. If the DB contains an empty string or malformed JSON, this will silently become null, which can make consumers handle extra_data inconsistently. Consider only replacing the value when decoding succeeds (check json_last_error()), and normalize the field to either null or an array consistently.
| if ( isset( $result['extra_data'] ) && null !== $result['extra_data'] ) { | |
| $result['extra_data'] = json_decode( $result['extra_data'], true ); | |
| if ( isset( $result['extra_data'] ) && '' !== $result['extra_data'] && null !== $result['extra_data'] ) { | |
| $decoded_extra_data = json_decode( $result['extra_data'], true ); | |
| if ( JSON_ERROR_NONE === json_last_error() && is_array( $decoded_extra_data ) ) { | |
| $result['extra_data'] = $decoded_extra_data; | |
| } else { | |
| $result['extra_data'] = null; | |
| } | |
| } else { | |
| $result['extra_data'] = null; |
| $array['extra_data'] = ! empty( $result['extra_data'] ) ? json_decode( $result['extra_data'], true ) : null; | ||
|
|
There was a problem hiding this comment.
extra_data is decoded from JSON without checking for decode errors. If the DB contains malformed JSON, this will quietly set extra_data to null and can make frontend behavior inconsistent. Consider verifying json_last_error() (or using a helper) and only returning decoded data when valid.
| $array['extra_data'] = ! empty( $result['extra_data'] ) ? json_decode( $result['extra_data'], true ) : null; | |
| if ( ! empty( $result['extra_data'] ) ) { | |
| $decoded_extra_data = json_decode( $result['extra_data'], true ); | |
| if ( JSON_ERROR_NONE === json_last_error() ) { | |
| $array['extra_data'] = $decoded_extra_data; | |
| } else { | |
| $array['extra_data'] = null; | |
| } | |
| } else { | |
| $array['extra_data'] = null; | |
| } |
![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)
src/pageScanner/index.js (1)
385-409:⚠️ Potential issue | 🟠 MajorExpand
extraDatato include relevant CSS context, not just computed values.The
extraDataobject stores computed colors, ratios, and font metrics, but omits the CSS declarations that produced the failing contrast pair. Per the PR requirement for "CSS context," this should also capture the relevant style properties (e.g.,color,background-color, etc.) to help identify hover/focus state issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pageScanner/index.js` around lines 385 - 409, When item.id === 'color_contrast_failure' and check.data exists, expand result.extraData to also include the CSS context that produced the failing pair: add a property (e.g., cssDeclarations or styleContext) to result.extraData that contains the relevant style properties such as color, background-color, background-image, border, opacity and any state-specific values (hover/focus) if available; pull these from check.data (or from the element's computed/style info available on the violation) so that result.extraData includes both the computed values (fgColor, bgColor, contrastRatio, etc.) and the originating CSS declarations for easier debugging (use the existing variables check.data and result.extraData to locate where to add this).
🧹 Nitpick comments (2)
admin/class-insert-rule-data.php (1)
166-166: Avoidsanitize_text_field()on an encoded JSON payload.Line 166 sanitizes a pre-encoded JSON string, which can alter structured data. Sanitize fields before
wp_json_encode(), then persist the JSON string as-is.Suggested patch
- 'extra_data' => isset( $rule_data['extra_data'] ) ? sanitize_text_field( $rule_data['extra_data'] ) : null, + 'extra_data' => ( isset( $rule_data['extra_data'] ) && is_string( $rule_data['extra_data'] ) ) + ? $rule_data['extra_data'] + : null,As per coding guidelines, "Follow WordPress security best practices (sanitization, validation, nonces) in all PHP code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/class-insert-rule-data.php` at line 166, The code is sanitizing an already-encoded JSON payload (using sanitize_text_field on $rule_data['extra_data']) which can corrupt structured data; instead sanitize and validate the individual fields of the array before calling wp_json_encode(), then store the resulting JSON string unchanged. Update the logic around $rule_data['extra_data'] in class-insert-rule-data.php (the place where extra_data is prepared/persisted) to remove sanitize_text_field from the encoded payload, sanitize each element of the source array (or run wp_kses/esc_* as appropriate), run wp_json_encode() on the sanitized array, and assign that JSON string to extra_data for storage.src/issueModal/components/IssueDetailsModal.js (1)
459-463: Guard ratio rendering when contrast metadata is incomplete.Line 461 and Line 462 can render
undefinedvalues whencontrastRatioorexpectedContrastRatiois missing. Please gate this block or provide a fallback string.Suggested patch
- <p className="edac-analysis__contrast-ratio"> - { __( 'Contrast ratio:', 'accessibility-checker' ) }{ ' ' } - <strong>{ issue.extra_data.contrastRatio }:1</strong>{ ' ' } - ({ __( 'required:', 'accessibility-checker' ) } { issue.extra_data.expectedContrastRatio }) - </p> + { issue?.extra_data?.contrastRatio && issue?.extra_data?.expectedContrastRatio && ( + <p className="edac-analysis__contrast-ratio"> + { __( 'Contrast ratio:', 'accessibility-checker' ) }{ ' ' } + <strong>{ issue.extra_data.contrastRatio }:1</strong>{ ' ' } + ({ __( 'required:', 'accessibility-checker' ) } { issue.extra_data.expectedContrastRatio }) + </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/issueModal/components/IssueDetailsModal.js` around lines 459 - 463, The contrast ratio block in IssueDetailsModal is rendering raw values that can be undefined; update the rendering logic in the IssueDetailsModal component to first check that issue.extra_data exists and that issue.extra_data.contrastRatio and issue.extra_data.expectedContrastRatio are present (or substitute safe fallbacks like 'N/A' or '—'), and only render the <p className="edac-analysis__contrast-ratio"> block (or its values) when those values are defined to avoid showing "undefined" or breaking the UI; adjust checks around issue.extra_data.contrastRatio and issue.extra_data.expectedContrastRatio accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@includes/classes/class-rest-api.php`:
- Around line 540-542: The $extra_data coming from the REST body must be
validated and normalized before persisting: in class-rest-api.php (before
calling ( new Insert_Rule_Data() )->insert(...)) whitelist allowed keys (e.g.
only accept the known keys used by admin/class-ajax.php and
src/frontendHighlighterApp/index.js), remove any unknown keys, and coerce/escape
each allowed value to a safe scalar (cast to string/int/bool as expected,
strip_tags/esc_html or use wp_kses_allowed_html for limited HTML, and enforce
length limits); if validation fails set $extra_data to null or an empty array;
then pass the sanitized $extra_data into Insert_Rule_Data::insert(). Ensure you
apply WordPress sanitization functions and document the accepted keys in a
comment.
---
Outside diff comments:
In `@src/pageScanner/index.js`:
- Around line 385-409: When item.id === 'color_contrast_failure' and check.data
exists, expand result.extraData to also include the CSS context that produced
the failing pair: add a property (e.g., cssDeclarations or styleContext) to
result.extraData that contains the relevant style properties such as color,
background-color, background-image, border, opacity and any state-specific
values (hover/focus) if available; pull these from check.data (or from the
element's computed/style info available on the violation) so that
result.extraData includes both the computed values (fgColor, bgColor,
contrastRatio, etc.) and the originating CSS declarations for easier debugging
(use the existing variables check.data and result.extraData to locate where to
add this).
---
Nitpick comments:
In `@admin/class-insert-rule-data.php`:
- Line 166: The code is sanitizing an already-encoded JSON payload (using
sanitize_text_field on $rule_data['extra_data']) which can corrupt structured
data; instead sanitize and validate the individual fields of the array before
calling wp_json_encode(), then store the resulting JSON string unchanged. Update
the logic around $rule_data['extra_data'] in class-insert-rule-data.php (the
place where extra_data is prepared/persisted) to remove sanitize_text_field from
the encoded payload, sanitize each element of the source array (or run
wp_kses/esc_* as appropriate), run wp_json_encode() on the sanitized array, and
assign that JSON string to extra_data for storage.
In `@src/issueModal/components/IssueDetailsModal.js`:
- Around line 459-463: The contrast ratio block in IssueDetailsModal is
rendering raw values that can be undefined; update the rendering logic in the
IssueDetailsModal component to first check that issue.extra_data exists and that
issue.extra_data.contrastRatio and issue.extra_data.expectedContrastRatio are
present (or substitute safe fallbacks like 'N/A' or '—'), and only render the <p
className="edac-analysis__contrast-ratio"> block (or its values) when those
values are defined to avoid showing "undefined" or breaking the UI; adjust
checks around issue.extra_data.contrastRatio and
issue.extra_data.expectedContrastRatio accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 35ba74b0-f6fa-40b8-bf42-7153d75f4719
📒 Files selected for processing (10)
accessibility-checker.phpadmin/class-ajax.phpadmin/class-frontend-highlight.phpadmin/class-insert-rule-data.phpadmin/class-update-database.phpincludes/classes/class-rest-api.phpsrc/frontendHighlighterApp/index.jssrc/frontendHighlighterApp/sass/app.scsssrc/issueModal/components/IssueDetailsModal.jssrc/pageScanner/index.js
| $extra_data = isset( $violation['extraData'] ) && is_array( $violation['extraData'] ) ? $violation['extraData'] : null; | ||
|
|
||
| ( new Insert_Rule_Data() )->insert( $post, $actual_rule_id, $impact, $html, $landmark, $landmark_selector, $selectors, $extra_data ); |
There was a problem hiding this comment.
Validate and normalize extraData before persisting it.
This field comes straight from the REST body and is later surfaced in the admin/front-end UIs (admin/class-ajax.php and src/frontendHighlighterApp/index.js). Storing arbitrary keys and strings here turns the new contrast metadata into a stored XSS/CSS-injection vector for anyone who can hit edit_post on the scanned post. Please whitelist the expected keys and cast each value to a safe format before calling Insert_Rule_Data::insert(). As per coding guidelines: Follow WordPress security best practices (sanitization, validation, nonces) in all PHP code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@includes/classes/class-rest-api.php` around lines 540 - 542, The $extra_data
coming from the REST body must be validated and normalized before persisting: in
class-rest-api.php (before calling ( new Insert_Rule_Data() )->insert(...))
whitelist allowed keys (e.g. only accept the known keys used by
admin/class-ajax.php and src/frontendHighlighterApp/index.js), remove any
unknown keys, and coerce/escape each allowed value to a safe scalar (cast to
string/int/bool as expected, strip_tags/esc_html or use wp_kses_allowed_html for
limited HTML, and enforce length limits); if validation fails set $extra_data to
null or an empty array; then pass the sanitized $extra_data into
Insert_Rule_Data::insert(). Ensure you apply WordPress sanitization functions
and document the accepted keys in a comment.
|
✅ Accessibility Checker build (primary only)
|
This pull request adds support for storing and displaying additional color contrast data for accessibility issues, specifically for color contrast failures. The changes span backend data handling, database schema, and frontend display, enabling more detailed color contrast information (such as foreground/background colors and contrast ratios) to be saved, retrieved, and shown in the user interface.
Database and Backend Enhancements:
extra_datacolumn to the accessibility checker table, and the plugin database version is incremented to1.0.7to reflect this change. [1] [2]extra_datafield (as JSON), ensuring that color contrast details can be saved with each relevant issue. [1] [2] [3] [4] [5] [6]extra_datafield, and it is properly decoded from JSON when retrieved. [1] [2] [3] [4] [5]Frontend Display Improvements:
Scanner and Data Collection:
color_contrast_failure, making this information available throughout the stack. [1] [2]Fixes: #1562
Checklist
Summary by CodeRabbit
New Features
Chores