Add iframe missing title fix (backend + frontend) with tests - #1586
SteveJonesDev wants to merge 1 commit into
Conversation
SteveJonesDev
added
the
codex
ChatGPT Codex Connector
📝 WalkthroughWalkthroughThis change introduces an iframe accessibility fix that automatically adds missing titles to iframe elements. It includes a PHP fix class registered with the manager, an associated rule update, frontend JavaScript implementation with URL-based fallback title generation, and comprehensive test coverage spanning both backend and frontend. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 introduces an opt-in accessibility fix that automatically adds descriptive fallback 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 introduces a valuable accessibility fix for <iframe> elements that are missing a title attribute. The implementation is well-structured, covering both backend (PHP) and frontend (JavaScript) aspects, and includes corresponding tests. My review includes a few suggestions for improvement, primarily focusing on enhancing code clarity, efficiency, and test coverage. Specifically, I've pointed out a redundant test in the PHPUnit suite, opportunities to improve the Jest test suite's coverage of edge cases, and a minor refactoring in the main PHP fix class for better performance and readability.
| $fields[ 'edac_fix_' . $this->get_slug() ] = [ | ||
| 'label' => esc_html__( 'Add Missing iframe Titles', 'accessibility-checker' ), | ||
| 'type' => 'checkbox', | ||
| 'labelledby' => '', | ||
| 'description' => esc_html__( 'Adds a generated, descriptive title to iframe elements that are missing one.', 'accessibility-checker' ), | ||
| 'fix_slug' => $this->get_slug(), | ||
| ]; |
There was a problem hiding this comment.
The labelledby property is set to an empty string. In HTML, the labelledby attribute is used to associate an element with its label via an ID. An empty value is not valid and serves no purpose here. For cleaner code, it's better to remove this line if it's not being used.
$fields[ 'edac_fix_' . $this->get_slug() ] = [
'label' => esc_html__( 'Add Missing iframe Titles', 'accessibility-checker' ),
'type' => 'checkbox',
'description' => esc_html__( 'Adds a generated, descriptive title to iframe elements that are missing one.', 'accessibility-checker' ),
'fix_slug' => $this->get_slug(),
];| public function run(): void { | ||
| if ( ! get_option( 'edac_fix_' . $this->get_slug(), false ) ) { | ||
| return; | ||
| } | ||
|
|
||
| add_filter( | ||
| 'edac_filter_frontend_fixes_data', | ||
| function ( $data ) { | ||
| $data[ $this->get_slug() ] = [ | ||
| 'enabled' => get_option( 'edac_fix_' . $this->get_slug(), false ), | ||
| ]; | ||
| return $data; | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
This method can be made more efficient and clearer:
- The
get_option()call is performed twice. It's better to fetch the option value once at the start of the method. - Since you've already confirmed the fix is enabled with the
ifcondition, the'enabled'key in the data array can be hardcoded totrueinside the filter callback. get_slug()is a static method. It's a best practice to call it usingself::get_slug()instead of$this->get_slug()to make it clear that it's a static call.
Here is a suggested refactoring that applies these points:
public function run(): void {
if ( ! get_option( 'edac_fix_' . self::get_slug(), false ) ) {
return;
}
add_filter(
'edac_filter_frontend_fixes_data',
function ( $data ) {
$data[ self::get_slug() ] = [
'enabled' => true,
];
return $data;
}
);
}| /** | ||
| * Tests for iframe missing title frontend fix. | ||
| */ | ||
|
|
||
| describe( 'iframeMissingTitleFix', () => { | ||
| let iframeMissingTitleFix; | ||
|
|
||
| beforeEach( async () => { | ||
| document.body.innerHTML = ''; | ||
| jest.resetModules(); | ||
| window.edac_frontend_fixes = { | ||
| iframe_missing_title: { | ||
| enabled: true, | ||
| }, | ||
| }; | ||
|
|
||
| const module = await import( '../../../src/frontendFixes/Fixes/iframeMissingTitleFix.js' ); | ||
| iframeMissingTitleFix = module.default; | ||
| } ); | ||
|
|
||
| test( 'adds fallback title using iframe hostname when src exists', () => { | ||
| document.body.innerHTML = '<iframe src="https://www.youtube.com/embed/example"></iframe>'; | ||
|
|
||
| iframeMissingTitleFix(); | ||
|
|
||
| const iframe = document.querySelector( 'iframe' ); | ||
| expect( iframe.getAttribute( 'title' ) ).toBe( 'Embedded content from www.youtube.com' ); | ||
| } ); | ||
|
|
||
| test( 'adds generic fallback title when src is missing', () => { | ||
| document.body.innerHTML = '<iframe></iframe>'; | ||
|
|
||
| iframeMissingTitleFix(); | ||
|
|
||
| const iframe = document.querySelector( 'iframe' ); | ||
| expect( iframe.getAttribute( 'title' ) ).toBe( 'Embedded content' ); | ||
| } ); | ||
|
|
||
| test( 'does not overwrite existing title attributes', () => { | ||
| document.body.innerHTML = '<iframe src="https://example.com" title="Custom iframe title"></iframe>'; | ||
|
|
||
| iframeMissingTitleFix(); | ||
|
|
||
| const iframe = document.querySelector( 'iframe' ); | ||
| expect( iframe.getAttribute( 'title' ) ).toBe( 'Custom iframe title' ); | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
The existing tests cover the main functionality well. To make the tests more robust, consider adding a few more test cases for edge scenarios:
- An
iframewith an emptytitleattribute (e.g.,title=""). - An
iframewith atitleattribute containing only whitespace (e.g.,title=" "). - An
iframewith an invalidsrcattribute, to ensure it falls back to the generic title.
This will help ensure the fix behaves as expected in all situations.
| /** | ||
| * Test frontend data is provided when the setting is enabled. | ||
| * | ||
| * @return void | ||
| */ | ||
| public function test_iframe_missing_title_fix_data_is_added() { | ||
| update_option( 'edac_fix_iframe_missing_title', true ); | ||
| $this->fix->run(); | ||
|
|
||
| $data = apply_filters( 'edac_filter_frontend_fixes_data', [] ); | ||
| $this->assertArrayHasKey( 'iframe_missing_title', $data ); | ||
| $this->assertTrue( $data['iframe_missing_title']['enabled'] ); | ||
| } |
There was a problem hiding this comment.
This test method test_iframe_missing_title_fix_data_is_added appears to be redundant. The FixTestTrait that this test class uses already provides a test test_frontend_data_filter_includes_slug which covers the same functionality. You can rely on the trait's test and remove this duplicate method to keep the test suite DRY (Don't Repeat Yourself).
![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: 1f0b505bf5
ℹ️ 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".
| if ( src ) { | ||
| try { | ||
| const parsedUrl = new URL( src, window.location.href ); | ||
| return `Embedded content from ${ parsedUrl.hostname }`; |
There was a problem hiding this comment.
Handle URLs like about:blank, data: or javascript: before building the hostname-based title. new URL(src, window.location.href) parses these successfully but returns an empty hostname, so this code currently sets title="Embedded content from ", which is an awkward, non-descriptive label for screen readers instead of the intended generic fallback.
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: 1
🧹 Nitpick comments (2)
tests/jest/frontendFixes/iframeMissingTitleFix.test.js (1)
21-46: Good test coverage for core scenarios.Consider adding tests for these edge cases to strengthen coverage:
- Iframe with empty string title (
title="")- Iframe with whitespace-only title (
title=" ")- Iframe with invalid/malformed
srcURL- Behavior when fix is disabled (
enabled: false)📝 Example additional test cases
+ test( 'adds fallback title when title attribute is empty string', () => { + document.body.innerHTML = '<iframe src="https://example.com" title=""></iframe>'; + + iframeMissingTitleFix(); + + const iframe = document.querySelector( 'iframe' ); + expect( iframe.getAttribute( 'title' ) ).toBe( 'Embedded content from example.com' ); + } ); + + test( 'adds fallback title when title attribute is whitespace only', () => { + document.body.innerHTML = '<iframe src="https://example.com" title=" "></iframe>'; + + iframeMissingTitleFix(); + + const iframe = document.querySelector( 'iframe' ); + expect( iframe.getAttribute( 'title' ) ).toBe( 'Embedded content from example.com' ); + } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/jest/frontendFixes/iframeMissingTitleFix.test.js` around lines 21 - 46, Add tests to cover iframeMissingTitleFix edge cases: (1) an iframe with title="" should be treated as missing and receive the fallback (either hostname-based if src valid, or generic if not); (2) an iframe with a whitespace-only title (e.g., " ") should be treated as missing and receive the fallback; (3) an iframe with a malformed/invalid src (e.g., "not-a-url" or "http://") should not crash and should get the generic fallback title; and (4) when the fix is disabled via the function’s options (enabled: false) iframeMissingTitleFix should not change existing titles or set fallbacks—add assertions using querySelector to check title attributes for each case referencing iframeMissingTitleFix and its options.tests/phpunit/includes/classes/Fixes/Fix/IframeMissingTitleFixTest.php (1)
24-38: Method naming follows WP conventions.The
set_up()(snake_case) is correct per WordPress unit test conventions, whiletearDown()(camelCase) is the PHPUnit standard. This mixed convention is typical in WordPress test suites.Consider adding a test to verify that frontend data is not added when the fix is disabled:
📝 Optional test for disabled state
+ /** + * Test frontend data is not provided when the setting is disabled. + * + * `@return` void + */ + public function test_iframe_missing_title_fix_data_not_added_when_disabled() { + update_option( 'edac_fix_iframe_missing_title', false ); + $this->fix->run(); + + $data = apply_filters( 'edac_filter_frontend_fixes_data', [] ); + $this->assertArrayNotHasKey( 'iframe_missing_title', $data ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpunit/includes/classes/Fixes/Fix/IframeMissingTitleFixTest.php` around lines 24 - 38, The test class mixes WP and PHPUnit naming correctly (set_up and tearDown); add a new test method named test_frontend_data_not_added_when_disabled in IframeMissingTitleFixTest that mirrors the existing frontend-data-positive test but disables the fix first (use the fix instance IframeMissingTitleFix via $this->fix and call the fix's disable/turn-off API or set its enabled flag), execute the same action that would normally add frontend data, and assert that no frontend data was produced (use the same helper/assertion used by other tests in this suite—e.g., the helper from common_setup/common_teardown—to check the absence of frontend data).
🤖 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/frontendFixes/Fixes/iframeMissingTitleFix.js`:
- Around line 5-18: The fallback strings in getFallbackIframeTitle are not
internationalized; update getFallbackIframeTitle to use wp.i18n functions (e.g.,
const { __, sprintf } = wp.i18n) and replace the raw strings with translatable
calls: use sprintf( __('Embedded content from %s', 'your-text-domain'),
parsedUrl.hostname ) when hostname is available, and __('Embedded content',
'your-text-domain') as the generic fallback; ensure you import/destructure __
and sprintf from wp.i18n at the top of the file and replace the two literal
strings accordingly.
---
Nitpick comments:
In `@tests/jest/frontendFixes/iframeMissingTitleFix.test.js`:
- Around line 21-46: Add tests to cover iframeMissingTitleFix edge cases: (1) an
iframe with title="" should be treated as missing and receive the fallback
(either hostname-based if src valid, or generic if not); (2) an iframe with a
whitespace-only title (e.g., " ") should be treated as missing and receive the
fallback; (3) an iframe with a malformed/invalid src (e.g., "not-a-url" or
"http://") should not crash and should get the generic fallback title; and (4)
when the fix is disabled via the function’s options (enabled: false)
iframeMissingTitleFix should not change existing titles or set fallbacks—add
assertions using querySelector to check title attributes for each case
referencing iframeMissingTitleFix and its options.
In `@tests/phpunit/includes/classes/Fixes/Fix/IframeMissingTitleFixTest.php`:
- Around line 24-38: The test class mixes WP and PHPUnit naming correctly
(set_up and tearDown); add a new test method named
test_frontend_data_not_added_when_disabled in IframeMissingTitleFixTest that
mirrors the existing frontend-data-positive test but disables the fix first (use
the fix instance IframeMissingTitleFix via $this->fix and call the fix's
disable/turn-off API or set its enabled flag), execute the same action that
would normally add frontend data, and assert that no frontend data was produced
(use the same helper/assertion used by other tests in this suite—e.g., the
helper from common_setup/common_teardown—to check the absence of frontend data).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5dae50fd-f5d8-4819-a155-7f2533e9cd39
📒 Files selected for processing (7)
includes/classes/Fixes/Fix/IframeMissingTitleFix.phpincludes/classes/Fixes/FixesManager.phpincludes/classes/Rules/Rule/IframeMissingTitleRule.phpsrc/frontendFixes/Fixes/iframeMissingTitleFix.jssrc/frontendFixes/index.jstests/jest/frontendFixes/iframeMissingTitleFix.test.jstests/phpunit/includes/classes/Fixes/Fix/IframeMissingTitleFixTest.php
| const getFallbackIframeTitle = ( iframe ) => { | ||
| const src = iframe.getAttribute( 'src' )?.trim(); | ||
|
|
||
| if ( src ) { | ||
| try { | ||
| const parsedUrl = new URL( src, window.location.href ); | ||
| return `Embedded content from ${ parsedUrl.hostname }`; | ||
| } catch ( e ) { | ||
| // Continue to generic fallback if the URL can't be parsed. | ||
| } | ||
| } | ||
|
|
||
| return 'Embedded content'; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
User-facing strings should use WordPress i18n functions.
The fallback title strings "Embedded content from" and "Embedded content" are user-facing text that should be translatable for international users. As per coding guidelines, use wp.i18n functions for translation support.
🌐 Proposed fix for i18n support
+import { __, sprintf } from '@wordpress/i18n';
+
const iframeMissingTitle = window.edac_frontend_fixes?.iframe_missing_title || {
enabled: false,
};
const getFallbackIframeTitle = ( iframe ) => {
const src = iframe.getAttribute( 'src' )?.trim();
if ( src ) {
try {
const parsedUrl = new URL( src, window.location.href );
- return `Embedded content from ${ parsedUrl.hostname }`;
+ return sprintf(
+ /* translators: %s: hostname of the embedded content */
+ __( 'Embedded content from %s', 'accessibility-checker' ),
+ parsedUrl.hostname
+ );
} catch ( e ) {
// Continue to generic fallback if the URL can't be parsed.
}
}
- return 'Embedded content';
+ return __( 'Embedded content', 'accessibility-checker' );
};As per coding guidelines: "Use WordPress i18n functions (wp.i18n) for translation support in JavaScript files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontendFixes/Fixes/iframeMissingTitleFix.js` around lines 5 - 18, The
fallback strings in getFallbackIframeTitle are not internationalized; update
getFallbackIframeTitle to use wp.i18n functions (e.g., const { __, sprintf } =
wp.i18n) and replace the raw strings with translatable calls: use sprintf(
__('Embedded content from %s', 'your-text-domain'), parsedUrl.hostname ) when
hostname is available, and __('Embedded content', 'your-text-domain') as the
generic fallback; ensure you import/destructure __ and sprintf from wp.i18n at
the top of the file and replace the two literal strings accordingly.
1 participant
Motivation
titleattributes to<iframe>elements that are missing them to improve accessibility for screen reader users.Description
IframeMissingTitleFixthat registers a settings field and exposes frontend data when enabled.FixesManagerso it is loaded with other fixes.IframeMissingTitleRuleto reference the new fix in the rule definition and to update thehow_to_fixguidance.src/frontendFixes/Fixes/iframeMissingTitleFix.jswhich computes a hostname-based or generic fallback and setstitleon iframes without overwriting existing titles, and wire it intosrc/frontendFixes/index.jsfor lazy loading.tests/phpunit/.../IframeMissingTitleFixTest.phpfor the fix class and Jest teststests/jest/frontendFixes/iframeMissingTitleFix.test.jsfor the frontend behavior.Testing
tests/phpunit/includes/classes/Fixes/Fix/IframeMissingTitleFixTest.php, which succeeded.tests/jest/frontendFixes/iframeMissingTitleFix.test.js, which succeeded.Codex Task
Summary by CodeRabbit
Release Notes
New Features
Tests