Skip to content

Conversation

@Miraeld
Copy link
Contributor

@Miraeld Miraeld commented Nov 26, 2025

Description

Fixes #7116
Avoid preloading empty href.

Type of change

  • Bug fix (non-breaking change which fixes an issue).

Detailed scenario

What was tested

  • All 144 existing tests pass
  • Added 11 new unit tests for empty href/src validation.
  • Tested with markup similar to the bug report: <a href=""> elements
  • Verified CSS background images with empty url() values are not captured

How to test

Verified CSS background images with empty url() values are not captured

Technical description

Documentation

This pull request improves the robustness of the BeaconLcp logic by ensuring that elements with empty or whitespace-only image sources are skipped, preventing invalid images from being processed or reported. It also expands the test suite to cover these edge cases, increasing confidence in the correctness of the implementation.

Robust handling of empty or whitespace-only sources:

  • The _getElementInfo method now returns null for img, svg image, video, and picture elements if their src, href, or poster attributes are empty or contain only whitespace. This prevents invalid image data from being included.
  • The _initWithFirstElementWithInfo method has been updated to ensure only elements with valid, non-empty src or srcset are considered for LCP (Largest Contentful Paint) analysis.

Regex improvement:

  • The regular expression used to extract URLs from CSS background properties has been refined to avoid matching empty or whitespace-only strings.

Expanded test coverage for edge cases:

  • New tests verify that elements with empty or whitespace-only src, href, or poster attributes are correctly skipped, and valid sources are properly handled for img, svg, video, and picture elements. [1] [2]

Mandatory Checklist

Code validation

  • I validated all the Acceptance Criteria. If possible, provide screenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I implemented built-in tests to cover the new/changed code.

Code style

  • I wrote a self-explanatory code about what it does.
  • I protected entry points against unexpected inputs.
  • I did not introduce unnecessary complexity.
  • Output messages (errors, notices, logs) are explicit enough for users to understand the issue and are actionnable.

- Updated regex pattern from /url\(\s*?['"]?\s*?(.+?)\s*?["']?\s*?\)/ig
  to /url\(\s*?['"]?([^'"\s]+)['"]?\s*?\)/ig
- Pattern now only matches non-empty URLs without whitespace
- Prevents matching empty or whitespace-only url() values
- Adds validation for empty src/href in img, video, svg, and picture elements
- Improves _initWithFirstElementWithInfo to handle empty/whitespace strings

Fixes: wp-media/wp-rocket#7116
@Miraeld Miraeld requested a review from a team November 26, 2025 03:44
@Miraeld Miraeld self-assigned this Nov 26, 2025
Copilot AI review requested due to automatic review settings November 26, 2025 03:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug where empty or whitespace-only URLs were being processed for LCP (Largest Contentful Paint) analysis. The changes ensure that elements with empty image sources are properly filtered out before being included in performance tracking.

  • Updated CSS background URL regex to prevent matching empty strings
  • Added validation to return null for elements with empty/whitespace-only sources
  • Enhanced filtering logic in _initWithFirstElementWithInfo to skip invalid sources

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/BeaconLcp.js Updated regex pattern and added empty/whitespace validation for video, SVG, and picture elements; enhanced filtering in _initWithFirstElementWithInfo
test/BeaconLcp.test.js Added comprehensive test coverage for empty and whitespace-only src/href handling across different element types
Comments suppressed due to low confidence (1)

src/BeaconLcp.js:109

  • Missing validation for empty or whitespace-only src in regular img elements. Unlike video, svg, and picture elements which now validate and return null for empty sources (lines 114-117, 121-124, 133-136), regular img elements lack this validation. This inconsistency means empty img src attributes could still be processed. Add validation similar to other element types:\njavascript\nelse if (nodeName === \"img\") {\n element_info.type = \"img\";\n element_info.src = element.src;\n element_info.current_src = element.currentSrc;\n if (!element_info.src || !element_info.src.trim()) {\n return null;\n }\n}\n
        if (nodeName === "img" && element.srcset) {
            element_info.type = "img-srcset";
            element_info.src = element.src;
            element_info.srcset = element.srcset;
            element_info.sizes = element.sizes;
            element_info.current_src = element.currentSrc;
        } else if (nodeName === "img") {
            element_info.type = "img";
            element_info.src = element.src;
            element_info.current_src = element.currentSrc;
        } else if (nodeName === "video") {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Update CSS background URL regex to handle quoted URLs with spaces
  using backreference pattern: /url\(\s*(['")?)(.*?)\1\s*\)/ig
- Improve hasSrc validation to explicitly check Array.isArray()
  for bg-img-set case instead of returning true for all non-strings
- Update bg_set mapping to use new regex capture groups (URL in m[2])
- Add filter to exclude empty URLs after regex matching

Addresses code review feedback from Copilot AI
@codacy-production
Copy link

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.96% (target: -1.00%) 86.21% (target: 50.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (641682e) 1946 1610 82.73%
Head commit (c6d4fac) 1968 (+22) 1647 (+37) 83.69% (+0.96%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#67) 29 25 86.21%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Avoid preloading empty href attributes with Optimize critical images

2 participants